Re: [PATCH] ipv4: remove all rt cache entries on UNREGISTER event

[Eric Dumazet <eric.dumazet@gmail.com>]

Le mercredi 29 septembre 2010 à 09:49 +0200, Nicolas Dichtel a écrit :
> Eric Dumazet wrote:
> > Le mardi 28 septembre 2010 à 18:45 +0200, Nicolas Dichtel a écrit :
> >> Eric Dumazet wrote:
> >>> Le mardi 28 septembre 2010 à 17:24 +0200, Nicolas Dichtel a écrit :
> >>>> Hi,
> >>>>
> >>>> I face a problem when I try to remove an interface, 
> >>>> netdev_wait_allrefs() complains about refcount.
> >>>>
> >>>> Here is a trivial scenario to reproduce the problem:
> >>>> # ip tunnel add mode ipip remote 10.16.0.164 local 10.16.0.72 dev eth0
> >>>> # ./a.out tunl1
> >>>> # ip tunnel del tunl1
> >>>>
> >>>> Note: a.out binary create an IPv4 raw socket, attach it to tunl1 
> >>>> (SO_BINDTODEVICE), set it as multicast (IP_MULTICAST_LOOP), set the 
> >>>> multicast interface to tunl1 (IP_MULTICAST_IF), build the IP header 
> >>>> (IP_HDRINCL) and then send a single packet (192.168.6.1 -> 224.0.0.18).
> >>>>
> >>>> Note2: when a.out is executed, tunl1 has no ip address and is down.
> >>>>
> >>> CC Octavian Purdila, the patch author.
> >>>
> >>> I am just wondering why this route is created in the first place.
> The route is created because no function will check interface status (up 
> and running or down). Just at the end, the packet will be enqueued in 
> the noop qdisc.
> 

In your case maybe, but I think there is another point where we can call
dev_hold() while device is in dismantle phase.


> >> At first, I asked myself the same question, but it seems that this is 
> >> allowed to send a packet through this kind of socket, even if interface 
> >> is down. Packet will be destroyed by the noop qdisk.
> >> But I agree that it is strange to perform route lookup and everything to 
> >>    destroy the packet at the end ...
> >> Maybe raw_sendmsg() can delete it directly ;-) ... or maybe 
> >> ip_route_output_flow().
> >>
> >> Any suggestions welcome.
> >>
> > 
> > Hmm...
> > 
> > One way to track this kind of problem would be to add a WARN_ON() in
> > dev_hold()
> > 
> > -> Check that when a reference on dev is taken, we are in a known state.
> > 
> > Something like this ?
> dev_hold() is done when interface is down, but before unregistering 
> process start.

Not on my machine. I can see the backtrace sometimes.

There is a race somewhere (maybe several), and your patch only reduce
the window of this race.

I am working on it.