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

Commit Message

Hangbin Liu March 7, 2025, 3:19 a.m. UTC

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 <[e-mail]>
Closes: https://lore.kernel.org/netdev/[e-mail]
Suggested-by: Cosmin Ratiu <[e-mail]>
Signed-off-by: Hangbin Liu <[e-mail]>
---
 drivers/net/bonding/bond_main.c | 69 ++++++++++++++++++++++++---------
 1 file changed, 51 insertions(+), 18 deletions(-)

Comments

Nikolay Aleksandrov March 7, 2025, 7:42 a.m. UTC | #1

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 <[e-mail]>
> Closes: https://lore.kernel.org/netdev/[e-mail]
> Suggested-by: Cosmin Ratiu <[e-mail]>
> Signed-off-by: Hangbin Liu <[e-mail]>
> ---
>  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.

> +
>  		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.

Cheers,
 Nik

Hangbin Liu March 7, 2025, 8:11 a.m. UTC | #2

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 <[e-mail]>
> > Closes: https://lore.kernel.org/netdev/[e-mail]
> > Suggested-by: Cosmin Ratiu <[e-mail]>
> > Signed-off-by: Hangbin Liu <[e-mail]>
> > ---
> >  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

Nikolay Aleksandrov March 7, 2025, 8:33 a.m. UTC | #3

On 3/7/25 10:11, Hangbin Liu wrote:
> 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 <[e-mail]>
>>> Closes: https://lore.kernel.org/netdev/[e-mail]
>>> Suggested-by: Cosmin Ratiu <[e-mail]>
>>> Signed-off-by: Hangbin Liu <[e-mail]>
>>> ---
>>>  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.
> 

I don't insist at all, I just pointed out that this is buggy and the value doesn't
make sense used like that. Adding more bugs to the existing code wouldn't make it better.

>>> +
>>>  		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.
> 

I do appreciate the hard work you've been doing on this, don't get me wrong, but this is
not really uapi, it's an optimization. The path will become slower as it won't be offloaded,
but it will still work and will be stable until a proper fix or new design comes in.

Are you suggesting to knowingly leave a race condition that might lead to a number
of problems in place with a comment?
IMO that is not ok, but ultimately it's up to the maintainers to decide if they can live
with it. :)

> By the way, I started this patch because my patch 2/3 is blocked by the
> selftest results from patch 3/3...
> 
> Thanks
> Hangbin

Hangbin Liu March 7, 2025, 10:31 a.m. UTC | #4

On Fri, Mar 07, 2025 at 10:33:57AM +0200, Nikolay Aleksandrov wrote:
> On 3/7/25 10:11, Hangbin Liu wrote:
> > 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 <[e-mail]>
> >>> Closes: https://lore.kernel.org/netdev/[e-mail]
> >>> Suggested-by: Cosmin Ratiu <[e-mail]>
> >>> Signed-off-by: Hangbin Liu <[e-mail]>
> >>> ---
> >>>  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.
> > 
> 
> I don't insist at all, I just pointed out that this is buggy and the value doesn't
> make sense used like that. Adding more bugs to the existing code wouldn't make it better.

I'm a little lost here. Do you mean we should hold spin lock for all xs
readings or just the km.state. The current bonding code didn't hold any
xs lock for xs readings. And I saw in xfrm code it only hold rcu read lock
when check the km.state.

> 
> >>> +
> >>>  		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.
> > 
> 
> I do appreciate the hard work you've been doing on this, don't get me wrong, but this is

I also appreciate your reviews. You helped find a lot of issues :)

> not really uapi, it's an optimization. The path will become slower as it won't be offloaded,
> but it will still work and will be stable until a proper fix or new design comes in.

I'm afraid it doesn't work correctly before this patch. The race condition
here has a long time. What I do is fix part of the races. And leave
bond_ipsec_add_sa() and bond_ipsec_add_sa_all() almost not changed.

The comment in the code is just to notify others there is something wrong.
Instead of just hide it there and only we know this.

Do I miss something?

Thanks
hangbin

> 
> Are you suggesting to knowingly leave a race condition that might lead to a number
> of problems in place with a comment?
> IMO that is not ok, but ultimately it's up to the maintainers to decide if they can live
> with it. :)
> 
> > By the way, I started this patch because my patch 2/3 is blocked by the
> > selftest results from patch 3/3...
> > 
> > Thanks
> > Hangbin
> 
>

