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 <k...@kernel.org>
> Closes:
> https://lore.kernel.org/netdev/20241212062734.182a0...@kernel.org
> Signed-off-by: Hangbin Liu <liuhang...@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.
