Re: [PATCH net V2 2/4] net/mlx5e: ipsec: Block eswitch mode changes before accessing priv->ipsec

[Jakub Kicinski <kuba@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
net/mlx5e: ipsec: Block eswitch mode changes before accessing priv->ipsec

This patch moves the eswitch mode block in mlx5e_xfrm_add_state() so it is
taken before priv->ipsec is read and before the state is validated. A
concurrent profile change can then no longer free the IPsec context while
it is in use. The acquire placeholder path is still not blocked, and
mlx5e_xfrm_add_policy() gains a local mdev with a NULL check.

> mlx5e_xfrm_add_state() reads priv->ipsec and validates mode-dependent
> capabilities before blocking eswitch mode changes. A concurrent profile
> change can free the saved IPsec context and cause use-after-free.

[ ... ]

> Fixes: 22239eb258bc ("net/mlx5e: Prevent tunnel reformat when tunnel mode not allowed")

[Severity: Medium]
Is this the right Fixes: tag for the race being closed?

Before 22239eb258bc, mlx5e_xfrm_add_state() had no eswitch mode block at
all. It already read priv->ipsec, stored it in sa_entry, and used it for
validation, SA hardware context creation and
mlx5e_accel_ipsec_fs_add_rule(). 22239eb258bc added a block after that
read. It did not move an existing block later.

Before 22239eb258bc, the only mode block on this path was the late one in
rx_get()/tx_get() in ipsec_fs.c:

    err = mlx5_eswitch_block_mode(mdev, true);

The previous patch in this series ("net/mlx5e: ipsec: Block eswitch mode
changes during policy creation") says that late block comes too late,
because the table lookup already touches the IPsec context. That patch
tags a5b8ca9471d3 and says "mlx5e_xfrm_add_state() has the same race".

With the current tag, could a stable tree that has the packet offload
state path but not 22239eb258bc take the add_policy half of the fix and
never get this add_state half? Should this patch carry the same tag as
the policy patch?

> 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 841ecdc2c4d9e..cf721ef83d597 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ipsec.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ipsec.c
> @@ -771,28 +771,44 @@ static int mlx5e_xfrm_add_state(struct net_device *dev,
>  				struct xfrm_state *x,
>  				struct netlink_ext_ack *extack)
>  {

[ ... ]

>  	priv = netdev_priv(dev);
> -	if (!priv->ipsec)
> +	mdev = priv->mdev;
> +	if (!mdev || !priv->ipsec)
>  		return -EOPNOTSUPP;
>  
> +	if (!is_acq) {
> +		err = mlx5_eswitch_block_mode(mdev, false);
> +		if (err)
> +			return err;
> +	}

[Severity: Low]
Should these loads of priv->mdev and priv->ipsec use READ_ONCE()?

They happen without a lock, before the mode block is held, so they can
race with a concurrent profile change:

mlx5e_netdev_change_profile()
  mlx5e_priv_cleanup()
    memset(priv, 0, sizeof(*priv));

The !mdev check only covers that window if priv->mdev is loaded exactly
once. With a plain load, the compiler may reload priv->mdev for the
mlx5_eswitch_block_mode() argument. The reloaded value could be NULL, and
mlx5_eswitch_block_mode() would then dereference dev->priv.eswitch. KCSAN
would also report these as data races.

The same question applies to the new mdev = priv->mdev read in
mlx5e_xfrm_add_policy().

[ ... ]

> @@ -1262,12 +1275,17 @@ static int mlx5e_xfrm_add_policy(struct xfrm_policy *x,
>  {
>  	struct net_device *netdev = x->xdo.dev;
>  	struct mlx5e_ipsec_pol_entry *pol_entry;
> +	struct mlx5_core_dev *mdev;
>  	struct mlx5e_priv *priv;
>  	int err;
>  
>  	priv = netdev_priv(netdev);
> +	mdev = priv->mdev;
> +	if (!mdev)
> +		return -EOPNOTSUPP;
> +
>  	/* Block esw mode changes until the policy holds its own block. */
> -	err = mlx5_eswitch_block_mode(priv->mdev, false);
> +	err = mlx5_eswitch_block_mode(mdev, false);

[Severity: Medium]
The commit message only covers add_state and does not mention this new
NULL check in mlx5e_xfrm_add_policy(). Should the check go into the
previous patch instead?

The previous patch ("net/mlx5e: ipsec: Block eswitch mode changes during
policy creation") moved mlx5_eswitch_block_mode(priv->mdev, false) ahead
of the !priv->ipsec check. mlx5_eswitch_block_mode() dereferences
dev->priv.eswitch without checking dev for NULL.

priv->mdev can be NULL while the netdev is still registered. If a profile
change fails and its rollback also fails, mlx5e_priv_cleanup() zeroes
priv:

mlx5e_priv_cleanup() {
    ...
	/* bail if change profile failed and also rollback failed */
	if (!priv->mdev)
		return;
    ...
	memset(priv, 0, sizeof(*priv));
}

netdev->xfrmdev_ops is not cleared, so a CAP_NET_ADMIN user who adds an
offloaded policy still reaches:

xfrm_dev_policy_add()
  mlx5e_xfrm_add_policy()
    mlx5_eswitch_block_mode(NULL, false)

At baseline, the !priv->ipsec check returned -EOPNOTSUPP in that state.
With only the previous patch applied, this path oopses.

The previous patch carries Fixes: a5b8ca9471d3 and this one carries
Fixes: 22239eb258bc. Could the previous patch be backported without this
guard? As posted, the series is also not bisect-safe at the previous
patch.

[ ... ]