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

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

[Tariq Toukan]

From: Shay Drory <shayd@nvidia.com>

mlx5e_sqs2vport_stop() frees each rep_sq and its per-peer send-to-vport
rules (rep_sq->sq_peer) while holding only the uplink's priv->state_lock
(it runs from the ndo_stop -> mlx5e_deactivate_priv_channels ->
mlx5e_rep_deactivate_channels path).

The same rep_sq->sq_peer entries are inserted and erased by the devcom
ESW_OFFLOADS PAIR/UNPAIR handler (mlx5e_vport_rep_event_pair/unpair),
which runs under the devcom component write semaphore and takes no
state_lock. The two lock sets are disjoint, so closing the uplink of one
eswitch can run concurrently with a pairing/unpairing event driven by
the peer eswitch, both mutating the same rep's vport_sqs_list / sq_peer.
This is a use-after-free: one side frees rep_sq / sq_peer while the
other walks the list or inserts/erases the xarray.

The add side (mlx5e_sqs2vport_start) is not safe either. It takes the
devcom read semaphore only via mlx5_devcom_for_each_peer_begin(), which
is gated on comp->ready. The PAIR handler does all of its rep work
before setting ready, so a first-time PAIR runs with ready == false and
start can list_add() into vport_sqs_list while the handler walks it.

Add mlx5_devcom_comp_read_lock/unlock(), which take the component
semaphore for read regardless of ready, and hold it across both
mlx5e_sqs2vport_start() and mlx5e_sqs2vport_stop(). Start still adds
peer rules only when the component is ready, now checked under the
lock.

Lock ordering is preserved: start already took state_lock -> comp->sem
(read); stop now does the same. The event handler takes comp->sem and
never state_lock, so there is no inversion.

Fixes: 898b07861565 ("net/mlx5: Add send to vport rules on paired device")
Signed-off-by: Shay Drory <shayd@nvidia.com>
Reviewed-by: Moshe Shemesh <moshe@nvidia.com>
Signed-off-by: Tariq Toukan <tariqt@nvidia.com>
---
 .../net/ethernet/mellanox/mlx5/core/en_rep.c  | 21 ++++++++++---------
 .../ethernet/mellanox/mlx5/core/lib/devcom.c  | 14 +++++++++++++
 .../ethernet/mellanox/mlx5/core/lib/devcom.h  |  2 ++
 3 files changed, 27 insertions(+), 10 deletions(-)

V2:
- Add mlx5_devcom_comp_read_lock/unlock() and use them instead of the
  ready-gated mlx5_devcom_for_each_peer_begin/end(), so the PAIR handler
  is excluded even while comp->ready is false.
- Apply the same locking to mlx5e_sqs2vport_start(), whose list_add()
  could race a first-time PAIR.
- Read esw->devcom once so lock and unlock use the same pointer.
- Update the Fixes tag to 898b07861565.

answer to Sashiko comment:
The esw->devcom lifetime race with mlx5_esw_offloads_devcom_cleanup() is
pre-existing and not specific to the close path: mlx5e_sqs2vport_start()
and other users (en_tc, esw bridge, shared_fdb) dereference esw->devcom
the same way without serializing against devcom cleanup. It needs its own
fix and is out of scope for this patch.

V1:
https://lore.kernel.org/all/20261015085744.1109107-1-tariqt@nvidia.com/

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_rep.c b/drivers/net/ethernet/mellanox/mlx5/core/en_rep.c
index a50a42581528..962c70b9b566 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);
 
 	for (i = 0; i < sqns_num; i++) {
 		rep_sq = kzalloc_obj(*rep_sq);
@@ -541,7 +541,7 @@ static int mlx5e_sqs2vport_start(struct mlx5_eswitch *esw,
 		rep_sq->sqn = sqns_array[i];
 
 		xa_init(&rep_sq->sq_peer);
-		if (devcom_locked) {
+		if (peers_ready) {
 			err = mlx5e_sqs2vport_add_peers_rules(esw, rep, rep_sq, i);
 			if (err) {
 				mlx5_eswitch_del_send_to_vport_rule(rep_sq->send_to_vport_rule);
@@ -554,16 +554,13 @@ static int mlx5e_sqs2vport_start(struct mlx5_eswitch *esw,
 		list_add(&rep_sq->list, &rpriv->vport_sqs_list);
 	}
 
-	if (devcom_locked)
-		mlx5_devcom_for_each_peer_end(esw->devcom);
+	mlx5_devcom_comp_read_unlock(devcom);
 
 	return 0;
 
 out_err:
 	mlx5e_sqs2vport_stop(esw, rep);
-
-	if (devcom_locked)
-		mlx5_devcom_for_each_peer_end(esw->devcom);
+	mlx5_devcom_comp_read_unlock(devcom);
 
 	return err;
 }
@@ -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);
 }
 
 static int
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/lib/devcom.c b/drivers/net/ethernet/mellanox/mlx5/core/lib/devcom.c
index 75855481522b..aed9e2a74162 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/lib/devcom.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/lib/devcom.c
@@ -456,6 +456,20 @@ void mlx5_devcom_comp_unlock(struct mlx5_devcom_comp_dev *devcom)
 	up_write(&devcom->comp->sem);
 }
 
+void mlx5_devcom_comp_read_lock(struct mlx5_devcom_comp_dev *devcom)
+{
+	if (!devcom)
+		return;
+	down_read(&devcom->comp->sem);
+}
+
+void mlx5_devcom_comp_read_unlock(struct mlx5_devcom_comp_dev *devcom)
+{
+	if (!devcom)
+		return;
+	up_read(&devcom->comp->sem);
+}
+
 int mlx5_devcom_comp_trylock(struct mlx5_devcom_comp_dev *devcom)
 {
 	if (!devcom)
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/lib/devcom.h b/drivers/net/ethernet/mellanox/mlx5/core/lib/devcom.h
index 7a704fafdbd3..94b0fe20cc26 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/lib/devcom.h
+++ b/drivers/net/ethernet/mellanox/mlx5/core/lib/devcom.h
@@ -79,6 +79,8 @@ void *mlx5_devcom_get_next_peer_data_rcu(struct mlx5_devcom_comp_dev *devcom,
 
 void mlx5_devcom_comp_lock(struct mlx5_devcom_comp_dev *devcom);
 void mlx5_devcom_comp_unlock(struct mlx5_devcom_comp_dev *devcom);
+void mlx5_devcom_comp_read_lock(struct mlx5_devcom_comp_dev *devcom);
+void mlx5_devcom_comp_read_unlock(struct mlx5_devcom_comp_dev *devcom);
 int mlx5_devcom_comp_trylock(struct mlx5_devcom_comp_dev *devcom);
 void mlx5_devcom_comp_assert_locked(struct mlx5_devcom_comp_dev *devcom);
 

base-commit: b71c101bbb9814d163c00234d43c41f0a17907d4
-- 
2.44.0

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

[netdev-bot+sinfo]

Hi!

This is an automated message. This series looks like a fix, but its
commit messages seem to be missing some information:

 - How the issue was discovered, e.g. hit in production, hit during
   development, syzbot report, manual code inspection, LLM or static
   analysis tool scan.

 - Whether the issue was actually triggered, or is only theoretical
   (e.g. found by LLM and not proven at runtime). If it was triggered
   please include the symptoms, like the stack trace or kernel messages.

Please do not repost the series just to address the above. Instead,
reply to this email with the missing information, so that reviewers
can take it into account. If the series needs another revision for
other reasons, please include the information in the commit messages
then.

The evaluation is done by an LLM so it may be wrong, if you think
that is the case please reply and explain.

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

[sashiko-bot]

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