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

[Cosmin Ratiu]

On Mon, 2026-09-21 at 18:49 +0000, netdev-bot+sashiko@xxxxxxxxxx wrote:
> Thank you for your contribution! Sashiko AI review found 3 potential
> issue(s) to consider.
> 
> Critical: 0 · High: 1 · Medium: 1 (2) · Low: 1
> 
> If you believe none of the issues are legitimate, please reply to
> each
> issue of Medium severity or higher and explain why the AI is wrong.
> If any of them are legitimate and you plan to respin, please let
> patchwork
> know by including "pw-bot: cr" as a separate line at the end of your
> reply
> (one such reply per series is enough).
> 
> - [High] By moving mlx5_eswitch_block_mode(priv->mdev, true) to the
> top
>   of mlx5e_xfrm_add_state(), the first profile-owned field touched
> is…
> - [Medium] mlx5e_xfrm_add_state() now calls
>   mlx5_eswitch_block_mode(priv->mdev, true) *before* checking
> whether…
> - [Low] mlx5e_xfrm_add_state() now emits the generic extack message
>   "mlx5_core: Device failed to offload this state" on paths that
> report…
> 
> Pre-existing issues:
> - [Medium] The two sibling xfrmdev_ops add callbacks take the same
>   temporary eswitch-mode block for the same stated purpose (protect…
> - [Medium] The CONFIG_MLX5_ESWITCH=n stub of
> mlx5_eswitch_block_ipsec()
>   (eswitch.h:1090-1093) returns false, i.e. "blocked/busy", whereas
> the…
> 
> --
> 
> net/mlx5e: ipsec: Block eswitch mode changes before accessing priv-
> >ipsec
> 
> This moves the mlx5_eswitch_block_mode() call in
> mlx5e_xfrm_add_state()
> ahead of the first priv->ipsec access so a concurrent profile change
> cannot free the context under it, and releases the block on all error
> paths.  The acquire-placeholder path is kept exempt from the block.
> 
> A few questions about the reordering below.
> 
> > 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..1488faece80eb 100644
> > --- a/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ipsec.c
> > +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ipsec.c
> > @@ -771,6 +771,7 @@ static int mlx5e_xfrm_add_state(struct
> > net_device *dev,
> >  				struct xfrm_state *x,
> >  				struct netlink_ext_ack *extack)
> >  {
> > +	bool is_acq = x->xso.flags & XFRM_DEV_OFFLOAD_FLAG_ACQ;
> >  	struct mlx5e_ipsec_sa_entry *sa_entry = NULL;
> >  	bool allow_tunnel_mode = false;
> >  	struct mlx5e_ipsec *ipsec;
> > @@ -779,20 +780,30 @@ static int mlx5e_xfrm_add_state(struct
> > net_device *dev,
> >  	int err;
> >  
> >  	priv = netdev_priv(dev);
> > -	if (!priv->ipsec)
> > -		return -EOPNOTSUPP;
> > +	if (!is_acq) {
> > +		err = mlx5_eswitch_block_mode(priv->mdev, true);
> > +		if (err)
> > +			return err;
> > +	}
> 
> [Severity: High]
> Can this dereference a NULL priv->mdev?  After this change the first
> profile-owned field touched is priv->mdev rather than priv->ipsec,
> and
> mlx5e_priv_cleanup() ends with:
> 
> drivers/net/ethernet/mellanox/mlx5/core/en_main.c:mlx5e_priv_cleanup(
> ) {
> 	...
> 	memset(priv, 0, sizeof(*priv));
> }
> 
> so priv->mdev can be observed as NULL while the netdev is still
> registered.  Besides the transient window between
> mlx5e_priv_cleanup()
> and mlx5e_netdev_init_profile() during a profile change, it also
> stays
> NULL indefinitely when both the new attach and the rollback fail:
> 
> drivers/net/ethernet/mellanox/mlx5/core/en_main.c:mlx5e_netdev_change
> _profile() {
> 	...
> rollback:
> 	if (!orig_profile) {
> 		netdev_warn(netdev, "no original profile to rollback
> to\n");
> 		priv->profile = NULL;
> 		return err;
> 	}
> 
> 	rollback_err = mlx5e_netdev_attach_profile(netdev, mdev,
> orig_profile, orig_ppriv);
> 	if (rollback_err) {
> 		netdev_err(netdev, "failed to rollback to orig
> profile, %d\n", rollback_err);
> 		priv->profile = NULL;
> 	}
> }
> 
> mlx5e_priv_cleanup() itself documents that state with an early
> "if (!priv->mdev) return;".  mlx5_eswitch_block_mode() then
> dereferences
> its argument right away:
> 
> drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c:mlx5_eswit
> ch_block_mode() {
> 	struct mlx5_eswitch *esw = dev->priv.eswitch;
> 	...
> }
> 
> The previous first access, "if (!priv->ipsec) return -EOPNOTSUPP;",
> handled a zeroed priv gracefully.  Would a priv->mdev check (or
> keeping
> the priv->ipsec test first) be worth adding here?

Good finding, will do.

> 
> [Severity: Medium]
> Does this change the errno userspace sees for crypto-mode offload?
> mlx5_eswitch_block_mode() forwards mlx5_esw_try_lock()'s returns:
> 
> drivers/net/ethernet/mellanox/mlx5/core/eswitch.c:mlx5_esw_try_lock()
> {
> 	if (down_write_trylock(&esw->mode_lock) == 0)
> 		return -EINVAL;
> 
> 	if (esw->eswitch_operation_in_progress ||
> 	    (check_users && atomic64_read(&esw->user_count) > 0)) {
> 		up_write(&esw->mode_lock);
> 		return -EBUSY;
> 	}
> }
> 
> Those now preempt the -EOPNOTSUPP that this netdev used to return
> whenever priv->ipsec was NULL, and only -EOPNOTSUPP gets the software
> fallback in the core:
> 
> net/xfrm/xfrm_device.c:xfrm_dev_state_add() {
> 		if ((err != -EOPNOTSUPP && !is_packet_offload) ||
> is_packet_offload) {
> 			NL_SET_ERR_MSG_WEAK(extack, "Device failed
> to offload this state");
> 			return err;
> 		}
> }
> 
> priv->ipsec is NULL when mlx5e_ipsec_init() failed (it logs "IPSec
> initialization failed" and leaves the pointer unset while
> mlx5e_ipsec_build_netdev() has already installed xfrmdev_ops), or
> while
> the profile is detached.  So "ip xfrm state add ... offload dev X" in
> crypto mode on such a netdev now fails hard with -EBUSY/-EINVAL
> instead
> of falling back to software.  Should the priv->ipsec test stay ahead
> of
> the mode block, or the eswitch error be translated for that case?

Will address.

> 
> [Severity: Medium]
> This isn't a bug introduced by this patch (the baseline already
> called
> mlx5_eswitch_block_mode(priv->mdev, true) for every non-acquire SA
> add,
> this only moves it earlier), but the two sibling add callbacks now
> sit
> side by side with opposite check_users arguments for the same stated
> purpose.  mlx5e_xfrm_add_policy() uses:
> 
> 	/* Block esw mode changes until the policy holds its own
> block. */
> 	err = mlx5_eswitch_block_mode(priv->mdev, false);
> 
> while mlx5e_xfrm_add_state() passes true, which makes
> mlx5_esw_try_lock() return -EBUSY whenever esw->user_count > 0, i.e.
> whenever any tc flower rule exists on a VF representor - the case the
> preceding commit added check_users to avoid.  Was check_users=true
> intentional for the state path?
> 
> >  
> >  	ipsec = priv->ipsec;
> > -	gfp = (x->xso.flags & XFRM_DEV_OFFLOAD_FLAG_ACQ) ?
> > GFP_ATOMIC : GFP_KERNEL;
> > +	if (!ipsec) {
> > +		err = -EOPNOTSUPP;
> > +		goto unblock_mode;
> > +	}
> > +
> > +	gfp = is_acq ? GFP_ATOMIC : GFP_KERNEL;
> >  	sa_entry = kzalloc_obj(*sa_entry, gfp);
> > -	if (!sa_entry)
> > -		return -ENOMEM;
> > +	if (!sa_entry) {
> > +		err = -ENOMEM;
> > +		goto unblock_mode;
> > +	}
> >  
> >  	sa_entry->x = x;
> >  	sa_entry->dev = dev;
> >  	sa_entry->ipsec = ipsec;
> >  	/* Check if this SA is originated from acquire flow
> > temporary SA */
> > -	if (x->xso.flags & XFRM_DEV_OFFLOAD_FLAG_ACQ) {
> > +	if (is_acq) {
> >  		x->xso.offload_handle = (unsigned long)sa_entry;
> >  		return 0;
> >  	}
> > @@ -806,10 +817,6 @@ static int mlx5e_xfrm_add_state(struct
> > net_device *dev,
> >  		goto err_xfrm;
> >  	}
> >  
> > -	err = mlx5_eswitch_block_mode(priv->mdev, true);
> > -	if (err)
> > -		goto unblock_ipsec;
> > -

Will fix.

> 
> [Severity: Medium]
> This is a pre-existing issue and not introduced here (git blame
> points
> at 8efd7b17a3b032 from 2023), but while the goto labels on this error
> path are being reshuffled it may be worth noting: the
> CONFIG_MLX5_ESWITCH=n stub has the opposite polarity from the real
> function.
> 
> drivers/net/ethernet/mellanox/mlx5/core/eswitch.h:
> static inline bool mlx5_eswitch_block_ipsec(struct mlx5_core_dev
> *dev)
> {
> 	return false;
> }
> 
> drivers/net/ethernet/mellanox/mlx5/core/eswitch.c:mlx5_eswitch_block_
> ipsec() {
> 	if (!mlx5_esw_allowed(esw))
> 		return true;
> 	...
> }
> 
> The caller just above this hunk maps false to -EBUSY:
> 
> 	if (!mlx5_eswitch_block_ipsec(priv->mdev)) {
> 		err = -EBUSY;
> 		goto err_xfrm;
> 	}
> 
> so with CONFIG_MLX5_EN_IPSEC=y and CONFIG_MLX5_ESWITCH=n, does every
> SA
> and policy offload fail with -EBUSY?  The sibling stubs follow the
> "allow" convention (mlx5_eswitch_block_encap() returns true,
> mlx5_eswitch_block_mode() returns 0).
> 
> >  	if (x->props.mode == XFRM_MODE_TUNNEL &&
> >  	    x->xso.type == XFRM_DEV_OFFLOAD_PACKET) {
> >  		allow_tunnel_mode =
> > mlx5e_ipsec_fs_tunnel_allowed(sa_entry);
> > @@ -817,7 +824,7 @@ static int mlx5e_xfrm_add_state(struct
> > net_device *dev,
> >  			NL_SET_ERR_MSG_MOD(extack,
> >  					   "Packet offload tunnel
> > mode is disabled due to encap settings");
> >  			err = -EINVAL;
> > -			goto unblock_mode;
> > +			goto unblock_ipsec;
> >  		}
> >  	}
> >  
> > @@ -893,12 +900,13 @@ static int mlx5e_xfrm_add_state(struct
> > net_device *dev,
> >  unblock_encap:
> >  	if (allow_tunnel_mode)
> >  		mlx5_eswitch_unblock_encap(priv->mdev);
> > -unblock_mode:
> > -	mlx5_eswitch_unblock_mode(priv->mdev);
> >  unblock_ipsec:
> >  	mlx5_eswitch_unblock_ipsec(priv->mdev);
> >  err_xfrm:
> >  	kfree(sa_entry);
> > +unblock_mode:
> > +	if (!is_acq)
> > +		mlx5_eswitch_unblock_mode(priv->mdev);
> >  	NL_SET_ERR_MSG_WEAK_MOD(extack, "Device failed to offload
> > this state");
> >  	return err;
> >  }
> 
> [Severity: Low]
> Placing unblock_mode above the NL_SET_ERR_MSG_WEAK_MOD() means the
> -EOPNOTSUPP (priv->ipsec == NULL) and -ENOMEM paths now attach
> "mlx5_core: Device failed to offload this state" to the extack, where
> they previously returned silently.
> 
> For crypto-mode requests the core swallows -EOPNOTSUPP and returns 0:
> 
> net/xfrm/xfrm_device.c:xfrm_dev_state_add() {
> 		if ((err != -EOPNOTSUPP && !is_packet_offload) ||
> is_packet_offload) {
> 			NL_SET_ERR_MSG_WEAK(extack, "Device failed
> to offload this state");
> 			return err;
> 		}
> 	}
> 
> 	return 0;
> }
> 
> but the message is still delivered:
> 
> net/netlink/af_netlink.c:netlink_ack_tlv_len() {
> 	if (extack->_msg)
> 		tlvlen += nla_total_size(strlen(extack->_msg) + 1);
> 	...
> 	/* Following attributes are only reported as error (not
> warning) */
> 	if (!err)
> 		return tlvlen;
> }
> 
> So does a successful "ip xfrm state add ... offload dev X" now print
> "Warning: mlx5_core: Device failed to offload this state"?  The
> sibling
> mlx5e_xfrm_add_policy() keeps its generic message above the label:
> 
> 	NL_SET_ERR_MSG_MOD(extack, "Device failed to offload this
> policy");
> unblock_mode:
> 	mlx5_eswitch_unblock_mode(priv->mdev);
> 
> Would the same layout work here?
> 
It would, will fix.

Cosmin.