netdev - Re: [PATCH net 4/4] bonding: fix xfrm state handling when clearing active slave

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: <ff8d2230-245a-4675-aca1-775be6b03777@blackwall.org>
Date: Mon, 19 Aug 2024 10:38:01 +0300
From: Nikolay Aleksandrov <razor@...ckwall.org>
To: Hangbin Liu <liuhangbin@...il.com>
Cc: netdev@...r.kernel.org, Taehee Yoo <ap420073@...il.com>,
 davem@...emloft.net, jv@...sburgh.net, andy@...yhouse.net,
 edumazet@...gle.com, kuba@...nel.org, pabeni@...hat.com, jarod@...hat.com
Subject: Re: [PATCH net 4/4] bonding: fix xfrm state handling when clearing
 active slave

On 19/08/2024 06:05, Hangbin Liu wrote:
> On Fri, Aug 16, 2024 at 02:48:13PM +0300, Nikolay Aleksandrov wrote:
>> If the active slave is cleared manually the xfrm state is not flushed.
>> This leads to xfrm add/del imbalance and adding the same state multiple
>> times. For example when the device cannot handle anymore states we get:
>>  [ 1169.884811] bond0: (slave eni0np1): bond_ipsec_add_sa_all: failed to add SA
>> because it's filled with the same state after multiple active slave
>> clearings. This change also has a few nice side effects: user-space
>> gets a notification for the change, the old device gets its mac address
>> and promisc/mcast adjusted properly.
>>
>> Fixes: 18cb261afd7b ("bonding: support hardware encryption offload to slaves")
>> Signed-off-by: Nikolay Aleksandrov <razor@...ckwall.org>
>> ---
>> Please review this one more carefully. I plan to add a selftest with
>> netdevsim for this as well.
>>
>>  drivers/net/bonding/bond_options.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/bonding/bond_options.c b/drivers/net/bonding/bond_options.c
>> index bc80fb6397dc..95d59a18c022 100644
>> --- a/drivers/net/bonding/bond_options.c
>> +++ b/drivers/net/bonding/bond_options.c
>> @@ -936,7 +936,7 @@ static int bond_option_active_slave_set(struct bonding *bond,
>>  	/* check to see if we are clearing active */
>>  	if (!slave_dev) {
>>  		netdev_dbg(bond->dev, "Clearing current active slave\n");
>> -		RCU_INIT_POINTER(bond->curr_active_slave, NULL);
>> +		bond_change_active_slave(bond, NULL);
> 
> The good part of this is we can do bond_ipsec_del_sa_all and
> bond_ipsec_add_sa_all. I'm not sure if we should do promisc/mcast adjustment
> when set active_slave to null.
> 
> Jay should know better.
> 
> Thanks
> Hangbin

Jay please correct me, but I'm pretty sure we should adjust them. They get adjusted on
every active slave change, this is no different. In fact I'd argue that it's a long
standing bug because they don't get adjusted when the active slave is cleared
manually and if a new one is chosen (we call bond_select_active_slave() right after)
then the old one would still have them set. During normal operations and automatic
curr active slave changes, it is always adjusted.

>>  		bond_select_active_slave(bond);
>>  	} else {
>>  		struct slave *old_active = rtnl_dereference(bond->curr_active_slave);
>> -- 
>> 2.44.0
>>