Re: [PATCH net] net/mlx5: Fix lock dependency cycle on uplink unbind in switchdev mode

[Shay Drori <shayd@nvidia.com>]

On 19/10/2026 12:34, 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: 1 · Medium: 1 · 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] mlx5_eswitch_safe_aux_devs_remove() now sets
>    esw->eswitch_operation_in_progress = true, drops mode_lock, removes…
> - [Medium] The patch replaces the read trylock (mlx5_esw_hold(),
>    down_read_trylock) with a write trylock in esw_try_lock_ignore_users()…
> 
> --
> 
> net/mlx5: Fix lock dependency cycle on uplink unbind in switchdev mode
> 
> This patch stops mlx5_eswitch_safe_aux_devs_remove() from holding
> esw->mode_lock for read while it removes the REP aux devices. It now takes
> mode_lock for write through a new esw_try_lock_ignore_users() helper, sets
> eswitch_operation_in_progress, drops mode_lock, removes the REPs, and then
> retakes mode_lock to clear the flag.
> 
>> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c b/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c
>> index eb74b6260168..f934cb2b26b0 100644
>> --- a/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c
>> +++ b/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c
>> @@ -4438,15 +4438,36 @@ static bool mlx5_devlink_switchdev_active_mode_change(struct mlx5_eswitch *esw,
>>   #define MLX5_ESW_HOLD_TIMEOUT_MS 7000
>>   #define MLX5_ESW_HOLD_RETRY_DELAY_MS 500
>>
>> +/* Unlike mlx5_esw_try_lock(), don't refuse the lock over eswitch users.
>> + * Teardown must run even with TC flows still offloaded, as those are only
>> + * flushed once the REPs are removed.
>> + */
>> +static bool esw_try_lock_ignore_users(struct mlx5_eswitch *esw)
>> +{
>> +     if (!down_write_trylock(&esw->mode_lock))
>> +             return false;
>> +
>> +     if (esw->eswitch_operation_in_progress) {
>> +             up_write(&esw->mode_lock);
>> +             return false;
>> +     }
>> +
>> +     return true;
>> +}
>> +
>>   void mlx5_eswitch_safe_aux_devs_remove(struct mlx5_core_dev *dev)
>>   {
>> +     struct mlx5_eswitch *esw = dev->priv.eswitch;
>>        unsigned long timeout;
>>        bool hold_esw = true;
>>
>> +     if (!mlx5_esw_allowed(esw))
>> +             return;
>> +
>>        /* Wait for any concurrent eswitch mode transition to complete. */
>> -     if (!mlx5_esw_hold(dev)) {
>> +     if (!esw_try_lock_ignore_users(esw)) {
>>                timeout = jiffies + msecs_to_jiffies(MLX5_ESW_HOLD_TIMEOUT_MS);
>> -             while (!mlx5_esw_hold(dev)) {
>> +             while (!esw_try_lock_ignore_users(esw)) {
>>                        if (!time_before(jiffies, timeout)) {
>>                                hold_esw = false;
>>                                break;
> 
> [Severity: Medium]
> Can concurrent TC flow insertion now make this loop time out?
> 
> The old mlx5_esw_hold() used down_read_trylock(), and other readers never
> block that. esw_try_lock_ignore_users() uses down_write_trylock(), which
> fails whenever any reader holds mode_lock.
> 
> mlx5e_configure_flower() holds mode_lock for read during the whole flow
> insert, FW commands included:
> 
> drivers/net/ethernet/mellanox/mlx5/core/en_tc.c:mlx5e_configure_flower() {
>      ...
>          if (!mlx5_esw_hold(priv->mdev))
>                  return -EBUSY;
>      ...
> }
> 
> TC offload on the REPs doesn't take rtnl, so several of these can run at
> the same time. If flow inserts keep overlapping (for example under heavy
> OVS offload), every 500 ms poll could find a reader. After
> MLX5_ESW_HOLD_TIMEOUT_MS the loop sets hold_esw = false.
> 
> In that case the IB and ETH REP aux devices are never removed, and nothing
> is logged. _mlx5e_remove() then goes on to tear down the uplink:
> 
> _mlx5e_remove()
>      mlx5_eswitch_safe_aux_devs_remove(mdev);   <- REPs left bound
>      mlx5_core_uplink_netdev_set(mdev, NULL);
>      ...
>      unregister_netdev() / mlx5e_destroy_netdev()
> 
> The uplink REP and the IB REP still reference that netdev. Could this bring
> back the unregister_netdevice hang, or a later use of the freed netdev, that
> commit 2ae8c7edea87 fixed?
> 
>> @@ -4455,9 +4476,19 @@ void mlx5_eswitch_safe_aux_devs_remove(struct mlx5_core_dev *dev)
>>                }
>>        }
>>        if (hold_esw) {
>> +             /* Keep mode_lock and reps_lock unnested. The operation flag
>> +              * excludes mode users while mode_lock is dropped before the
>> +              * REPs removal takes reps_lock.
>> +              */
>> +             esw->eswitch_operation_in_progress = true;
>> +             mlx5_esw_unlock(esw);
>> +
>>                if (mlx5_eswitch_mode(dev) == MLX5_ESWITCH_OFFLOADS)
>>                        mlx5_core_reps_aux_devs_remove(dev);
>> -             mlx5_esw_release(dev);
>> +
>> +             down_write(&esw->mode_lock);
>> +             esw->eswitch_operation_in_progress = false;
>> +             mlx5_esw_unlock(esw);
> 
> [Severity: High]
> Does this flag really keep other writers out while mode_lock is dropped?
> 
> The commit message says "The flag keeps excluding concurrent mode
> transitions and mode users while mode_lock is not held". However, several
> devlink paths take mode_lock with a plain down_write(). They set and later
> clear the flag without checking it first:
> 
> mlx5_devlink_eswitch_encap_mode_set() {
>          down_write(&esw->mode_lock);
>          ...
>          esw->eswitch_operation_in_progress = true;
>          up_write(&esw->mode_lock);
> 
>          esw_destroy_offloads_fdb_tables(esw);
>          ...
>          err = esw_create_offloads_fdb_tables(esw);
>          ...
>          down_write(&esw->mode_lock);
>          esw->eswitch_operation_in_progress = false;
>      ...
> }
> 
> mlx5_devlink_eswitch_inline_mode_set() does the same around
> mlx5_esw_vports_inline_set(). mlx5_devlink_switchdev_active_mode_change()
> does it around mlx5_esw_fdb_inactive() and mlx5_esw_fdb_active().
> mlx5_devlink_eswitch_mode_set() calls that function before it calls
> mlx5_esw_try_lock().
> 
> Before this patch, the read lock held across REP removal blocked all of
> these in down_write(). The other flag setters run under devl_lock. The
> unbind path (unbind_store->mlx5e_remove->_mlx5e_remove) doesn't take
> devl_lock, so this setter can overlap them.
> 
> One possible interleaving:
> 
> CPU0 (unbind)                        CPU1 (devlink encap-mode set)
> flag = true, up_write()
> mlx5_core_reps_aux_devs_remove()
>                                       down_write() succeeds
>                                       flag = true, up_write()
>                                       esw_destroy_offloads_fdb_tables()
> down_write()
> flag = false, up_write()
>                                       esw_create_offloads_fdb_tables()
> 
> Once CPU0 clears the flag, mlx5e_configure_flower()->mlx5_esw_hold() can
> succeed while CPU1 is still rebuilding the FDB tables.
> 
> The reverse order is possible too. If CPU1 finishes first, it clears the
> flag while CPU0 is still removing REPs. A later
> mlx5_devlink_eswitch_mode_set() can then pass mlx5_esw_try_lock() (when
> user_count is 0) and start a mode transition in the middle of REP removal.
> 
> CPU0 checked mlx5_eswitch_mode() without holding mode_lock. When it
> finishes, it also clears the flag that mode_set just raised.
> 
> This looks like a flag being used as a lock: set on entry and cleared on
> exit. Should the other writers check the flag before setting it, or should
> the bool be replaced with a real lock or a counter?

good find. will fix

pw-bot: cr

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