cvs commit: src/sys/sys protosw.h src/sys/kern
andre at freebsd.org
Tue Oct 19 16:04:18 PDT 2004
Max Laier wrote:
> On Tuesday 19 October 2004 23:46, Andre Oppermann wrote:
> > > problems. For example, in ip_icmp.c line 457 ff we have:
> > >
> > > ctlfunc =
> > > inetsw[ip_protox[icp->icmp_ip.ip_p]].pr_ctlinput; if (ctlfunc)
> > > (*ctlfunc)(code, (struct sockaddr *)&icmpsrc,
> > > (void *)&icp->icmp_ip);
> > Ok, this one is easy to fix. I'll audit the code for any other of these
> > abuses.
> One of many, I am afraid.
Where are the others?
> > > This is clearly a problem if we can remove protocols. There might be more
> > > places where we (temporary) cache values from the protocol array. Another
> > > problem might be that we check for protocol existence early and assume
> > > that this remains true ...
> > Well, too bad if some code tries to remember this. Doesn't hurt then.
> > From my reading of many parts of the netinet/* code this is usually not
> > a problem and the code is rather well behaved. I refuse to take this
> > argument as reason to not have loadable protocols.
> "... usually ... rather ..." I really urge you, to reconsider. Many have
> argumented in the same way. I understand that it is nice to have this
> possibility, but it *does* cause *real* problems!
The choice is up to the protocol module writer. Unloading a protocol is
a very convinient function during prototyping. For the final version you
can refuse to unload.
> > The point of the protocol arrays is precisely to have them as the only
> > and sole place where such information is stored. Any code that copies
> > any part of it to its own private structures is horribly broken by design
> > and must be fixed anyway! (BTW: I'm not aware of any code within netinet/*
> > that does this.)
> I mentioned one above, I am sure there are others. Some as obvious as the one
> above, some less so ...
Please point them out. We can discuss specifics then instead of creating
a clout of FUD.
> > > Another point: If you really want to keep the possibility to remove a
> > > protocol, you have to introduce some busy counter that pervents removal
> > > while the kernel is inside a protocol function. This has to be handled by
> > > the protocol itself, but it has to be taken care of somehow.
> > Yes, the protocol has to be able to handle its own unloading. I have
> > documented that fact. If a protocol in unable to do so it should simply
> > refuse any unload attempts with EBUSY.
> Divert can be paniced with the sysctl code, btw. You have something like:
> SYSCTL_OUT; <-- this can be made to take *some* time
> lock; <-- this will panic once the lock is destroyed
> And there are other problems. Yes, it is not a problem in the common case, but
> you have to account for edge cases as well!
And you have to account for that unloads do not happen for every packet that
goes through the box.
More information about the cvs-all