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

Cosmin Ratiu
2025-03-04T10:25:56Z

On Tue, 2025-03-04 at 09:18 +0000, Hangbin Liu wrote:
>
> Just to make sure I added the lock in correct place, would you please
> help
> confirm.
>
> diff --git a/drivers/net/bonding/bond_main.c
> b/drivers/net/bonding/bond_main.c
> index e85878b12376..c59ad3a5cf43 100644
> --- a/drivers/net/bonding/bond_main.c
> +++ b/drivers/net/bonding/bond_main.c
> @@ -537,19 +537,25 @@ 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)
> + if (ipsec->xs->km.state == XFRM_STATE_DEAD) {
> + spin_unlock_bh(&ipsec->xs->lock);

Instead of unlocking on every branch, I recommend adding a "next:" tag
before the unlock at the end of the loop and switching the "continue"
statements with "goto next".

>   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;
>   }

Add the "next:" tag here.

> + spin_unlock_bh(&ipsec->xs->lock);
>   }
>  out:
>   mutex_unlock(&bond->ipsec_lock);
> @@ -614,6 +620,7 @@ static void bond_ipsec_del_sa_all(struct bonding
> *bond)
>   if (!ipsec->xs->xso.real_dev)
>   continue;

The above if should be in the critical section as well.

>
> + spin_lock_bh(&ipsec->xs->lock);
>   if (ipsec->xs->km.state == XFRM_STATE_DEAD) {
>   /* already dead no need to delete again */
>   if (ipsec->xs->xso.real_dev == real_dev &&
> @@ -621,6 +628,7 @@ static void bond_ipsec_del_sa_all(struct bonding
> *bond)
>   real_dev->xfrmdev_ops-
> >xdo_dev_state_free(ipsec->xs);
>   list_del(&ipsec->list);
>   kfree(ipsec);
> + spin_unlock_bh(&ipsec->xs->lock);

And I recommend the same thing with "goto next" here, jumping at the
end of the loop, before the unlock.

>   continue;
>   }
>
> @@ -635,6 +643,7 @@ static void bond_ipsec_del_sa_all(struct bonding
> *bond)
>   if (real_dev->xfrmdev_ops-
> >xdo_dev_state_free)
>   real_dev->xfrmdev_ops-
> >xdo_dev_state_free(ipsec->xs);
>   }
> + spin_unlock_bh(&ipsec->xs->lock);
>   }
>   mutex_unlock(&bond->ipsec_lock);
>  }
>
> Thanks
> Hangbin