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

[Max Laier]

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.

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

> > I'd suggest, that you remove the possibility to remove protocols
> > completely. It is very likely that there are no races with adding
> > protocols - though it might take "some time" for the protocol to be fully
> > useable - but the removal is critical.
>
> I don't think it should be a one-way street.  To be able to unload
> protocols is an important but seldomly used function and it's certainly not
> that a crash is guarnteed.  Far from it.
>
> > We also have to check that really all code can cope with the addition and
> > properly reinitializes it's view of the protocol arrays.
>
> 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 ...

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

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!

-- 
/"\  Best regards,                      | mlaier at freebsd.org
\ /  Max Laier                          | ICQ #67774661
 X   http://pf4freebsd.love2party.net/  | mlaier at EFnet
/ \  ASCII Ribbon Campaign              | Against HTML Mail and News
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 187 bytes
Desc: not available
Url : http://lists.freebsd.org/pipermail/cvs-src/attachments/20041020/eee0d38b/attachment.bin