Hi Nikolay, On Fri, Mar 07, 2025 at 09:42:49AM +0200, Nikolay Aleksandrov wrote:
On 3/7/25 05:19, 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, there are also some race conditions as bond_ipsec_del_sa_all() and __xfrm_state_delete could running in parallel without any lock. e.g.
bond_ipsec_del_sa_all() __xfrm_state_delete() - .xdo_dev_state_delete - bond_ipsec_del_sa() - .xdo_dev_state_free - .xdo_dev_state_delete() - bond_ipsec_free_sa() bond active_slave changes - .xdo_dev_state_free()
bond_ipsec_add_sa_all() - ipsec->xs->xso.real_dev = real_dev; - xdo_dev_state_add
To fix this, let's add xs->lock during bond_ipsec_del_sa_all(), and delete the IPsec list when the XFRM state is DEAD, which could prevent xdo_dev_state_free() from being triggered again in bond_ipsec_free_sa().
In bond_ipsec_add_sa(), if .xdo_dev_state_add() failed, the xso.real_dev is set without clean. Which will cause trouble if __xfrm_state_delete is called at the same time. Reset the xso.real_dev to NULL if state add failed.
Despite the above fixes, there are still races in bond_ipsec_add_sa() and bond_ipsec_add_sa_all(). If __xfrm_state_delete() is called immediately after we set the xso.real_dev and before .xdo_dev_state_add() is finished, like
ipsec->xs->xso.real_dev = real_dev; __xfrm_state_delete - bond_ipsec_del_sa() - .xdo_dev_state_delete() - bond_ipsec_free_sa() - .xdo_dev_state_free() .xdo_dev_state_add()
But there is no good solution yet. So I just added a FIXME note in here and hope we can fix it in future.
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 | 69 ++++++++++++++++++++++++--------- 1 file changed, 51 insertions(+), 18 deletions(-)
diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c index e45bba240cbc..dd3d0d41d98f 100644 --- a/drivers/net/bonding/bond_main.c +++ b/drivers/net/bonding/bond_main.c @@ -506,6 +506,7 @@ static int bond_ipsec_add_sa(struct xfrm_state *xs, list_add(&ipsec->list, &bond->ipsec_list); mutex_unlock(&bond->ipsec_lock); } else {
xs->xso.real_dev = NULL;out: @@ -541,7 +542,15 @@ static void bond_ipsec_add_sa_all(struct bonding *bond) if (ipsec->xs->xso.real_dev == real_dev) continue;
/* Skip dead xfrm states, they'll be freed later. */if (ipsec->xs->km.state == XFRM_STATE_DEAD)continue;As we commented earlier, reading this state without x->lock is wrong.
But even we add the lock, like
spin_lock_bh(&ipsec->xs->lock); if (ipsec->xs->km.state == XFRM_STATE_DEAD) { spin_unlock_bh(&ipsec->xs->lock); continue; }
We still may got the race condition. Like the following note said. So I just leave it as the current status. But I can add the spin lock if you insist.
- ipsec->xs->xso.real_dev = real_dev;
/* FIXME: there is a race that before .xdo_dev_state_add()* is called, the __xfrm_state_delete() is called in parallel,* which will call .xdo_dev_state_delete() and xdo_dev_state_free()*/[snip]
TBH, keeping buggy code with a comment doesn't sound good to me. I'd rather remove this support than tell people "good luck, it might crash". It's better to be safe until a correct design is in place which takes care of these issues.
I agree it's not a good experience to let users using an unstable feature. But this is a race condition, although we don't have a good fix yet.
On the other hand, I think we can't remove a feature people is using, can we? What I can do is try fix the issues as my best.
By the way, I started this patch because my patch 2/3 is blocked by the selftest results from patch 3/3...
Thanks Hangbin