From owner-cvs-src@FreeBSD.ORG Tue Oct 19 23:04:18 2004 Return-Path: Delivered-To: cvs-src@freebsd.org Received: from mx1.FreeBSD.org (mx1.freebsd.org [216.136.204.125]) by hub.freebsd.org (Postfix) with ESMTP id 6C22916A4CF for ; Tue, 19 Oct 2004 23:04:18 +0000 (GMT) Received: from c00l3r.networx.ch (c00l3r.networx.ch [62.48.2.2]) by mx1.FreeBSD.org (Postfix) with ESMTP id 7B75D43D49 for ; Tue, 19 Oct 2004 23:04:17 +0000 (GMT) (envelope-from andre@freebsd.org) Received: (qmail 63220 invoked from network); 19 Oct 2004 23:03:12 -0000 Received: from unknown (HELO freebsd.org) ([62.48.0.53]) (envelope-sender ) by c00l3r.networx.ch (qmail-ldap-1.03) with SMTP for ; 19 Oct 2004 23:03:12 -0000 Message-ID: <41759D75.1B6BDDC2@freebsd.org> Date: Wed, 20 Oct 2004 01:04:21 +0200 From: Andre Oppermann X-Mailer: Mozilla 4.8 [en] (Windows NT 5.0; U) X-Accept-Language: en MIME-Version: 1.0 To: Max Laier References: <200410191513.i9JFDUbf072176@repoman.freebsd.org> <200410192329.46723.max@love2party.net> <41758B35.D5340AEA@freebsd.org> <200410200035.19752.max@love2party.net> Content-Type: text/plain; charset=us-ascii Content-Transfer-Encoding: 7bit cc: Sam Leffler cc: src-committers@freebsd.org cc: cvs-all@freebsd.org cc: cvs-src@freebsd.org Subject: Re: cvs commit: src/sys/sys protosw.h src/sys/kern uipc_domain.cuipc_socket2.c X-BeenThere: cvs-src@freebsd.org X-Mailman-Version: 2.1.1 Precedence: list List-Id: CVS commit messages for the src tree List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 19 Oct 2004 23:04:18 -0000 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