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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <4CA2EF9C.9040909@6wind.com>
Date:	Wed, 29 Sep 2010 09:49:48 +0200
From:	Nicolas Dichtel <nicolas.dichtel@...nd.com>
To:	Eric Dumazet <eric.dumazet@...il.com>
CC:	netdev <netdev@...r.kernel.org>,
	Octavian Purdila <opurdila@...acom.com>
Subject: Re: [PATCH] ipv4: remove all rt cache entries on UNREGISTER event

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);
>  }
>  
> 
> 
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html