[patch net-next 0/2] net: devlink: move netdev notifier block to dest namespace during reload

[patch net-next 0/2] net: devlink: move netdev notifier block to dest namespace during reload

[Jiri Pirko]

From: Jiri Pirko <jiri@nvidia.com>

Patch #1 is just a dependency of patch #2, which is the actual fix.

Jiri Pirko (2):
  net: introduce a helper to move notifier block to different namespace
  net: devlink: move netdev notifier block to dest namespace during
    reload

 include/linux/netdevice.h |  2 ++
 net/core/dev.c            | 22 ++++++++++++++++++----
 net/core/devlink.c        |  5 ++++-
 3 files changed, 24 insertions(+), 5 deletions(-)

-- 
2.37.3

[patch net-next 1/2] net: introduce a helper to move notifier block to different namespace

[Jiri Pirko]

From: Jiri Pirko <jiri@nvidia.com>

Currently, net_dev() netdev notifier variant follows the netdev with
per-net notifier from namespace to namespace. This is implemented
by move_netdevice_notifiers_dev_net() helper.

For devlink it is needed to re-register per-net notifier during
devlink reload. Introduce a new helper called
move_netdevice_notifier_net() and share the unregister/register code
with existing move_netdevice_notifiers_dev_net() helper.

Signed-off-by: Jiri Pirko <jiri@nvidia.com>
---
 include/linux/netdevice.h |  2 ++
 net/core/dev.c            | 22 ++++++++++++++++++----
 2 files changed, 20 insertions(+), 4 deletions(-)

diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index d45713a06568..6be93b59cfea 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -2828,6 +2828,8 @@ 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 3bacee3bee78..762d7255065d 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -1876,6 +1876,22 @@ int unregister_netdevice_notifier_net(struct net *net,
 }
 EXPORT_SYMBOL(unregister_netdevice_notifier_net);
 
+void __move_netdevice_notifier_net(struct net *src_net, struct net *dst_net,
+				   struct notifier_block *nb)
+{
+	__unregister_netdevice_notifier_net(src_net, nb);
+	__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();
+}
+EXPORT_SYMBOL(move_netdevice_notifier_net);
+
 int register_netdevice_notifier_dev_net(struct net_device *dev,
 					struct notifier_block *nb,
 					struct netdev_net_notifier *nn)
