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

Hangbin Liu
2025-03-06T13:29:26Z

On Thu, Mar 06, 2025 at 10:02:34AM +0000, Hangbin Liu wrote:
> > Set xs->xso.real_dev = NULL is a good idea. As we will break
> > in bond_ipsec_del_sa()/bond_ipsec_free_sa() when there is no
> > xs->xso.real_dev.
> >
> > For bond_ipsec_add_sa_all(), I will move the xso.real_dev = real_dev
> > after .xdo_dev_state_add() in case the following situation.
> >
> Hmm, do we still need to the spin_lock in bond_ipsec_add_sa_all()? With
> xs->xso.real_dev = NULL after bond_ipsec_del_sa_all(), it looks there is
> no need the spin_lock in bond_ipsec_add_sa_all(). e.g.
>
>
> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
> index 04b677d0c45b..3ada51c63207 100644
> --- a/drivers/net/bonding/bond_main.c
> +++ b/drivers/net/bonding/bond_main.c
> @@ -537,15 +537,27 @@ static void bond_ipsec_add_sa_all(struct bonding *bond)
> }
>
> list_for_each_entry(ipsec, &bond->ipsec_list, list) {
> + spin_lock_bh(&ipsec->xs->lock);
> + /* Skip dead xfrm states, they'll be freed later. */
> + if (ipsec->xs->km.state == XFRM_STATE_DEAD) {
> + spin_unlock_bh(&ipsec->xs->lock);
> + continue;
> + }
> +
> /* If new state is added before ipsec_lock acquired */
> - if (ipsec->xs->xso.real_dev == real_dev)
> + if (ipsec->xs->xso.real_dev == real_dev) {
> + spin_unlock_bh(&ipsec->xs->lock);
> continue;
> + }
>
> - ipsec->xs->xso.real_dev = real_dev;
> if (real_dev->xfrmdev_ops->xdo_dev_state_add(ipsec->xs, NULL)) {
> slave_warn(bond_dev, real_dev, "%s: failed to add SA\n", __func__);
> ipsec->xs->xso.real_dev = NULL;
> }
> + /* Set real_dev after .xdo_dev_state_add in case
> + * __xfrm_state_delete() is called in parallel
> + */
> + ipsec->xs->xso.real_dev = real_dev;
> }

OK, please ignore this, the .xdo_dev_state_add() need xso.real_dev to
be set first. Then I'm still wonder how to avoid the race before
.xdo_dev_state_add() is called, e.g.

bond_ipsec_add_sa_all()
spin_lock_bh(&ipsec->xs->lock);
ipsec->xs->xso.real_dev = real_dev;
spin_unlock(&ipsec->x->lock);
__xfrm_state_delete
- bond_ipsec_del_sa()
- .xdo_dev_state_delete()
- bond_ipsec_free_sa()
- .xdo_dev_state_free()
.xdo_dev_state_add()

Thanks
Hangbin