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

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: <Y2k/TUCa3/BXne9Q@nanopsycho>
Date:   Mon, 7 Nov 2022 18:24:29 +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, bigeasy@...utronix.de,
        imagedong@...cent.com, kuniyu@...zon.com, petrm@...dia.com
Subject: Re: [patch net-next 2/2] net: devlink: move netdev notifier block to
 dest namespace during reload

Mon, Nov 07, 2022 at 05:52:08PM CET, idosch@...sch.org wrote:
>On Mon, Nov 07, 2022 at 03:52:13PM +0100, Jiri Pirko wrote:
>> From: Jiri Pirko <jiri@...dia.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@...sch.org>
>> Fixes: 02a68a47eade ("net: devlink: track netdev with devlink_port assigned")
>> Signed-off-by: Jiri Pirko <jiri@...dia.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
>>