cvs commit: src/sys/sys protosw.h src/sys/kern uipc_domain.cuipc_socket2.c

[Andre Oppermann]

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:
> 
> lock;
> unlock;
> SYSCTL_OUT; <-- this can be made to take *some* time
> lock;  <-- this will panic once the lock is destroyed

Oh well...

> 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.

-- 
Andre