netdev - Re: [PATCHv3 net 1/3] bonding: move IPsec deletion to bond_ipsec_free

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: <f88b234a-37ec-46a4-b920-35f598ab6c38@blackwall.org>
Date: Thu, 27 Feb 2025 15:31:01 +0200
From: Nikolay Aleksandrov <razor@...ckwall.org>
To: Hangbin Liu <liuhangbin@...il.com>
Cc: netdev@...r.kernel.org, Jay Vosburgh <jv@...sburgh.net>,
 Andrew Lunn <andrew+netdev@...n.ch>, "David S. Miller"
 <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>,
 Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
 Simon Horman <horms@...nel.org>, Shuah Khan <shuah@...nel.org>,
 Tariq Toukan <tariqt@...dia.com>, Jianbo Liu <jianbol@...dia.com>,
 Jarod Wilson <jarod@...hat.com>,
 Steffen Klassert <steffen.klassert@...unet.com>,
 Cosmin Ratiu <cratiu@...dia.com>, linux-kselftest@...r.kernel.org,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCHv3 net 1/3] bonding: move IPsec deletion to
 bond_ipsec_free_sa

On 2/27/25 15:21, Hangbin Liu wrote:
> On Thu, Feb 27, 2025 at 11:21:51AM +0200, Nikolay Aleksandrov wrote:
>>>> @@ -617,6 +611,12 @@ static void bond_ipsec_del_sa_all(struct bonding *bond)
>>>>  
>>>>  	mutex_lock(&bond->ipsec_lock);
>>>>  	list_for_each_entry(ipsec, &bond->ipsec_list, list) {
>>>> +		if (ipsec->xs->km.state == XFRM_STATE_DEAD) {
>>>> +			list_del(&ipsec->list);
>>>
>>> To be able to do this here, you'll have to use list_for_each_entry_safe().
>>>
>>
>> One more thing - note I'm not an xfrm expert by far but it seems to me here you have
>> to also call  xdo_dev_state_free() with the old active slave dev otherwise that will
>> never get called with the original real_dev after the switch to a new
>> active slave (or more accurately it might if the GC runs between the switching
>> but it is a race), care must be taken wrt sequence of events because the XFRM
> 
> Can we just call xs->xso.real_dev->xfrmdev_ops->xdo_dev_state_free(xs)
> no matter xs->xso.real_dev == real_dev or not? I'm afraid calling
> xdo_dev_state_free() every where may make us lot more easily.
> 

You'd have to check all drivers that implement the callback to answer that and even then
I'd stick to the canonical way of how it's done in xfrm and make the bond just passthrough.
Any other games become dangerous and new code will have to be carefully reviewed every
time, calling another device's free_sa when it wasn't added before doesn't sound good.

>> GC may be running in parallel which probably means that in bond_ipsec_free_sa()
>> you'll have to take the mutex before calling xdo_dev_state_free() and check
>> if the entry is still linked in the bond's ipsec list before calling the free_sa
>> callback, if it isn't then del_sa_all got to it before the GC and there's nothing
>> to do if it also called the dev's free_sa callback. The check for real_dev doesn't
>> seem enough to protect against this race.
> 
> I agree that we need to take the mutex before calling xdo_dev_state_free()
> in bond_ipsec_free_sa(). Do you think if this is enough? I'm a bit lot here.
> 
> Thanks
> Hangbin

Well, the race is between the xfrm GC and del_sa_all, in bond's free_sa if you
walk the list under the mutex before calling real_dev's free callback and
don't find the current element that's being freed in free_sa then it was
cleaned up by del_sa_all, otherwise del_sa_all is waiting to walk that
list and clean the entries. I think it should be fine as long as free_sa
was called once with the proper device.