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

[netdev-bot+sashiko@kernel.org]

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