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
