Date: Tue, 4 May 2004 23:36:50 -0700 From: Luigi Rizzo <rizzo@icir.org> To: Colin Percival <colin.percival@wadham.ox.ac.uk> Cc: freebsd-net@freebsd.org Subject: Re: [patch] Verify that ifaddr_byindex(foo) != NULL Message-ID: <20040504233650.B2575@xorpc.icir.org> In-Reply-To: <6.1.0.6.1.20040505065826.03e1d510@popserver.sfu.ca>; from colin.percival@wadham.ox.ac.uk on Wed, May 05, 2004 at 07:02:33AM %2B0100 References: <6.1.0.6.1.20040504133711.03d1ce18@popserver.sfu.ca> <20040504063500.A37862@xorpc.icir.org> <6.1.0.6.1.20040505065826.03e1d510@popserver.sfu.ca>
next in thread | previous in thread | raw e-mail | index | archive | help
On Wed, May 05, 2004 at 07:02:33AM +0100, Colin Percival wrote: ... > >On Tue, May 04, 2004 at 01:42:20PM +0100, Colin Percival wrote: > >> if we're going to check that > >> 0 < ifp->if_index <= if_index, it seems that we should also be > >> checking that ifp->if_index corresponds to an interface which > >> still exists (rather than a gap left behind when an interface was > >> removed). > > > >well, the problem here and elsewhere is whether we trust the rcvif > >field or not > > Right; I wasn't sure if we did trust it. In particular, I wonder > about packets received immediately before an interface is removed. if the interface teardown code does not go through queued packets to invalidate the stale rcvif pointer (and it doesn't right now) we are certainly in trouble and no check is going to save us. If people are going to scrutinize the code, I suggest the following course of actions: + in if_attach() add a comment indicating that once the function is complete, if_index and ifaddr_byindex() are valid; + in the various if_output routines, when the rcvif field is not to be used anymore, set it to NULL to reduce the chance of trouble; + replace the useless checks as the one you found with assertions, comments, or remove them altogether; + in the interface teardown code, bzero() the struct ifnet before freeing, and add an XXX comment noting that at this point one should go and remove packets queued (ipintrq, arpintrq, reassembly queue, divert socket, maybe bpf, possibly dummynet pipes -- you see, the list easily becomes a long one; if we are smart, perhaps we can force places where packets accumulate to register a callback to be used at interface teardown to invalidate stale info in the mbuf metadata); Now part of the work in the last bullet might be saved if we replace rcvif with an if_index of some kind taken from a large, non-recycled namespace. On the other hand, considering that interface deletion is relatively rare and I am pretty sure it requires some kind of heavy housekeeping anyways, maybe the best approach is still to leave rcvif mostly unchanged (or use an if_index from a recycled namespace) and be more judicious when passing around pointers to interfaces. At least, when i wrote dummynet (which had similar issues as stored packets might held a reference to a firewall rule) i followed this approach, and it wasn't too hard. cheers luigi
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20040504233650.B2575>