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

[Nicolas Dichtel <nicolas.dichtel@6wind.com>]

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.

>> 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.

Regards,
Nicolas

> 
> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> index 83de0eb..54bef78 100644
> --- a/include/linux/netdevice.h
> +++ b/include/linux/netdevice.h
> @@ -1773,6 +1774,7 @@ static inline void dev_put(struct net_device *dev)
>   */
>  static inline void dev_hold(struct net_device *dev)
>  {
> +	WARN_ON(dev->reg_state != NETREG_REGISTERED);
>  	atomic_inc(&dev->refcnt);
>  }
>  
> 
>