netdev - Re: [PATCH v1 net-next 4/4] net: Hold rtnl_net_lock() in (un)?register_netdevice_notifier_dev

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: <049d3a80-1b51-4796-83df-efb80f3b3107@nvidia.com>
Date: Wed, 29 Jan 2025 18:21:23 +0200
From: Yael Chemla <ychemla@...dia.com>
To: Kuniyuki Iwashima <kuniyu@...zon.com>
Cc: davem@...emloft.net, edumazet@...gle.com, horms@...nel.org,
 kuba@...nel.org, kuni1840@...il.com, netdev@...r.kernel.org,
 pabeni@...hat.com
Subject: Re: [PATCH v1 net-next 4/4] net: Hold rtnl_net_lock() in
 (un)?register_netdevice_notifier_dev_net().

On 28/01/2025 1:26, Kuniyuki Iwashima wrote:
> From: Yael Chemla <ychemla@...dia.com>
> Date: Mon, 20 Jan 2025 20:55:07 +0200
>>>>> diff --git a/net/core/dev.c b/net/core/dev.c
>>>>> index f6c6559e2548..a0dd34463901 100644
>>>>> --- a/net/core/dev.c
>>>>> +++ b/net/core/dev.c
>>>>> @@ -1943,15 +1943,17 @@ int register_netdevice_notifier_dev_net(struct net_device *dev,
>>>>>     					struct notifier_block *nb,
>>>>>     					struct netdev_net_notifier *nn)
>>>>>     {
>>>>> +	struct net *net = dev_net(dev);
>>>>
>>>> it seems to happen since the net pointer is acquired here without a lock.
>>>> Note that KASAN issue is not triggered when executing with rtnl_lock()
>>>> taken before this line. and our kernel .config expands
>>>> rtnl_net_lock(net) to rtnl_lock() (CONFIG_DEBUG_NET_SMALL_RTNL is not set).
>>>
>>> It sounds like the device was being moved to another netns while
>>> unregister_netdevice_notifier_dev_net() was called.
>>>
>>> Could you check if dev_net() is changed before/after rtnl_lock() in
>>>
>>>     * register_netdevice_notifier_dev_net()
>>>     * unregister_netdevice_notifier_dev_net()
>>>
>>> ?
>>
>> When checking dev_net before and after taking the lock the issue won’t
>> reproduce.
>> note that when issue reproduce we arrive to
>> unregister_netdevice_notifier_dev_net with an invalid net pointer
>> (verified it with prints of its value, and it's not the same consistent
>> value as is throughout rest of the test).
> 
> Does an invalid net pointer means a dead netns pointer ?
> dev_net() and dev_net_set() use rcu_dereference() and rcu_assign_pointer(),
> so I guess it should not be an invalid address at least.
> 
I logged several values at the entrance of 
unregister_netdevice_notifier_dev_net when issue reproduced:
1) fields of net->ns (struct ns_common):
count: the namespace refcount is 0 (i.e. net->ns.count, used 
refcount_read to read it).

inum: the value doesn't appear to be garbage but differ from its 
constant value throughout the test.

2) net pointer (struct net): value differ from its constant value 
observed during the rest of the test.

hope this helps and please let me know if more info is needed.

> 
>> we suspect the issue related to the async ns deletion.
> 
> I think async netns change would trigger the issue too.
> 
> Could you try this patch ?
> 

I tested your patch and issue won't reproduce with it 
(CONFIG_DEBUG_NET_SMALL_RTNL is not set in my config).

Tested-by: Yael Chemla <ychemla@...dia.com>

Thanks a lot!

> ---8<---
> diff --git a/net/core/dev.c b/net/core/dev.c
> index afa2282f2604..f4438ec24683 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -2070,20 +2070,50 @@ static void __move_netdevice_notifier_net(struct net *src_net,
>   	__register_netdevice_notifier_net(dst_net, nb, true);
>   }
>   
> +static void rtnl_net_dev_lock(struct net_device *dev)
> +{
> +	struct net *net;
> +
> +again:
> +	/* netns might be being dismantled. */
> +	net = maybe_get_net(dev_net(dev));
> +	if (!net) {
> +		cond_resched();
> +		goto again;
> +	}
> +
> +	rtnl_net_lock(net);
> +
> +	/* dev might be moved to another netns. */
> +	if (!net_eq(net, dev_net(dev))) {
> +		rtnl_net_unlock(net);
> +		put_net(net);
> +		cond_resched();
> +		goto again;
> +	}
> +}
> +
> +static void rtnl_net_dev_unlock(struct net_device *dev)
> +{
> +	struct net *net = dev_net(dev);
> +
> +	rtnl_net_unlock(net);
> +	put_net(net);
> +}
> +
>   int register_netdevice_notifier_dev_net(struct net_device *dev,
>   					struct notifier_block *nb,
>   					struct netdev_net_notifier *nn)
>   {
> -	struct net *net = dev_net(dev);
>   	int err;
>   
> -	rtnl_net_lock(net);
> -	err = __register_netdevice_notifier_net(net, nb, false);
> +	rtnl_net_dev_lock(dev);
> +	err = __register_netdevice_notifier_net(dev_net(dev), nb, false);
>   	if (!err) {
>   		nn->nb = nb;
>   		list_add(&nn->list, &dev->net_notifier_list);
>   	}
> -	rtnl_net_unlock(net);
> +	rtnl_net_dev_unlock(dev);
>   
>   	return err;
>   }
> @@ -2093,13 +2123,12 @@ int unregister_netdevice_notifier_dev_net(struct net_device *dev,
>   					  struct notifier_block *nb,
>   					  struct netdev_net_notifier *nn)
>   {
> -	struct net *net = dev_net(dev);
>   	int err;
>   
> -	rtnl_net_lock(net);
> +	rtnl_net_dev_lock(dev);
>   	list_del(&nn->list);
> -	err = __unregister_netdevice_notifier_net(net, nb);
> -	rtnl_net_unlock(net);
> +	err = __unregister_netdevice_notifier_net(dev_net(dev), nb);
> +	rtnl_net_dev_unlock(dev);
>   
>   	return err;
>   }
> ---8<---