[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <Z8Bm9i9St0zzDhRZ@fedora>
Date: Thu, 27 Feb 2025 13:21:58 +0000
From: Hangbin Liu <liuhangbin@...il.com>
To: Nikolay Aleksandrov <razor@...ckwall.org>
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 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.
> 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
Powered by blists - more mailing lists