Jakub Kicinski March 7, 2025, 5:03 p.m. UTC | #5

On Fri, 7 Mar 2025 09:42:49 +0200 Nikolay Aleksandrov wrote:
> 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.

That's my feeling too, FWIW. I think we knew about this issue
for a while now, the longer we wait the more users we may disrupt
with the revert.

Simon Horman March 8, 2025, 8:54 a.m. UTC | #6

On Fri, Mar 07, 2025 at 03:19:01AM +0000, Hangbin Liu wrote:

...

> @@ -616,9 +615,22 @@ static void bond_ipsec_del_sa_all(struct bonding *bond)
>  		return;
>  
>  	mutex_lock(&bond->ipsec_lock);
> -	list_for_each_entry(ipsec, &bond->ipsec_list, list) {
> -		if (!ipsec->xs->xso.real_dev)
> +	list_for_each_entry_safe(ipsec, tmp_ipsec, &bond->ipsec_list, list) {
> +		spin_lock_bh(&ipsec->xs->lock);
> +		if (!ipsec->xs->xso.real_dev) {
> +			spin_unlock_bh(&ipsec->xs->lock);
>  			continue;
> +		}
> +
> +		if (ipsec->xs->km.state == XFRM_STATE_DEAD) {
> +			list_del(&ipsec->list);
> +			kfree(ipsec);

Hi Hangbin,

Apologies if this was covered elsewhere, but ipsec is kfree'd here...


> +			/* Need to free device here, or the xs->xso.real_dev
> +			 * may changed in bond_ipsec_add_sa_all and free
> +			 * on old device will never be called.
> +			 */
> +			goto next;
> +		}
>  
>  		if (!real_dev->xfrmdev_ops ||
>  		    !real_dev->xfrmdev_ops->xdo_dev_state_delete ||
> @@ -626,11 +638,20 @@ static void bond_ipsec_del_sa_all(struct bonding *bond)
>  			slave_warn(bond_dev, real_dev,
>  				   "%s: no slave xdo_dev_state_delete\n",
>  				   __func__);
> -		} else {
> -			real_dev->xfrmdev_ops->xdo_dev_state_delete(ipsec->xs);
> -			if (real_dev->xfrmdev_ops->xdo_dev_state_free)
> -				real_dev->xfrmdev_ops->xdo_dev_state_free(ipsec->xs);
> +			spin_unlock_bh(&ipsec->xs->lock);
> +			continue;
>  		}
> +
> +		real_dev->xfrmdev_ops->xdo_dev_state_delete(ipsec->xs);
> +
> +next:
> +		/* set real_dev to NULL in case __xfrm_state_delete() is called in parallel */
> +		ipsec->xs->xso.real_dev = NULL;

... and the dereferenced here.

Flagged by Smatch.

> +
> +		/* Unlock before freeing device state, it could sleep. */
> +		spin_unlock_bh(&ipsec->xs->lock);
> +		if (real_dev->xfrmdev_ops->xdo_dev_state_free)
> +			real_dev->xfrmdev_ops->xdo_dev_state_free(ipsec->xs);
>  	}
>  	mutex_unlock(&bond->ipsec_lock);
>  }

...

Hangbin Liu March 10, 2025, 7:22 a.m. UTC | #7

