On 2/27/25 10:37, Hangbin Liu wrote:
The fixed commit placed mutex_lock() inside spin_lock_bh(), which triggers a warning:
BUG: sleeping function called from invalid context at...
Fix this by moving the IPsec deletion operation to bond_ipsec_free_sa, which is not held by spin_lock_bh().
Additionally, delete the IPsec list in bond_ipsec_del_sa_all() when the XFRM state is DEAD to prevent xdo_dev_state_free() from being triggered again in bond_ipsec_free_sa().
For bond_ipsec_free_sa(), there are now three conditions:
- if (!slave): When no active device exists.
- if (!xs->xso.real_dev): When xdo_dev_state_add() fails.
- if (xs->xso.real_dev != real_dev): When an xs has already been freed by bond_ipsec_del_sa_all() due to migration, and the active slave has changed to a new device. At the same time, the xs is marked as DEAD due to the XFRM entry is removed, triggering xfrm_state_gc_task() and bond_ipsec_free_sa().
In all three cases, xdo_dev_state_free() should not be called, only xs should be removed from bond->ipsec list.
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 Suggested-by: Cosmin Ratiu cratiu@nvidia.com Signed-off-by: Hangbin Liu liuhangbin@gmail.com
drivers/net/bonding/bond_main.c | 34 ++++++++++++++++++++++----------- 1 file changed, 23 insertions(+), 11 deletions(-)
diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c index e45bba240cbc..683bf1221caf 100644 --- a/drivers/net/bonding/bond_main.c +++ b/drivers/net/bonding/bond_main.c @@ -537,6 +537,10 @@ static void bond_ipsec_add_sa_all(struct bonding *bond) } list_for_each_entry(ipsec, &bond->ipsec_list, list) {
/* Skip dead xfrm states, they'll be freed later. */if (ipsec->xs->km.state == XFRM_STATE_DEAD)continue;- /* If new state is added before ipsec_lock acquired */ if (ipsec->xs->xso.real_dev == real_dev) continue;
@@ -560,7 +564,6 @@ static void bond_ipsec_del_sa(struct xfrm_state *xs) struct net_device *bond_dev = xs->xso.dev; struct net_device *real_dev; netdevice_tracker tracker;
- struct bond_ipsec *ipsec; struct bonding *bond; struct slave *slave;
@@ -592,15 +595,6 @@ 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);
} static void bond_ipsec_del_sa_all(struct bonding *bond) @@ -617,6 +611,12 @@ static void bond_ipsec_del_sa_all(struct bonding *bond) mutex_lock(&bond->ipsec_lock); list_for_each_entry(ipsec, &bond->ipsec_list, list) {
if (ipsec->xs->km.state == XFRM_STATE_DEAD) {list_del(&ipsec->list);
To be able to do this here, you'll have to use list_for_each_entry_safe().
kfree(ipsec);continue;}- if (!ipsec->xs->xso.real_dev) continue;
@@ -640,6 +640,7 @@ static void bond_ipsec_free_sa(struct xfrm_state *xs) struct net_device *bond_dev = xs->xso.dev; struct net_device *real_dev; netdevice_tracker tracker;
- struct bond_ipsec *ipsec; struct bonding *bond; struct slave *slave;
@@ -659,13 +660,24 @@ static void bond_ipsec_free_sa(struct xfrm_state *xs) if (!xs->xso.real_dev) goto out;
- WARN_ON(xs->xso.real_dev != real_dev);
- if (xs->xso.real_dev != real_dev)
goto out;if (real_dev && real_dev->xfrmdev_ops && real_dev->xfrmdev_ops->xdo_dev_state_free) real_dev->xfrmdev_ops->xdo_dev_state_free(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);
} /**