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

[Nikolay Aleksandrov <razor@blackwall.org>]

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.

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