Re: [PATCHv5 net 1/3] bonding: fix calling sleeping function in spin lock and some race conditions

[Hangbin Liu <liuhangbin@gmail.com>]

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;
> >  		kfree(ipsec);
> >  	}
> >  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()
> > +		 */
> >  		if (real_dev->xfrmdev_ops->xdo_dev_state_add(ipsec->xs, NULL)) {
> >  			slave_warn(bond_dev, real_dev, "%s: failed to add SA\n", __func__);
> >  			ipsec->xs->xso.real_dev = NULL;
> [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