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

[Cosmin Ratiu <cratiu@nvidia.com>]

On Tue, 2026-10-06 at 17:51 -0700, Jakub Kicinski wrote:
> 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.

Will fix.

> 
> [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.

A new and exciting deadlock! Will fix.

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

Will fix.

> 
> [ ... ]
> 
> > 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.

Will improve.

Cosmin.