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

[Shay Drori <shayd@nvidia.com>]

On 19/10/2026 12:00, netdev-bot+sashiko@kernel.org wrote:
> External email: Use caution opening links or attachments
> 
> 
> 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.

good find, will fix

pw-bot: cr

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