On 2/25/25 16:00, Cosmin Ratiu wrote:
> 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.
I know the question was for Hangbin, but I do like this solution. I missed
the xdo_dev_state_free callback, it could lead to a much simpler solution
with some care.
Cheers,
Nik
