Date: Tue, 19 Oct 2004 23:46:29 +0200 From: Andre Oppermann <andre@freebsd.org> To: Max Laier <max@love2party.net> Cc: cvs-src@freebsd.org Subject: Re: cvs commit: src/sys/sys protosw.h src/sys/kern uipc_domain.cuipc_socket2.c Message-ID: <41758B35.D5340AEA@freebsd.org> References: <200410191513.i9JFDUbf072176@repoman.freebsd.org> <417532A2.9000901@errno.com> <41753522.1E39FEAE@freebsd.org> <200410192329.46723.max@love2party.net>
next in thread | previous in thread | raw e-mail | index | archive | help
Max Laier wrote: > > On Tuesday 19 October 2004 17:39, Andre Oppermann wrote: > > Sam Leffler wrote: > > > Andre Oppermann wrote: > > > > andre 2004-10-19 15:13:30 UTC > > > > > > > > FreeBSD src repository > > > > > > > > Modified files: > > > > sys/sys protosw.h > > > > sys/kern uipc_domain.c uipc_socket2.c > > > > Log: > > > > Support for dynamically loadable and unloadable protocols within > > > > existing protocol families. > > > > > > I don't recall seeing this posted anywhere for comment. I have some > > > concerns about this general topic and this code seems incomplete (e.g. I > > > see no locking). > > > > Locking is not needed because there are no dead moments in transitioning > > from unregistered to registered and back. All calls to any of the protocol > > specific functions will return a valid result (even if it is only > > EOPNOTSUPP). There is no list manipulation going on. > > > > The caller of the function is required to assure that no dangeling sockets, > > references or memory allocations are left behind after unregistering. It's > > simply impossible to solve otherwise. For IPDIVERT which I have converted > > this works very well (it will simply refuse to unload if a divert socket is > > open). > > > > What remaining concerns do you have? > > I am also a bit worried about this. While it is a cool thing to have something > like this, but I am afraid that there is code that will trigger 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. > 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. > 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.) > 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. -- Andre
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?41758B35.D5340AEA>