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
