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

[PATCH net] 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) already guards the peer-rule work
with mlx5_devcom_for_each_peer_begin(), i.e. the devcom read semaphore,
which excludes the write-side event. The stop side did not. Take the
same read side around the sq_peer teardown so it cannot race the event
handler.

Lock ordering is preserved: like the start path, stop already holds
state_lock and then takes the devcom semaphore (state_lock ->
comp->sem); the event handler takes comp->sem and never state_lock, so
there is no inversion.

Fixes: ed7a8fe71836 ("net/mlx5e: rep, store send to vport rules per peer")
Signed-off-by: Shay Drory <shayd@nvidia.com>
Reviewed-by: Moshe Shemesh <moshe@nvidia.com>
Signed-off-by: Tariq Toukan <tariqt@nvidia.com>
---
 drivers/net/ethernet/mellanox/mlx5/core/en_rep.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_rep.c b/drivers/net/ethernet/mellanox/mlx5/core/en_rep.c
index 88a170e40bd9..7211faaffd8d 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_rep.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_rep.c
@@ -629,8 +629,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;
+	bool devcom_locked;
 
+	devcom_locked = mlx5_devcom_for_each_peer_begin(esw->devcom);
 	mlx5e_sqs2vport_stop(esw, rep);
+	if (devcom_locked)
+		mlx5_devcom_for_each_peer_end(esw->devcom);
 }
 
 static int

base-commit: 513444cc62e231a11c9b15ea88de0890da9b2b4f
-- 
2.44.0

Re: [PATCH net] 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 code inspection). If it was triggered please include
   the symptoms, like the stack trace or error messages.

 - What hardware the change was tested on. For driver fixes please
   mention the device (and if relevant firmware version) used for
   testing, or say that the change was not tested on real hardware.

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] net/mlx5e: rep, Fix races between devcom PAIR event and uplink close

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider.

Critical: 0 · High: 2 · Medium: 0 · Low: 0

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] The fix is incomplete.
- [High] The patch adds a lockless load and dereference of esw->devcom to
  every rep channel deactivation.

--

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

This commit wraps mlx5e_sqs2vport_stop() in mlx5e_remove_sqs_fwd_rules()
with mlx5_devcom_for_each_peer_begin()/end(). The goal is to serialize
rep_sq and sq_peer teardown against the devcom ESW_OFFLOADS PAIR/UNPAIR
event handler, which follows the pattern already used on the add side.

> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_rep.c b/drivers/net/ethernet/mellanox/mlx5/core/en_rep.c
> index 88a170e40bd9..7211faaffd8d 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/en_rep.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_rep.c
> @@ -629,8 +629,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;
> +	bool devcom_locked;
>  
> +	devcom_locked = mlx5_devcom_for_each_peer_begin(esw->devcom);

[Severity: High]
Before this patch, the stop path never touched esw->devcom. Now every rep
channel deactivation loads and dereferences it with no lock held, and does
so before the esw->mode check in mlx5e_sqs2vport_stop(). Can esw->devcom
be freed while this runs?

esw_offloads_disable() cleans up devcom before it unloads the reps:

	mlx5_esw_offloads_devcom_cleanup(esw);
	mlx5_eswitch_disable_pf_vf_vports(esw);
	mlx5_esw_offloads_rep_unload(esw, MLX5_VPORT_UPLINK);

mlx5_esw_offloads_devcom_cleanup() frees the devcom before it clears the
pointer:

	mlx5_devcom_unregister_component(esw->devcom);
	xa_destroy(&esw->paired);
	xa_destroy(&esw->fdb_table.offloads.peer_miss_rules);
	esw->devcom = NULL;

The free happens in mlx5_devcom_unregister_component()->
devcom_free_comp_dev(), which does kfree(devcom) and then
kref_put(&comp->ref, mlx5_devcom_comp_release). That also frees comp if
this was the last member.

One possible interleaving:

CPU0 (ip link set <rep> down)
mlx5e_rep_deactivate_channels()->mlx5e_remove_sqs_fwd_rules()
  mlx5_devcom_for_each_peer_begin(esw->devcom)   /* loads P */