@@ -1912,10 +1928,8 @@ static void move_netdevice_notifiers_dev_net(struct net_device *dev,
 {
 	struct netdev_net_notifier *nn;
 
-	list_for_each_entry(nn, &dev->net_notifier_list, list) {
-		__unregister_netdevice_notifier_net(dev_net(dev), nn->nb);
-		__register_netdevice_notifier_net(net, nn->nb, true);
-	}
+	list_for_each_entry(nn, &dev->net_notifier_list, list)
+		__move_netdevice_notifier_net(dev_net(dev), net, nn->nb);
 }
 
 /**
-- 
2.37.3

[patch net-next 2/2] net: devlink: move netdev notifier block to dest namespace during reload

[Jiri Pirko]

From: Jiri Pirko <jiri@nvidia.com>

The notifier block tracking netdev changes in devlink is registered
during devlink_alloc() per-net, it is then unregistered
in devlink_free(). When devlink moves from net namespace to another one,
the notifier block needs to move along.

Fix this by adding forgotten call to move the block.

Reported-by: Ido Schimmel <idosch@idosch.org>
Fixes: 02a68a47eade ("net: devlink: track netdev with devlink_port assigned")
Signed-off-by: Jiri Pirko <jiri@nvidia.com>
---
 net/core/devlink.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/net/core/devlink.c b/net/core/devlink.c
index 40fcdded57e6..ea0b319385fc 100644
--- a/net/core/devlink.c
+++ b/net/core/devlink.c
@@ -4502,8 +4502,11 @@ static int devlink_reload(struct devlink *devlink, struct net *dest_net,
 	if (err)
 		return err;
 
-	if (dest_net && !net_eq(dest_net, curr_net))
+	if (dest_net && !net_eq(dest_net, curr_net)) {
+		move_netdevice_notifier_net(curr_net, dest_net,
+					    &devlink->netdevice_nb);
 		write_pnet(&devlink->_net, dest_net);
+	}
 
 	err = devlink->ops->reload_up(devlink, action, limit, actions_performed, extack);
 	devlink_reload_failed_set(devlink, !!err);
-- 
2.37.3

Re: [patch net-next 2/2] net: devlink: move netdev notifier block to dest namespace during reload

[Ido Schimmel]

On Mon, Nov 07, 2022 at 03:52:13PM +0100, Jiri Pirko wrote:
> From: Jiri Pirko <jiri@nvidia.com>
> 
> The notifier block tracking netdev changes in devlink is registered
> during devlink_alloc() per-net, it is then unregistered
> in devlink_free(). When devlink moves from net namespace to another one,
> the notifier block needs to move along.
> 
> Fix this by adding forgotten call to move the block.
> 
> Reported-by: Ido Schimmel <idosch@idosch.org>
> Fixes: 02a68a47eade ("net: devlink: track netdev with devlink_port assigned")
> Signed-off-by: Jiri Pirko <jiri@nvidia.com>

Does not trigger with my reproducer. Will test the fix tonight in
regression and report tomorrow morning.

> ---
>  net/core/devlink.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/net/core/devlink.c b/net/core/devlink.c
> index 40fcdded57e6..ea0b319385fc 100644
> --- a/net/core/devlink.c
> +++ b/net/core/devlink.c
> @@ -4502,8 +4502,11 @@ static int devlink_reload(struct devlink *devlink, struct net *dest_net,
>  	if (err)
>  		return err;
>  
> -	if (dest_net && !net_eq(dest_net, curr_net))
> +	if (dest_net && !net_eq(dest_net, curr_net)) {
> +		move_netdevice_notifier_net(curr_net, dest_net,
> +					    &devlink->netdevice_nb);
>  		write_pnet(&devlink->_net, dest_net);
> +	}

I suggest adding this:

diff --git a/net/core/devlink.c b/net/core/devlink.c
index 83fd10aeddd5..3b5aedc93335 100644
--- a/net/core/devlink.c
+++ b/net/core/devlink.c
@@ -9843,8 +9843,8 @@ void devlink_free(struct devlink *devlink)
 
        xa_destroy(&devlink->snapshot_ids);
 
-       unregister_netdevice_notifier_net(devlink_net(devlink),
-                                         &devlink->netdevice_nb);
+       WARN_ON(unregister_netdevice_notifier_net(devlink_net(devlink),
+                                                 &devlink->netdevice_nb));
 
        xa_erase(&devlinks, devlink->index);

This tells about the failure right away. Instead, we saw random memory
corruptions in later tests.

>  
>  	err = devlink->ops->reload_up(devlink, action, limit, actions_performed, extack);
>  	devlink_reload_failed_set(devlink, !!err);
> -- 
> 2.37.3
> 

Re: [patch net-next 2/2] net: devlink: move netdev notifier block to dest namespace during reload

[Jiri Pirko]

Mon, Nov 07, 2022 at 05:52:08PM CET, idosch@idosch.org wrote:
>On Mon, Nov 07, 2022 at 03:52:13PM +0100, Jiri Pirko wrote:
>> From: Jiri Pirko <jiri@nvidia.com>
>> 
>> The notifier block tracking netdev changes in devlink is registered
>> during devlink_alloc() per-net, it is then unregistered
>> in devlink_free(). When devlink moves from net namespace to another one,
>> the notifier block needs to move along.
>> 
>> Fix this by adding forgotten call to move the block.
>> 
>> Reported-by: Ido Schimmel <idosch@idosch.org>
>> Fixes: 02a68a47eade ("net: devlink: track netdev with devlink_port assigned")
>> Signed-off-by: Jiri Pirko <jiri@nvidia.com>
>
>Does not trigger with my reproducer. Will test the fix tonight in
>regression and report tomorrow morning.

Ok!

>
>> ---
>>  net/core/devlink.c | 5 ++++-
>>  1 file changed, 4 insertions(+), 1 deletion(-)
>> 
>> diff --git a/net/core/devlink.c b/net/core/devlink.c
>> index 40fcdded57e6..ea0b319385fc 100644
>> --- a/net/core/devlink.c
>> +++ b/net/core/devlink.c
>> @@ -4502,8 +4502,11 @@ static int devlink_reload(struct devlink *devlink, struct net *dest_net,
>>  	if (err)
>>  		return err;
>>  
>> -	if (dest_net && !net_eq(dest_net, curr_net))
>> +	if (dest_net && !net_eq(dest_net, curr_net)) {
>> +		move_netdevice_notifier_net(curr_net, dest_net,
>> +					    &devlink->netdevice_nb);
>>  		write_pnet(&devlink->_net, dest_net);
>> +	}
>
>I suggest adding this:
>
>diff --git a/net/core/devlink.c b/net/core/devlink.c
>index 83fd10aeddd5..3b5aedc93335 100644
>--- a/net/core/devlink.c
>+++ b/net/core/devlink.c
>@@ -9843,8 +9843,8 @@ void devlink_free(struct devlink *devlink)
> 
>        xa_destroy(&devlink->snapshot_ids);
> 
>-       unregister_netdevice_notifier_net(devlink_net(devlink),
>-                                         &devlink->netdevice_nb);
>+       WARN_ON(unregister_netdevice_notifier_net(devlink_net(devlink),
>+                                                 &devlink->netdevice_nb));
> 
>        xa_erase(&devlinks, devlink->index);
>
>This tells about the failure right away. Instead, we saw random memory
>corruptions in later tests.

Should be a separate patch then.


>
>>  
>>  	err = devlink->ops->reload_up(devlink, action, limit, actions_performed, extack);
>>  	devlink_reload_failed_set(devlink, !!err);
>> -- 
>> 2.37.3
>> 

Re: [patch net-next 1/2] net: introduce a helper to move notifier block to different namespace

[Jakub Kicinski]

On Mon,  7 Nov 2022 15:52:12 +0100 Jiri Pirko wrote:
> +void __move_netdevice_notifier_net(struct net *src_net, struct net *dst_net,
> +				   struct notifier_block *nb)
> +{
> +	__unregister_netdevice_notifier_net(src_net, nb);
> +	__register_netdevice_notifier_net(dst_net, nb, true);
> +}

'static' missing

> +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();
> +}
> +EXPORT_SYMBOL(move_netdevice_notifier_net);

Do we need to export this?  Maybe let's wait for a module user?

Re: [patch net-next 1/2] net: introduce a helper to move notifier block to different namespace

[Jiri Pirko]

Tue, Nov 08, 2022 at 05:29:37AM CET, kuba@kernel.org wrote:
>On Mon,  7 Nov 2022 15:52:12 +0100 Jiri Pirko wrote:
>> +void __move_netdevice_notifier_net(struct net *src_net, struct net *dst_net,
>> +				   struct notifier_block *nb)
>> +{
>> +	__unregister_netdevice_notifier_net(src_net, nb);
>> +	__register_netdevice_notifier_net(dst_net, nb, true);
>> +}
>
>'static' missing

Yep.

>
>> +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();
>> +}
>> +EXPORT_SYMBOL(move_netdevice_notifier_net);
>
>Do we need to export this?  Maybe let's wait for a module user?

Ah, right.

Re: [patch net-next 2/2] net: devlink: move netdev notifier block to dest namespace during reload

[Ido Schimmel]

On Mon, Nov 07, 2022 at 03:52:13PM +0100, Jiri Pirko wrote:
> From: Jiri Pirko <jiri@nvidia.com>
> 
> The notifier block tracking netdev changes in devlink is registered
> during devlink_alloc() per-net, it is then unregistered
> in devlink_free(). When devlink moves from net namespace to another one,
> the notifier block needs to move along.
> 
> Fix this by adding forgotten call to move the block.
> 
> Reported-by: Ido Schimmel <idosch@idosch.org>
> Fixes: 02a68a47eade ("net: devlink: track netdev with devlink_port assigned")
> Signed-off-by: Jiri Pirko <jiri@nvidia.com>

Reviewed-by: Ido Schimmel <idosch@nvidia.com>
Tested-by: Ido Schimmel <idosch@nvidia.com>

Thanks!

Re: [patch net-next 2/2] net: devlink: move netdev notifier block to dest namespace during reload

[Jiri Pirko]

Tue, Nov 08, 2022 at 09:11:49AM CET, idosch@idosch.org wrote:
>On Mon, Nov 07, 2022 at 03:52:13PM +0100, Jiri Pirko wrote:
>> From: Jiri Pirko <jiri@nvidia.com>
>> 
>> The notifier block tracking netdev changes in devlink is registered
>> during devlink_alloc() per-net, it is then unregistered
>> in devlink_free(). When devlink moves from net namespace to another one,
>> the notifier block needs to move along.
>> 
>> Fix this by adding forgotten call to move the block.
>> 
>> Reported-by: Ido Schimmel <idosch@idosch.org>
>> Fixes: 02a68a47eade ("net: devlink: track netdev with devlink_port assigned")
>> Signed-off-by: Jiri Pirko <jiri@nvidia.com>
>
>Reviewed-by: Ido Schimmel <idosch@nvidia.com>
>Tested-by: Ido Schimmel <idosch@nvidia.com>

Sending v2 with cosmetical changes. Please put your tags there again.
Thanks!

>
>Thanks!

Re: [patch net-next 2/2] net: devlink: move netdev notifier block to dest namespace during reload

[Jiri Pirko]

Tue, Nov 08, 2022 at 01:59:50PM CET, jiri@resnulli.us wrote:
>Tue, Nov 08, 2022 at 09:11:49AM CET, idosch@idosch.org wrote:
>>On Mon, Nov 07, 2022 at 03:52:13PM +0100, Jiri Pirko wrote:
>>> From: Jiri Pirko <jiri@nvidia.com>
>>> 
>>> The notifier block tracking netdev changes in devlink is registered
>>> during devlink_alloc() per-net, it is then unregistered
>>> in devlink_free(). When devlink moves from net namespace to another one,
>>> the notifier block needs to move along.
>>> 
>>> Fix this by adding forgotten call to move the block.
>>> 
>>> Reported-by: Ido Schimmel <idosch@idosch.org>
>>> Fixes: 02a68a47eade ("net: devlink: track netdev with devlink_port assigned")
>>> Signed-off-by: Jiri Pirko <jiri@nvidia.com>
>>
>>Reviewed-by: Ido Schimmel <idosch@nvidia.com>
>>Tested-by: Ido Schimmel <idosch@nvidia.com>
>
>Sending v2 with cosmetical changes. Please put your tags there again.

Actually, this patch stays untouched. So I'll add it.

>Thanks!
>
>>
>>Thanks!