On Sat, Mar 08, 2025 at 08:54:51AM +0000, Simon Horman wrote:
> On Fri, Mar 07, 2025 at 03:19:01AM +0000, Hangbin Liu wrote:
> 
> ...
> 
> > @@ -616,9 +615,22 @@ static void bond_ipsec_del_sa_all(struct bonding *bond)
> >  		return;
> >  
> >  	mutex_lock(&bond->ipsec_lock);
> > -	list_for_each_entry(ipsec, &bond->ipsec_list, list) {
> > -		if (!ipsec->xs->xso.real_dev)
> > +	list_for_each_entry_safe(ipsec, tmp_ipsec, &bond->ipsec_list, list) {
> > +		spin_lock_bh(&ipsec->xs->lock);
> > +		if (!ipsec->xs->xso.real_dev) {
> > +			spin_unlock_bh(&ipsec->xs->lock);
> >  			continue;
> > +		}
> > +
> > +		if (ipsec->xs->km.state == XFRM_STATE_DEAD) {
> > +			list_del(&ipsec->list);
> > +			kfree(ipsec);
> 
> Hi Hangbin,
> 
> Apologies if this was covered elsewhere, but ipsec is kfree'd here...

Oh.. I need to get the xs with xs = ipsec->xs, then hold the xs lock.

Thanks
Hangbin
> 
> 
> > +			/* Need to free device here, or the xs->xso.real_dev
> > +			 * may changed in bond_ipsec_add_sa_all and free
> > +			 * on old device will never be called.
> > +			 */
> > +			goto next;
> > +		}
> >  
> >  		if (!real_dev->xfrmdev_ops ||
> >  		    !real_dev->xfrmdev_ops->xdo_dev_state_delete ||
> > @@ -626,11 +638,20 @@ static void bond_ipsec_del_sa_all(struct bonding *bond)
> >  			slave_warn(bond_dev, real_dev,
> >  				   "%s: no slave xdo_dev_state_delete\n",
> >  				   __func__);
> > -		} else {
> > -			real_dev->xfrmdev_ops->xdo_dev_state_delete(ipsec->xs);
> > -			if (real_dev->xfrmdev_ops->xdo_dev_state_free)
> > -				real_dev->xfrmdev_ops->xdo_dev_state_free(ipsec->xs);
> > +			spin_unlock_bh(&ipsec->xs->lock);
> > +			continue;
> >  		}
> > +
> > +		real_dev->xfrmdev_ops->xdo_dev_state_delete(ipsec->xs);
> > +
> > +next:
> > +		/* set real_dev to NULL in case __xfrm_state_delete() is called in parallel */
> > +		ipsec->xs->xso.real_dev = NULL;
> 
> ... and the dereferenced here.
> 
> Flagged by Smatch.
> 
> > +
> > +		/* Unlock before freeing device state, it could sleep. */
> > +		spin_unlock_bh(&ipsec->xs->lock);
> > +		if (real_dev->xfrmdev_ops->xdo_dev_state_free)
> > +			real_dev->xfrmdev_ops->xdo_dev_state_free(ipsec->xs);
> >  	}
> >  	mutex_unlock(&bond->ipsec_lock);
> >  }
> 
> ...

Hangbin Liu March 10, 2025, 7:53 a.m. UTC | #8

On Fri, Mar 07, 2025 at 09:03:32AM -0800, Jakub Kicinski wrote:
> On Fri, 7 Mar 2025 09:42:49 +0200 Nikolay Aleksandrov wrote:
> > 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.
> 
> That's my feeling too, FWIW. I think we knew about this issue
> for a while now, the longer we wait the more users we may disrupt
> with the revert.

Steffen said we can't sleep in xfrm_timer_handler(), which calls
__xfrm_state_delete(). So I can't find a way to handle the race condition
between bond_ipsec_add_sa_all() -> xdo_dev_state_add, which may sleep.
And __xfrm_state_delete() -> xdo_dev_state_delete, which can't sleep.

Hi Jay, do you have any comments?

Thanks
Hangbin

Cosmin Ratiu March 11, 2025, 9:08 p.m. UTC | #9

On Fri, 2025-03-07 at 09:03 -0800, Jakub Kicinski wrote:
> On Fri, 7 Mar 2025 09:42:49 +0200 Nikolay Aleksandrov wrote:
> > 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.
> 
> That's my feeling too, FWIW. I think we knew about this issue
> for a while now, the longer we wait the more users we may disrupt
> with the revert.

These are preexisting races between the bond link failover and the user
removing the xfrm states. Unless the user wants to intentionally
trigger these bugs, chances are nobody has ever encountered them in the
wild in normal operation. In steady state, bond link failover works,
and adding/removing states works. It's the combination of the two
control plane events that may have a chance to double free or leak
states.

I would not pull everything out just yet.

Today, I managed to find a solution for these races (I think), based on
a patch series I am preparing against ipsec-next with other changes
related to real_dev.

Hangbin, do you mind if I take over fixing the locking issue as part of
my series? I plan to send it upstream the following days.

Cosmin.

Hangbin Liu March 12, 2025, 1:04 a.m. UTC | #10