CPU1 (devlink eswitch mode set legacy)
mlx5_devlink_eswitch_mode_set()->mlx5_eswitch_disable_locked()
  ->esw_offloads_disable()->mlx5_esw_offloads_devcom_cleanup()
    kfree(P), possibly frees P->comp as well

CPU0
  comp = devcom->comp;
  down_read(&comp->sem);

The ndo_stop path holds rtnl, the netdev lock and priv->state_lock. The
devlink mode set path holds the devlink lock and reps_lock. Nothing
appears to serialize the two.

Could this become a use-after-free on the comp_dev and on comp->sem? The
start path already has a similar unsynchronized use of esw->devcom, but
this patch adds a new one on the close path.

>  	mlx5e_sqs2vport_stop(esw, rep);
> +	if (devcom_locked)
> +		mlx5_devcom_for_each_peer_end(esw->devcom);
>  }

[Severity: High]
Does this fully close the race with the PAIR event?
mlx5_devcom_for_each_peer_begin() drops the semaphore and returns false
when the component is not ready:

lib/devcom.c:mlx5_devcom_for_each_peer_begin() {
	...
	down_read(&comp->sem);
	if (!READ_ONCE(comp->ready)) {
		up_read(&comp->sem);
		return false;
	}
	...
}

In that case mlx5e_sqs2vport_stop() still runs, with no devcom exclusion.
It calls list_del() and kfree() on each rep_sq, kfree() on each sq_peer,
and xa_destroy(&rep_sq->sq_peer).

The PAIR handler, though, does all of its rep work while ready is still
false. mlx5_esw_offloads_devcom_event() sets ready only at the end:

	err = mlx5_esw_offloads_pair(esw, peer_esw);
	...
	err = mlx5_esw_offloads_pair(peer_esw, esw);
	...
	mlx5_devcom_comp_set_ready(esw->devcom, true);

UNPAIR sets ready back to false once the last peer is gone. With two
ports, every first PAIR and every re-pair therefore runs with ready set
to false.

One possible interleaving:

CPU0 (ip link set <rep> down on eswitch A)
mlx5e_rep_close()->mlx5e_close_locked()->mlx5e_deactivate_priv_channels()
  ->mlx5e_rep_deactivate_channels()->mlx5e_remove_sqs_fwd_rules()
    mlx5_devcom_for_each_peer_begin() returns false
    mlx5e_sqs2vport_stop()
      list_del(&rep_sq->list);
      kfree(rep_sq);

CPU1 (peer eswitch B entering switchdev)
mlx5_devcom_send_event(PAIR), which takes comp->sem for write
  ->mlx5_esw_offloads_devcom_event()->mlx5_esw_offloads_pair()
    ->mlx5e_vport_rep_event_pair()
      list_for_each_entry(rep_sq, &rpriv->vport_sqs_list, list) {
          sq_peer = xa_load(&rep_sq->sq_peer, i);
          ...
          err = xa_insert(&rep_sq->sq_peer, i, sq_peer, GFP_KERNEL);

Can CPU1 then walk a freed rep_sq, or call xa_insert() on an xarray that
has already been destroyed?

If the insert lands between the xa_for_each() and the xa_destroy() in
mlx5e_sqs2vport_stop(), the sq_peer and its peer FDB send-to-vport rule
look like they would be leaked.

The commit message says:

  "The add side (mlx5e_sqs2vport_start) already guards the peer-rule work
  with mlx5_devcom_for_each_peer_begin(), i.e. the devcom read semaphore,
  which excludes the write-side event."

mlx5e_sqs2vport_start() uses the same ready-gated check:

	if (mlx5_devcom_comp_is_ready(esw->devcom) &&
	    mlx5_devcom_for_each_peer_begin(esw->devcom))
		devcom_locked = true;

As a result, its list_add() into rpriv->vport_sqs_list can also run while
a first-time PAIR is walking the list.

Would both the start and stop paths need exclusion that does not depend
on comp->ready? One option is to hold comp->sem for read whenever
esw->devcom is non-NULL, which would need a new devcom helper. Another is
a lock that the event handler also takes.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261015085744.1109107-1-tariqt%40nvidia.com