m_pkthdr.rcvif dangling pointer problem

[Daan Vreeken]

Hi Robert,

On Sunday 24 July 2011 10:43:59 Robert N. M. Watson wrote:
> On 24 Jul 2011, at 04:51, Ryan Stone wrote:
> > I ran headlong into this issue today when trying to test some network
> > stack changes.  It's depressingly easy to crash HEAD by periodically
> > destroying vlan interfaces while you are sending traffic over those
> > interfaces -- I get a panic within minutes.
> >
> >> http://people.freebsd.org/~glebius/patches/ifnet.no_free
> >
> > This patch makes my test system survive longer but does not resolve the
> > issue.
>
> Unfortunately, I'm a bit preoccupied currently, so haven't had a chance to
> follow up as yet, but just to follow up on the general issue here: this
> problem existed pre-SMP as well, and could be easily triggered by DUMMYNET
> and removable interfaces as well (as additional queuing delays just make
> the problem worse). In general: we need a solution that penalises interface
> removal, not common-case packet processing. As many packets have their
> source ifnet looked up in common-case processing (worth checking this
> assumption) because it's cheap, any solution that causes an interface
> lookup on every input packet (with synchronisation) is also an issue.
>
> Instead, I think we should go for a more radical notion, which is a bit
> harder to implement in our stack: the network stack needs a race-free way
> to "drain" all mbufs referring to a particular ifnet, which does not cause
> existing processing to become more expensive. This is easy in some
> subsystems, but more complex in others -- and the composition of subsystems
> makes it all much harder since we need to know that (to be 100% correct)
> packets aren't getting passed between subsystems (and hence belong to
> neither) in a way that races with a sweep through the subsystems. It may be
> possible to get this 99.9% right simply by providing a series of callbacks
> into subsystems that cause queues to be walked and drained of packets
> matching the doomed ifnet. It may also be quite cheap to have subsystems
> that "hold" packets outside of explicit queues for some period (i.e., in a
> thread-local pointer out of the stack) add explicit invalidation tests 
> (i.e., for IFF_DYING) before handing off to prevent those packets from
> traversing into other subsystems -- which can be done synchronisation-free,
> but still wouldn't 100% prevent the race
>
> Just to give an example: netisr should offer a method for
> netisr_drain_ifnet(struct ifnet *) that causes netisr to walk all of its
> queues to find matching packets and free them. Due to direct dispatch and
> thread-local queues during processing, netisr should also check IFF_DYING
> before handing off.
>
> If we do that, I wonder how robust the system then becomes...? This may not
> be too hard to test. But I'd rather we penalise ifnet removal than, say,
> the IP input path when it needs to check a source interface property.

Couldn't the dangling pointer problem be solved by adding a 'generation' field 
to the mbuf structure?
The 'generation' could be a system-wide number that gets incremented whenever 
an interface is removed. The mbuf* functions could keep a (per CPU?) 
reference count on the number of mbufs allocated/freed during 
that 'generation'. After interface removal, the ifnet structure could be 
freed when all the reference counters of generations before the current 
generation reach zero (whenever that happens).


--
Daan Vreeken
Vitsch Electronics
http://Vitsch.nl
tel: +31-(0)40-7113051 / +31-(0)6-46210825
KvK nr: 17174380