netdev - Re: [patch net] devlink: change port event netdev notifier from per-net to global

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] [day] [month] [year] [list]

Message-ID: <Y+tm23NsQcWSAlhU@nanopsycho>
Date:   Tue, 14 Feb 2023 11:47:55 +0100
From:   Jiri Pirko <jiri@...nulli.us>
To:     Ido Schimmel <idosch@...sch.org>
Cc:     netdev@...r.kernel.org, davem@...emloft.net, kuba@...nel.org,
        pabeni@...hat.com, edumazet@...gle.com, jacob.e.keller@...el.com
Subject: Re: [patch net] devlink: change port event netdev notifier from
 per-net to global

Tue, Feb 14, 2023 at 09:41:36AM CET, idosch@...sch.org wrote:
>On Mon, Feb 06, 2023 at 10:41:51AM +0100, Jiri Pirko wrote:
>> From: Jiri Pirko <jiri@...dia.com>
>> 
>> Currently only the network namespace of devlink instance is monitored
>> for port events. If netdev is moved to a different namespace and then
>> unregistered, NETDEV_PRE_UNINIT is missed which leads to trigger
>> following WARN_ON in devl_port_unregister().
>> WARN_ON(devlink_port->type != DEVLINK_PORT_TYPE_NOTSET);
>> 
>> Fix this by changing the netdev notifier from per-net to global so no
>> event is missed.
>> 
>> Fixes: 02a68a47eade ("net: devlink: track netdev with devlink_port assigned")
>> Signed-off-by: Jiri Pirko <jiri@...dia.com>
>> ---
>>  net/core/devlink.c | 9 ++++++---
>>  1 file changed, 6 insertions(+), 3 deletions(-)
>> 
>> diff --git a/net/core/devlink.c b/net/core/devlink.c
>> index 032d6d0a5ce6..909a10e4b0dd 100644
>> --- a/net/core/devlink.c
>> +++ b/net/core/devlink.c
>> @@ -9979,7 +9979,7 @@ struct devlink *devlink_alloc_ns(const struct devlink_ops *ops,
>>  		goto err_xa_alloc;
>>  
>>  	devlink->netdevice_nb.notifier_call = devlink_netdevice_event;
>> -	ret = register_netdevice_notifier_net(net, &devlink->netdevice_nb);
>> +	ret = register_netdevice_notifier(&devlink->netdevice_nb);
>>  	if (ret)
>>  		goto err_register_netdevice_notifier;
>>  
>> @@ -10171,8 +10171,7 @@ void devlink_free(struct devlink *devlink)
>>  	xa_destroy(&devlink->snapshot_ids);
>>  	xa_destroy(&devlink->ports);
>>  
>> -	WARN_ON_ONCE(unregister_netdevice_notifier_net(devlink_net(devlink),
>> -						       &devlink->netdevice_nb));
>> +	WARN_ON_ONCE(unregister_netdevice_notifier(&devlink->netdevice_nb));
>>  
>>  	xa_erase(&devlinks, devlink->index);
>>  
>> @@ -10503,6 +10502,8 @@ static int devlink_netdevice_event(struct notifier_block *nb,
>>  		break;
>>  	case NETDEV_REGISTER:
>>  	case NETDEV_CHANGENAME:
>> +		if (devlink_net(devlink) != dev_net(netdev))
>> +			return NOTIFY_OK;
>>  		/* Set the netdev on top of previously set type. Note this
>>  		 * event happens also during net namespace change so here
>>  		 * we take into account netdev pointer appearing in this
>> @@ -10512,6 +10513,8 @@ static int devlink_netdevice_event(struct notifier_block *nb,
>>  					netdev);
>>  		break;
>>  	case NETDEV_UNREGISTER:
>> +		if (devlink_net(devlink) != dev_net(netdev))
>> +			return NOTIFY_OK;
>>  		/* Clear netdev pointer, but not the type. This event happens
>>  		 * also during net namespace change so we need to clear
>>  		 * pointer to netdev that is going to another net namespace.
>
>Since the notifier block is no longer registered per-netns it shouldn't
>be moved to a different netns on reload. I'm testing the following diff
>[1] against net-next (although it should be eventually submitted to
>net).

Oh yeah. That is needed. Thanks!

>
>[1]
>diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
>index d9cdbc047b49..efbee940bb03 100644
>--- a/include/linux/netdevice.h
>+++ b/include/linux/netdevice.h
>@@ -2858,8 +2858,6 @@ int unregister_netdevice_notifier(struct notifier_block *nb);
> int register_netdevice_notifier_net(struct net *net, struct notifier_block *nb);
> int unregister_netdevice_notifier_net(struct net *net,
> 				      struct notifier_block *nb);
>-void move_netdevice_notifier_net(struct net *src_net, struct net *dst_net,
>-				 struct notifier_block *nb);
> int register_netdevice_notifier_dev_net(struct net_device *dev,
> 					struct notifier_block *nb,
> 					struct netdev_net_notifier *nn);
>diff --git a/net/core/dev.c b/net/core/dev.c
>index 7307a0c15c9f..709b1a02820b 100644
>--- a/net/core/dev.c
>+++ b/net/core/dev.c
>@@ -1870,14 +1870,6 @@ static void __move_netdevice_notifier_net(struct net *src_net,
> 	__register_netdevice_notifier_net(dst_net, nb, true);
> }
> 
>-void move_netdevice_notifier_net(struct net *src_net, struct net *dst_net,
>-				 struct notifier_block *nb)
>-{
>-	rtnl_lock();
>-	__move_netdevice_notifier_net(src_net, dst_net, nb);
>-	rtnl_unlock();
>-}
>-
> int register_netdevice_notifier_dev_net(struct net_device *dev,
> 					struct notifier_block *nb,
> 					struct netdev_net_notifier *nn)
>diff --git a/net/devlink/dev.c b/net/devlink/dev.c
>index ab4e0f3c4e3d..c879c3c78e18 100644
>--- a/net/devlink/dev.c
>+++ b/net/devlink/dev.c
>@@ -343,8 +343,6 @@ static void devlink_reload_netns_change(struct devlink *devlink,
> 	 * reload process so the notifications are generated separatelly.
> 	 */
> 	devlink_notify_unregister(devlink);
>-	move_netdevice_notifier_net(curr_net, dest_net,
>-				    &devlink->netdevice_nb);
> 	write_pnet(&devlink->_net, dest_net);
> 	devlink_notify_register(devlink);
> }