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>