Re: [PATCHv3 net 1/3] bonding: move IPsec deletion to bond_ipsec_free_sa

[Hangbin Liu <liuhangbin@gmail.com>]

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