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 for Android: free password hash cracker in your pocket

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

Message-ID: <20250130003218.69951-1-kuniyu@amazon.com>
Date: Wed, 29 Jan 2025 16:32:18 -0800
From: Kuniyuki Iwashima <kuniyu@...zon.com>
To: <ychemla@...dia.com>
CC: <davem@...emloft.net>, <edumazet@...gle.com>, <horms@...nel.org>,
	<kuba@...nel.org>, <kuni1840@...il.com>, <kuniyu@...zon.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().

From: Yael Chemla <ychemla@...dia.com>
Date: Wed, 29 Jan 2025 18:21:23 +0200
> 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).

This indicates here we race with cleanup_net().

> 
> 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 for testing !

Will post the fix officially.