Re: [PATCH net V3 3/3] bonding: change ipsec_lock from spin lock to mutex

[Hangbin Liu <liuhangbin@gmail.com>]

On Mon, Aug 05, 2024 at 08:03:57AM +0300, Tariq Toukan wrote:
> From: Jianbo Liu <jianbol@nvidia.com>
> 
> In the cited commit, bond->ipsec_lock is added to protect ipsec_list,
> hence xdo_dev_state_add and xdo_dev_state_delete are called inside
> this lock. As ipsec_lock is a spin lock and such xfrmdev ops may sleep,
> "scheduling while atomic" will be triggered when changing bond's
> active slave.
> 
> [  101.055189] BUG: scheduling while atomic: bash/902/0x00000200
> [  101.055726] Modules linked in:
> [  101.058211] CPU: 3 PID: 902 Comm: bash Not tainted 6.9.0-rc4+ #1
> [  101.058760] Hardware name:
> [  101.059434] Call Trace:
> [  101.059436]  <TASK>
> [  101.060873]  dump_stack_lvl+0x51/0x60
> [  101.061275]  __schedule_bug+0x4e/0x60
> [  101.061682]  __schedule+0x612/0x7c0
> [  101.062078]  ? __mod_timer+0x25c/0x370
> [  101.062486]  schedule+0x25/0xd0
> [  101.062845]  schedule_timeout+0x77/0xf0
> [  101.063265]  ? asm_common_interrupt+0x22/0x40
> [  101.063724]  ? __bpf_trace_itimer_state+0x10/0x10
> [  101.064215]  __wait_for_common+0x87/0x190
> [  101.064648]  ? usleep_range_state+0x90/0x90
> [  101.065091]  cmd_exec+0x437/0xb20 [mlx5_core]
> [  101.065569]  mlx5_cmd_do+0x1e/0x40 [mlx5_core]
> [  101.066051]  mlx5_cmd_exec+0x18/0x30 [mlx5_core]
> [  101.066552]  mlx5_crypto_create_dek_key+0xea/0x120 [mlx5_core]
> [  101.067163]  ? bonding_sysfs_store_option+0x4d/0x80 [bonding]
> [  101.067738]  ? kmalloc_trace+0x4d/0x350
> [  101.068156]  mlx5_ipsec_create_sa_ctx+0x33/0x100 [mlx5_core]
> [  101.068747]  mlx5e_xfrm_add_state+0x47b/0xaa0 [mlx5_core]
> [  101.069312]  bond_change_active_slave+0x392/0x900 [bonding]
> [  101.069868]  bond_option_active_slave_set+0x1c2/0x240 [bonding]
> [  101.070454]  __bond_opt_set+0xa6/0x430 [bonding]
> [  101.070935]  __bond_opt_set_notify+0x2f/0x90 [bonding]
> [  101.071453]  bond_opt_tryset_rtnl+0x72/0xb0 [bonding]
> [  101.071965]  bonding_sysfs_store_option+0x4d/0x80 [bonding]
> [  101.072567]  kernfs_fop_write_iter+0x10c/0x1a0
> [  101.073033]  vfs_write+0x2d8/0x400
> [  101.073416]  ? alloc_fd+0x48/0x180
> [  101.073798]  ksys_write+0x5f/0xe0
> [  101.074175]  do_syscall_64+0x52/0x110
> [  101.074576]  entry_SYSCALL_64_after_hwframe+0x4b/0x53
> 
> As bond_ipsec_add_sa_all and bond_ipsec_del_sa_all are only called
> from bond_change_active_slave, which requires holding the RTNL lock.
> And bond_ipsec_add_sa and bond_ipsec_del_sa are xfrm state
> xdo_dev_state_add and xdo_dev_state_delete APIs, which are in user
> context. So ipsec_lock doesn't have to be spin lock, change it to
> mutex, and thus the above issue can be resolved.
> 
> Fixes: 9a5605505d9c ("bonding: Add struct bond_ipesc to manage SA")
> Signed-off-by: Jianbo Liu <jianbol@nvidia.com>
> Signed-off-by: Tariq Toukan <tariqt@nvidia.com>
> ---
>  drivers/net/bonding/bond_main.c | 75 +++++++++++++++++----------------
>  include/net/bonding.h           |  2 +-
>  2 files changed, 40 insertions(+), 37 deletions(-)
> 
> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
> index e550b1c08fdb..56764f1c39b8 100644
> --- a/drivers/net/bonding/bond_main.c
> +++ b/drivers/net/bonding/bond_main.c
> @@ -481,35 +476,43 @@ static void bond_ipsec_add_sa_all(struct bonding *bond)
>  	struct bond_ipsec *ipsec;
>  	struct slave *slave;
>  
> -	rcu_read_lock();
> -	slave = rcu_dereference(bond->curr_active_slave);
> -	if (!slave)
> -		goto out;
> +	slave = rtnl_dereference(bond->curr_active_slave);
> +	real_dev = slave ? slave->dev : NULL;
> +	if (!real_dev)
> +		return;
>  
> -	real_dev = slave->dev;
> +	mutex_lock(&bond->ipsec_lock);
>  	if (!real_dev->xfrmdev_ops ||
>  	    !real_dev->xfrmdev_ops->xdo_dev_state_add ||
>  	    netif_is_bond_master(real_dev)) {
> -		spin_lock_bh(&bond->ipsec_lock);
>  		if (!list_empty(&bond->ipsec_list))
>  			slave_warn(bond_dev, real_dev,
>  				   "%s: no slave xdo_dev_state_add\n",
>  				   __func__);
> -		spin_unlock_bh(&bond->ipsec_lock);
>  		goto out;
>  	}
>  
> -	spin_lock_bh(&bond->ipsec_lock);
>  	list_for_each_entry(ipsec, &bond->ipsec_list, list) {
> +		struct net_device *dev = ipsec->xs->xso.real_dev;
> +
> +		/* If new state is added before ipsec_lock acquired */
> +		if (dev) {
> +			if (dev == real_dev)
> +				continue;
Hi Jianbo,

Why we skip the deleting here if dev == real_dev? What if the state
is added again on the same slave? From the previous logic it looks we
don't check and do over write for the same device.

Thanks
Hangbin
> +			dev->xfrmdev_ops->xdo_dev_state_delete(ipsec->xs);
> +			if (dev->xfrmdev_ops->xdo_dev_state_free)
> +				dev->xfrmdev_ops->xdo_dev_state_free(ipsec->xs);
> +		}
> +
>  		ipsec->xs->xso.real_dev = real_dev;
>  		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;
>  		}
>  	}
> -	spin_unlock_bh(&bond->ipsec_lock);
>  out:
> -	rcu_read_unlock();
> +	mutex_unlock(&bond->ipsec_lock);
>  }