Re: [PATCH net V2] net/mlx5e: rep, Fix races between devcom PAIR event and uplink close

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Medium] net/mlx5e: esw->devcom UAF race with
  mlx5_esw_offloads_devcom_cleanup()
--

commit 7323974cbd0d255bb995fa9d0fba3a2187d60aad
Author: Shay Drory <shayd@nvidia.com>

net/mlx5e: rep, Fix races between devcom PAIR event and uplink close

This commit adds mlx5_devcom_comp_read_lock() and
mlx5_devcom_comp_read_unlock() and holds the devcom component read
semaphore across mlx5e_sqs2vport_start() and mlx5e_sqs2vport_stop() to
serialize against devcom PAIR and UNPAIR events.

> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_rep.c b/drivers/net/ethernet/mellanox/mlx5/core/en_rep.c
> index a50a42581528a..962c70b9b5661 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/en_rep.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_rep.c
> @@ -506,10 +506,11 @@ static int mlx5e_sqs2vport_start(struct mlx5_eswitch *esw,
>  				 struct mlx5_eswitch_rep *rep,
>  				 u32 *sqns_array, int sqns_num)
>  {
> +	struct mlx5_devcom_comp_dev *devcom = esw->devcom;
>  	struct mlx5_flow_handle *flow_rule;
>  	struct mlx5e_rep_priv *rpriv;
>  	struct mlx5e_rep_sq *rep_sq;
> -	bool devcom_locked = false;
> +	bool peers_ready;
>  	int err;
>  	int i;
>  
> @@ -518,9 +519,8 @@ static int mlx5e_sqs2vport_start(struct mlx5_eswitch *esw,
>  
>  	rpriv = mlx5e_rep_to_rep_priv(rep);
>  
> -	if (mlx5_devcom_comp_is_ready(esw->devcom) &&
> -	    mlx5_devcom_for_each_peer_begin(esw->devcom))
> -		devcom_locked = true;
> +	mlx5_devcom_comp_read_lock(devcom);
> +	peers_ready = mlx5_devcom_comp_is_ready(devcom);

[ ... ]

> @@ -630,8 +627,12 @@ mlx5e_remove_sqs_fwd_rules(struct mlx5e_priv *priv)
>  	struct mlx5_eswitch *esw = priv->mdev->priv.eswitch;
>  	struct mlx5e_rep_priv *rpriv = priv->ppriv;
>  	struct mlx5_eswitch_rep *rep = rpriv->rep;
> +	struct mlx5_devcom_comp_dev *devcom;
>  
> +	devcom = esw->devcom;
> +	mlx5_devcom_comp_read_lock(devcom);
>  	mlx5e_sqs2vport_stop(esw, rep);
> +	mlx5_devcom_comp_read_unlock(devcom);
>  }

[Severity: Medium]
Can mlx5e_remove_sqs_fwd_rules() and mlx5e_sqs2vport_start() race with
mlx5_esw_offloads_devcom_cleanup() and dereference a freed esw->devcom?

esw_offloads_disable() tears down esw->devcom while esw->mode is still
MLX5_ESWITCH_OFFLOADS and before the VF and uplink representor netdevs
are unloaded:

drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c:esw_offloads_disable() {
    ...
	mlx5_esw_offloads_devcom_cleanup(esw);
	mlx5_eswitch_disable_pf_vf_vports(esw);
	mlx5_esw_offloads_rep_unload(esw, MLX5_VPORT_UPLINK);
    ...
}

In mlx5_esw_offloads_devcom_cleanup(), mlx5_devcom_unregister_component()
frees esw->devcom (and devcom->comp when the last peer unregisters)
before clearing esw->devcom = NULL.  esw_offloads_disable() holds
devl_lock and esw->offloads.reps_lock, while representor netdev callbacks
(such as mlx5e_close() or mlx5e_open()) hold only rtnl_lock/netdev_lock
and priv->state_lock:

CPU0                                    CPU1
esw_offloads_disable()                  mlx5e_close()
  mlx5_esw_offloads_devcom_cleanup()      ...
                                          mlx5e_remove_sqs_fwd_rules()
                                            devcom = esw->devcom;
    mlx5_devcom_unregister_component()
      devcom_free_comp_dev(devcom)
        kfree(devcom)
                                            mlx5_devcom_comp_read_lock(devcom)
                                              down_read(&devcom->comp->sem)
                                              <-- use-after-free
    esw->devcom = NULL;

mlx5e_sqs2vport_start() has the same race because it now calls
mlx5_devcom_comp_read_lock(devcom) unconditionally without checking
comp->ready first.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261112182403.3646298-1-tariqt@nvidia.com?part=1