Re: [PATCHv2 net 1/3] bonding: move mutex lock to a work queue for XFRM GC tasks

[Cosmin Ratiu <cratiu@nvidia.com>]

On Tue, 2025-02-25 at 09:40 +0000, Hangbin Liu wrote:
> The fixed commit placed mutex_lock() inside spin_lock_bh(), which
> triggers
> a warning like:
> 
> BUG: sleeping function called from invalid context at...
> 
> Fix this by moving the mutex_lock() operation to a work queue.
> 
> Fixes: 2aeeef906d5a ("bonding: change ipsec_lock from spin lock to
> mutex")
> Reported-by: Jakub Kicinski <kuba@kernel.org>
> Closes:
> https://lore.kernel.org/netdev/20241212062734.182a0164@kernel.org
> Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
> ---
>  drivers/net/bonding/bond_main.c | 41 +++++++++++++++++++++++++------
> --
>  include/net/bonding.h           |  6 +++++
>  2 files changed, 37 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/net/bonding/bond_main.c
> b/drivers/net/bonding/bond_main.c
> index e45bba240cbc..cc7064aa4b35 100644
> --- a/drivers/net/bonding/bond_main.c
> +++ b/drivers/net/bonding/bond_main.c
> @@ -551,6 +551,25 @@ static void bond_ipsec_add_sa_all(struct bonding
> *bond)
>  	mutex_unlock(&bond->ipsec_lock);
>  }
>  
> +static void bond_xfrm_state_gc_work(struct work_struct *work)
> +{
> +	struct bond_xfrm_work *xfrm_work = container_of(work, struct
> bond_xfrm_work, work);
> +	struct bonding *bond = xfrm_work->bond;
> +	struct xfrm_state *xs = xfrm_work->xs;
> +	struct bond_ipsec *ipsec;
> +
> +	mutex_lock(&bond->ipsec_lock);
> +	list_for_each_entry(ipsec, &bond->ipsec_list, list) {
> +		if (ipsec->xs == xs) {
> +			list_del(&ipsec->list);
> +			kfree(ipsec);
> +			xfrm_state_put(xs);

I would expect xfrm_state_put to be called from outside the loop,
regardless of whether an entry is found in the list or not, because it
was unconditionally referenced when the work was created.

> +			break;
> +		}
> +	}
> +	mutex_unlock(&bond->ipsec_lock);
> +}
> +
>  /**
>   * bond_ipsec_del_sa - clear out this specific SA
>   * @xs: pointer to transformer state struct
> @@ -558,9 +577,9 @@ static void bond_ipsec_add_sa_all(struct bonding
> *bond)
>  static void bond_ipsec_del_sa(struct xfrm_state *xs)
>  {
>  	struct net_device *bond_dev = xs->xso.dev;
> +	struct bond_xfrm_work *xfrm_work;
>  	struct net_device *real_dev;
>  	netdevice_tracker tracker;
> -	struct bond_ipsec *ipsec;
>  	struct bonding *bond;
>  	struct slave *slave;
>  
> @@ -592,15 +611,17 @@ static void bond_ipsec_del_sa(struct xfrm_state
> *xs)
>  	real_dev->xfrmdev_ops->xdo_dev_state_delete(xs);
>  out:
>  	netdev_put(real_dev, &tracker);
> -	mutex_lock(&bond->ipsec_lock);
> -	list_for_each_entry(ipsec, &bond->ipsec_list, list) {
> -		if (ipsec->xs == xs) {
> -			list_del(&ipsec->list);
> -			kfree(ipsec);
> -			break;
> -		}
> -	}
> -	mutex_unlock(&bond->ipsec_lock);
> +
> +	xfrm_work = kmalloc(sizeof(*xfrm_work), GFP_ATOMIC);
> +	if (!xfrm_work)
> +		return;
> +
> +	INIT_WORK(&xfrm_work->work, bond_xfrm_state_gc_work);
> +	xfrm_work->bond = bond;
> +	xfrm_work->xs = xs;
> +	xfrm_state_hold(xs);
> +
> +	queue_work(bond->wq, &xfrm_work->work);
>  }
>  
>  static void bond_ipsec_del_sa_all(struct bonding *bond)
> diff --git a/include/net/bonding.h b/include/net/bonding.h
> index 8bb5f016969f..d54ba5e3affb 100644
> --- a/include/net/bonding.h
> +++ b/include/net/bonding.h
> @@ -209,6 +209,12 @@ struct bond_ipsec {
>  	struct xfrm_state *xs;
>  };
>  
> +struct bond_xfrm_work {
> +	struct work_struct work;
> +	struct bonding *bond;
> +	struct xfrm_state *xs;
> +};

Also, like Nikolai said, something needs to wait on all in-flight work
items.

This got me to stare at the code again. What if we move the removal of
the xs from bond->ipsec from bond_ipsec_del_sa to bond_ipsec_free_sa?
bond_ipsec_free_sa, unlike bond_ipsec_del_sa, is not called with x-
>lock held. It is called from the xfrm gc task or directly via
xfrm_state_put_sync and therefore wouldn't suffer from the locking
issue.

The tricky part is to make sure that inactive bond->ipsec entries
(after bond_ipsec_del_sa calls) do not cause issues if there's a
migration (bond_ipsec_del_sa_all is called) happening before
bond_ipsec_free_sa. Perhaps filtering by x->km.state != XFRM_STATE_DEAD
in bond_ipsec_del_sa_all.

What do you think about this idea?

Cosmin.