On Tue, Mar 11, 2025 at 09:08:49PM +0000, Cosmin Ratiu wrote:
> On Fri, 2025-03-07 at 09:03 -0800, Jakub Kicinski wrote:
> > On Fri, 7 Mar 2025 09:42:49 +0200 Nikolay Aleksandrov wrote:
> > > 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.
> > 
> > That's my feeling too, FWIW. I think we knew about this issue
> > for a while now, the longer we wait the more users we may disrupt
> > with the revert.
> 
> These are preexisting races between the bond link failover and the user
> removing the xfrm states. Unless the user wants to intentionally
> trigger these bugs, chances are nobody has ever encountered them in the
> wild in normal operation. In steady state, bond link failover works,
> and adding/removing states works. It's the combination of the two
> control plane events that may have a chance to double free or leak
> states.
> 
> I would not pull everything out just yet.
> 
> Today, I managed to find a solution for these races (I think), based on
> a patch series I am preparing against ipsec-next with other changes
> related to real_dev.
> 
> Hangbin, do you mind if I take over fixing the locking issue as part of
> my series? I plan to send it upstream the following days.

No, I don't mind. Please go ahead to fixing the locking issue. And thanks
a lot for your reviewing.

Regards
Hangbin

Patch

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;
+
 		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;
@@ -560,7 +569,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,22 +600,13 @@  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)
 {
 	struct net_device *bond_dev = bond->dev;
+	struct bond_ipsec *ipsec, *tmp_ipsec;
 	struct net_device *real_dev;
-	struct bond_ipsec *ipsec;
 	struct slave *slave;
 
 	slave = rtnl_dereference(bond->curr_active_slave);
@@ -616,9 +615,22 @@  static void bond_ipsec_del_sa_all(struct bonding *bond)
 		return;
 
 	mutex_lock(&bond->ipsec_lock);
-	list_for_each_entry(ipsec, &bond->ipsec_list, list) {
-		if (!ipsec->xs->xso.real_dev)
+	list_for_each_entry_safe(ipsec, tmp_ipsec, &bond->ipsec_list, list) {
+		spin_lock_bh(&ipsec->xs->lock);
+		if (!ipsec->xs->xso.real_dev) {
+			spin_unlock_bh(&ipsec->xs->lock);
 			continue;
+		}
+
+		if (ipsec->xs->km.state == XFRM_STATE_DEAD) {
+			list_del(&ipsec->list);
+			kfree(ipsec);
+			/* Need to free device here, or the xs->xso.real_dev
+			 * may changed in bond_ipsec_add_sa_all and free
+			 * on old device will never be called.
+			 */
+			goto next;
+		}
 
 		if (!real_dev->xfrmdev_ops ||
 		    !real_dev->xfrmdev_ops->xdo_dev_state_delete ||
@@ -626,11 +638,20 @@  static void bond_ipsec_del_sa_all(struct bonding *bond)
 			slave_warn(bond_dev, real_dev,
 				   "%s: no slave xdo_dev_state_delete\n",
 				   __func__);
-		} else {
-			real_dev->xfrmdev_ops->xdo_dev_state_delete(ipsec->xs);
-			if (real_dev->xfrmdev_ops->xdo_dev_state_free)
-				real_dev->xfrmdev_ops->xdo_dev_state_free(ipsec->xs);
+			spin_unlock_bh(&ipsec->xs->lock);
+			continue;
 		}
+
+		real_dev->xfrmdev_ops->xdo_dev_state_delete(ipsec->xs);
+
+next:
+		/* set real_dev to NULL in case __xfrm_state_delete() is called in parallel */
+		ipsec->xs->xso.real_dev = NULL;
+
+		/* Unlock before freeing device state, it could sleep. */
+		spin_unlock_bh(&ipsec->xs->lock);
+		if (real_dev->xfrmdev_ops->xdo_dev_state_free)
+			real_dev->xfrmdev_ops->xdo_dev_state_free(ipsec->xs);
 	}
 	mutex_unlock(&bond->ipsec_lock);
 }
@@ -638,6 +659,7 @@  static void bond_ipsec_del_sa_all(struct bonding *bond)
 static void bond_ipsec_free_sa(struct xfrm_state *xs)
 {
 	struct net_device *bond_dev = xs->xso.dev;
+	struct bond_ipsec *ipsec, *tmp_ipsec;
 	struct net_device *real_dev;
 	netdevice_tracker tracker;
 	struct bonding *bond;
@@ -659,13 +681,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_safe(ipsec, tmp_ipsec, &bond->ipsec_list, list) {
+		if (ipsec->xs == xs) {
+			list_del(&ipsec->list);
+			kfree(ipsec);
+			break;
+		}
+	}
+	mutex_unlock(&bond->ipsec_lock);
 }
 
 /**