Re: [PATCH net V2 1/4] net/mlx5e: ipsec: Block eswitch mode changes during policy creation

[Jakub Kicinski <kuba@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email says "Auto-emailing seemed not to have triggered."
---
net/mlx5e: ipsec: Block eswitch mode changes during policy creation

This commit takes a temporary eswitch mode block at the start of
mlx5e_xfrm_add_policy() and releases it once the flow tables hold their
own reference. It also adds a check_users argument to
mlx5_eswitch_block_mode() and mlx5_esw_lock(), and switches
mlx5_eswitch_block_mode() from mlx5_esw_try_lock() to mlx5_esw_lock().

> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ipsec.c b/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ipsec.c
> index db260e3d1412f..841ecdc2c4d9e 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ipsec.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ipsec.c

[ ... ]

> @@ -1266,18 +1266,28 @@ static int mlx5e_xfrm_add_policy(struct xfrm_policy *x,
>  	int err;
>  
>  	priv = netdev_priv(netdev);
> +	/* Block esw mode changes until the policy holds its own block. */
> +	err = mlx5_eswitch_block_mode(priv->mdev, false);

[Severity: High]

Can priv->mdev be NULL at this point? mlx5_eswitch_block_mode() reads
dev->priv.eswitch right away.

mlx5e_priv_cleanup() does memset(priv, 0, sizeof(*priv)), but the netdev
stays registered and keeps its xfrmdev_ops. One case is the
switchdev->legacy profile swap in mlx5e_vport_uplink_rep_unload(). The
state becomes permanent if a profile change and its rollback both fail.

xfrm_dev_policy_add() only checks that xdo_dev_policy_add exists, so this
path looks reachable:

xfrm_dev_policy_add()
  mlx5e_xfrm_add_policy()
    mlx5_eswitch_block_mode(NULL, false)
      esw = dev->priv.eswitch;

Before this patch, the !priv->ipsec check ran first and returned
-EOPNOTSUPP.

The next patch in the series, "net/mlx5e: ipsec: Block eswitch mode
changes before accessing priv->ipsec", adds a !mdev check before this
call. Without that patch, this commit oopses with CONFIG_MLX5_ESWITCH=y.

[Severity: High]

Can this blocking down_write() deadlock against driver unload or devlink
reload? The xfrm core calls this callback with a netdev reference held.
xfrm_dev_policy_add() takes it with dev_get_by_index(), and the call runs
under xfrm_cfg_mutex.

On the unload side, mlx5_eswitch_safe_aux_devs_remove() holds
esw->mode_lock for read across the uplink netdev unregister:

CPU1
_mlx5e_remove()
  mlx5_eswitch_safe_aux_devs_remove()
    mlx5_esw_hold()                  /* mode_lock held for read */
    mlx5_core_reps_aux_devs_remove()
      ...
      mlx5e_vport_uplink_rep_unload()
        unregister_netdev()
          netdev_wait_allrefs_any()  /* waits for CPU0's netdev ref */

CPU0
xfrm_dev_policy_add()
  dev_get_by_index()
  mlx5e_xfrm_add_policy()
    mlx5_eswitch_block_mode()
      mlx5_esw_lock()
        down_write(&esw->mode_lock)  /* waits for CPU1's read hold */

CPU0's policy isn't inserted yet, so the unregister flush never reaches
it. Each task then waits on the other forever. xfrm_cfg_mutex stays held,
so XFRM configuration in the netns would hang as well.

Before this patch, mlx5_eswitch_block_mode() used down_write_trylock()
and failed at once. mlx5e_xfrm_add_state(), rx_get() and tx_get() get the
same trylock-to-blocking change. The mlx5_eswitch_unblock_mode() on the
success path here is another down_write().

At baseline, mlx5e_ipsec_block_tc_offload()->mlx5_esw_lock() already
took the lock in a similar blocking way. The later patch "net/mlx5e:
Serialize TC and IPsec offload exclusion counters" removes that
acquisition, which leaves these block_mode/unblock_mode calls as the ones
that can trigger the hang. The lock is now also taken for policies that
fail validation and for devices without IPsec.

> +	if (err) {
> +		NL_SET_ERR_MSG_MOD(extack, "Eswitch busy, can't add policy");
> +		return err;
> +	}
> +
>  	if (!priv->ipsec) {
>  		NL_SET_ERR_MSG_MOD(extack, "Device doesn't support IPsec packet offload");
> -		return -EOPNOTSUPP;
> +		err = -EOPNOTSUPP;
> +		goto unblock_mode;
>  	}

[ ... ]

> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ipsec_fs.c b/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ipsec_fs.c
> index 8ffa8068e90ac..cd7ff36135244 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ipsec_fs.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ipsec_fs.c
> @@ -1127,7 +1127,7 @@ static int rx_get(struct mlx5_core_dev *mdev, struct mlx5e_ipsec *ipsec,
>  	if (rx->ft.refcnt)
>  		goto skip;
>  
> -	err = mlx5_eswitch_block_mode(mdev);
> +	err = mlx5_eswitch_block_mode(mdev, true);
>  	if (err)
>  		return err;

[Severity: High]

This is a pre-existing issue and was not introduced by this patch. This
patch adds more blocking down_write() sites on mode_lock, though. Can
the release side of this reference self-deadlock during driver unload or
devlink reload in switchdev mode?

mlx5_eswitch_safe_aux_devs_remove() holds esw->mode_lock for read while
it unregisters the uplink netdev. NETDEV_UNREGISTER then flushes the
offloaded policies in the same task:

mlx5_eswitch_safe_aux_devs_remove()
  mlx5_esw_hold()            /* down_read_trylock(&esw->mode_lock) */
  mlx5_core_reps_aux_devs_remove()
    ...
    mlx5e_vport_uplink_rep_unload()
      unregister_netdev()
        xfrm_dev_unregister()
          xfrm_dev_policy_flush()
            xfrm_policy_kill()
              xfrm_dev_policy_delete()
                mlx5e_xfrm_del_policy()
                  mlx5e_accel_ipsec_fs_del_pol()
                    rx_ft_put_policy()
                      rx_put()
                        mlx5_eswitch_unblock_mode()
                          down_write(&esw->mode_lock)

tx_put() has the same pattern. An rwsem read hold can't be upgraded to a
write hold.

Wouldn't the task hang whenever an offloaded IPsec policy exists on the
uplink at unload time? The read hold comes from commit 2ae8c7edea87f5,
and this path is still there at the end of the series.

[ ... ]

> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c b/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c
> index eb74b62601682..95e856f8d834d 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c
> @@ -4343,7 +4343,7 @@ static int esw_inline_mode_to_devlink(u8 mlx5_mode, u8 *mode)
>  	return 0;
>  }
>  
> -int mlx5_eswitch_block_mode(struct mlx5_core_dev *dev)
> +int mlx5_eswitch_block_mode(struct mlx5_core_dev *dev, bool check_users)
>  {
>  	struct mlx5_eswitch *esw = dev->priv.eswitch;
>  	int err;
> @@ -4351,9 +4351,8 @@ int mlx5_eswitch_block_mode(struct mlx5_core_dev *dev)
>  	if (!mlx5_esw_allowed(esw))
>  		return 0;
>  
> -	/* Take TC into account */
> -	err = mlx5_esw_try_lock(esw);
> -	if (err < 0)
> +	err = mlx5_esw_lock(esw, check_users);
> +	if (err)
>  		return err;

[Severity: Medium]

This replaces mlx5_esw_try_lock() with mlx5_esw_lock(). The old function
used down_write_trylock() and returned -EINVAL when mode_lock was held.
The new one does an unconditional down_write().

All existing callers now sleep waiting for mode_lock where they used to
fail right away. That covers mlx5e_xfrm_add_state(), rx_get() and
tx_get(). rx_get() and tx_get() run under rx->ft.mutex and tx->ft.mutex.

The commit message only says:

    To avoid that, I was forced to separate the mode blocking from the TC
    check by introducing a check_users argument to
    mlx5_eswitch_block_mode().

Could the commit message also mention the switch from trylock to a
blocking acquisition? It adds a new lock dependency, as described in the
deadlock question in mlx5e_xfrm_add_policy() above.