Re: [PATCH net v2] net/mlx5: Don't return firmware-owned command mailboxes to the DMA pool

[Moshe Shemesh <moshe@nvidia.com>]

On 9/27/2026 10:22 PM, netdev-bot+sashiko@kernel.org wrote:
> 
> Thank you for your contribution! Sashiko AI review found 5 potential
> issue(s) to consider.
> 
> Critical: 0 · High: 1 (2) · Medium: 3 · Low: 1
> 
> 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] Moving the mailbox free into cmd_ent_put() makes it run on
>    whichever context drops the last reference, with nothing synchronising…
> - [Medium] The commit message says an entry that firmware never completes
>    "keeps its mailboxes for the lifetime of the device" and calls the…
> - [Medium] Mailbox retention depends on which reference happens to be
>    dropped last, not on whether firmware still owns the descriptor.
> - [Medium] Retained mailboxes can outlive the dma_pool they came from,
>    but are freed through whatever dev->cmd.pool points to at put time.
> - [Low] The kernel-doc above mlx5_cmd_invoke() (cmd.c ~1252-1266) says
>    that for !callback, 'ret < 0 : Command execution couldn't be submitted…
> 
> Pre-existing issues:
> - [High] This problem wasn't introduced by this patch, but a command that
>    times out while the device is up holds its command slot, and the slot…
> - [High] This problem wasn't introduced by this patch, but on the
>    synchronous path a sync timeout can race with a real completion and…
> 
> --
> 
> net/mlx5: Don't return firmware-owned command mailboxes to the DMA pool
> 
> This patch uses a new own_msgs flag to put off freeing a timed-out
> command's mailboxes until the final cmd_ent_put() of the command entry.
> It does this on both the synchronous and the callback paths. The aim is to
> stop late firmware writes from landing in blocks already returned to the
> mlx5_cmd dma_pool.
> 
>> An entry firmware never completes keeps its mailboxes for the lifetime of
>> the device. dma_pool_destroy() then reports the pool busy and declines to
>> free its pages, which is the memory safe outcome: the pages stay mapped,
>> so a late write lands there rather than in memory handed back to the
>> allocator.
> 
> [Severity: Medium]
> Is "for the lifetime of the device" accurate here? The mlx5_cmd dma_pool
> is created in mlx5_cmd_enable() and destroyed in mlx5_cmd_disable(). Those
> run from mlx5_function_enable()/mlx5_function_disable() on every
> load/unload cycle, not only at probe/remove:
> 
> mlx5_cmd_enable()
>      cmd->pool = dma_pool_create("mlx5_cmd", mlx5_core_dma_dev(dev), size, align, 0);
>      ...
>      create_msg_cache(dev);
> 
> mlx5_cmd_disable()
>      destroy_msg_cache(dev);
>      free_cmd_page(dev, cmd);
>      dma_pool_destroy(cmd->pool);
> 
> If a retained entry still holds blocks, dma_pool_destroy() skips
> dma_free_coherent() for every page in the pool. That includes the pages
> behind the message cache that destroy_msg_cache() has just returned:
> 512x1 + 32x2 + 16x16 + 8x256 + 2x512 = 3904 blocks of 1024 bytes.
> 
> So each command interface incarnation that ends with an unrecovered
> timed-out mailbox appears to leak about 4 MB of DMA-coherent memory. The
> entry and its msg structs are never freed either. Before this patch the
> pool was torn down cleanly, because cmd_exec() had already freed the
> mailboxes.
> 
> Two ways this can be reached:
> 
> - A MANAGE_PAGES timeout in
>    mlx5_function_disable()->mlx5_reclaim_startup_pages(), followed by
>    mlx5_core_disable_hca() and mlx5_cmd_disable().
> - A timeout during mlx5_function_enable()/mlx5_function_open(), followed
>    by the error unwind.
> 
> Later pool incarnations start fresh, so this happens once per
> unrecovered timeout followed by a teardown, not on every later disable.
> 
> Given the Cc: stable tag, could the commit message describe this cost
> more precisely?
> 
>> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/cmd.c b/drivers/net/ethernet/mellanox/mlx5/core/cmd.c
>> index 84583dc5eb1c0..4051f97b2ae12 100644
>> --- a/drivers/net/ethernet/mellanox/mlx5/core/cmd.c
>> +++ b/drivers/net/ethernet/mellanox/mlx5/core/cmd.c
> 
> [ ... ]
> 
>> @@ -207,6 +214,11 @@ static void cmd_ent_put(struct mlx5_cmd_work_ent *ent)
>>        }
>>        spin_unlock_irqrestore(&cmd->alloc_lock, flags);
>>
>> +     if (ent->own_msgs) {
>> +             mlx5_free_cmd_msg(dev, ent->out);
>> +             free_msg(dev, ent->in);
>> +     }
>> +
> 
> [Severity: Medium]
> Can a retained mailbox end up freed into a different dma_pool than the
> one it was allocated from? These frees always go through whatever
> dev->cmd.pool and dev->cmd.cache[] are when the last put happens.
> 
> One possible sequence:
> 
> 1. A timed-out entry keeps its FW reference, with own_msgs set.
> 
> 2. mlx5_cmd_disable() runs destroy_msg_cache() and dma_pool_destroy().
>     The pool is busy, so its pages are kept, but the struct dma_pool is
>     still kfree()d.
> 
> 3. mlx5_cmd_enable() does memset(&cmd->vars, 0, ...), sets
>     cmd->vars.bitmask = MLX5_CMD_MASK, and creates a new pool and cache.
>     cmd->ent_arr[] is outside vars and is not cleared, so ent_arr[i]
>     still points at the stale entry.
> 
> 4. A non-forced completion for bit i arrives before slot i is reused.
>     mlx5_cmd_comp_handler() finds PENDING_COMP clear, logs "Command
>     completion arrived after timeout" and calls cmd_ent_put().
> 
> 5. That is the final reference. mlx5_free_cmd_msg()->free_cmd_box() then
>     calls dma_pool_free(dev->cmd.pool, ...) on the new pool with a block
>     from the old one. Without DMAPOOL_DEBUG, pool_block_err() does not
>     catch this. The block goes onto the new pool's free list, and the new
>     pool's nr_active is decremented.
> 
> 6. free_msg(dev, ent->in) also adds an old-pool cached msg to the rebuilt
>     cache list.
> 
> After that, the new pool's nr_active can read 0 while another timed-out
> entry still owns a real block. A later dma_pool_destroy() would then
> dma_free_coherent() pages that firmware may still write to.
> 
> Before this patch, a stale final put only did cmd_free_index(), up() and
> kfree(ent). It never touched the pool or the cache.
> 
>>        cmd_free_ent(ent);
>>   }
>>
>> @@ -958,10 +970,6 @@ static void cb_timeout_handler(struct work_struct *work)
>>        cmd_ent_put(ent); /* for the cmd_ent_get() took on schedule delayed work */
>>   }
> 
> [Severity: High]
> Now that cmd_ent_put() can free mailboxes, can this put race with
> command interface teardown?

That's a valid concern, in cmd_ent_put() mailbox should be freed before 
up() not after.
Also I would drop the last paragraph of the commit message, as we don't 
handle here "An entry firmware never completes".
Thanks, Moshe.

> 
> cmd_work_handler() takes a reference for the timeout work:
> 
>      if (ent->callback && schedule_delayed_work(&ent->cb_timeout_work, timeout))
>          cmd_ent_get(ent);
> 
> Take the "Async, recovered after timeout" case.
> mlx5_cmd_eq_recover() recovers the EQE, and mlx5_cmd_comp_handler()
> cannot cancel the running work, because cancel_delayed_work() returns
> false. The handler drops the FW and consumer references, sets own_msgs
> and calls the callback.
> 
> That leaves the put above in cb_timeout_handler(), running on
> system_wq, as the final one. The same happens when a real completion
> races with the timeout work.
> 
> In cmd_ent_put() the slot and semaphore are released under alloc_lock,
> and the mailboxes are only freed after the lock is dropped:
> 
>      cmd_free_index(cmd, ent->idx);
>      up(ent->page_queue ? &cmd->vars.pages_sem : &cmd->vars.sem);
>      ...
>      spin_unlock_irqrestore(&cmd->alloc_lock, flags);
> 
>      if (ent->own_msgs) {
>          mlx5_free_cmd_msg(dev, ent->out);
>          free_msg(dev, ent->in);
>      }
> 
> The teardown paths don't wait for that free:
> 
> - mlx5_cmd_allowed_opcode() and mlx5_cmd_flush() only wait for the up().
> - mlx5_cmd_cleanup_async_ctx() only waits for the callback.
> - mlx5_cmd_disable() only flushes cmd->wq, not system_wq.
> 
> Suppose this worker is delayed just after the up(). Could
> mlx5_cmd_disable() get through destroy_msg_cache() and dma_pool_destroy()
> first? The pool is busy, so its struct is kfree()d. The worker would then
> call dma_pool_free() on the freed pool, or on a newly created one, and
> list_add_tail() onto a destroyed or re-initialised cache list.
> 
> [ ... ]
> 
>> @@ -1313,7 +1321,12 @@ static int mlx5_cmd_invoke(struct mlx5_core_dev *dev, struct mlx5_cmd_msg *in,
>>                return 0; /* mlx5_cmd_comp_handler() will put(ent) */
>>
>>        err = wait_func(dev, ent);
>> -     if (err == -ETIMEDOUT || err == -ECANCELED || err == -EBUSY)
>> +     if (err == -ETIMEDOUT) {
>> +             /* firmware may still DMA into the mailboxes; keep them */
>> +             ent->own_msgs = true;
>> +             goto out_free;
>> +     }
> 
> [Severity: High]
> This isn't a bug introduced by this patch, but can a sync timeout race
> with a real completion and free ent and its messages while the IRQ side
> is still using them?
> 
> CPU A: wait_func_handle_exec_timeout()
>      ent->ret = -ETIMEDOUT;
>      mlx5_cmd_comp_handler(dev, 1ULL << ent->idx, true);
>          loses test_and_clear_bit(MLX5_CMD_ENT_STATE_PENDING_COMP)
>          continue;
> 
> CPU B: EQ IRQ, mlx5_cmd_comp_handler(dev, vec, false)
>      wins test_and_clear_bit(MLX5_CMD_ENT_STATE_PENDING_COMP)
>      cmd_ent_put(ent);        /* refcount 2 -> 1 */
>      ent->ts2 = ktime_get_ns();
>      memcpy(ent->out->first.data, ent->lay->out, sizeof(ent->lay->out));
>      dump_command(dev, ent, 0);
>      ...
>      complete(&ent->done);
> 
> On CPU A, wait_func() returns -ETIMEDOUT without waiting on ent->done
> again. mlx5_cmd_invoke() sets own_msgs and makes the final
> cmd_ent_put() (1 -> 0), which frees ent->out, ent->in and ent. CPU B may
> still be writing ent->out and calling complete(&ent->done) at that point.
> 
> The same window existed before, with cmd_exec() freeing the messages
> right after the put. This change moves the frees into that final put but
> doesn't close the window.
> 
> [Severity: Low]
> This isn't a bug, but the kernel-doc above mlx5_cmd_invoke() still says:
> 
>   * return value in case (!callback):
>   *      ret < 0 : Command execution couldn't be submitted by driver
> 
> With this change, -ETIMEDOUT means the command was submitted and in/out
> now belong to the entry, so the caller must not free them. Only
> cmd_exec() knows this, through the new goto out_up. Could the comment
> document the ownership transfer, so a future caller that frees on a
> negative return doesn't double-free?
> 
>> +     if (err == -ECANCELED || err == -EBUSY)
>>                goto out_free;
>>
>>        ds = ent->ts2 - ent->ts1;
>> @@ -1816,8 +1829,10 @@ static void mlx5_cmd_comp_handler(struct mlx5_core_dev *dev, u64 vec, bool force
>>                                                                 ent->out,
>>                                                                 ent->uout_size);
>>
>> -                             mlx5_free_cmd_msg(dev, ent->out);
>> -                             free_msg(dev, ent->in);
>> +                             /* firmware may still DMA into the mailboxes;
>> +                              * keep them
>> +                              */
>> +                             ent->own_msgs = true;
>>
>>                                /* final consumer is done, release ent */
>>                                cmd_ent_put(ent);
> 
> [Severity: High]
> This is a pre-existing issue, not one introduced by this patch. It is
> raised here because the patch handles exactly these retained entries.
> Can a command that times out while the device is up hold its slot
> semaphore forever, and leave mlx5_cmd_flush() spinning?
> 
> After the timeout, the forced mlx5_cmd_comp_handler() clears
> PENDING_COMP but keeps the FW reference, because mlx5_cmd_is_down() is
> false and the opcode is allowed. The final cmd_ent_put(), which does
> up(&cmd->vars.sem), therefore only runs on a real completion.
> 
> enter_error_state() then sets MLX5_DEVICE_STATE_INTERNAL_ERROR and calls
> mlx5_cmd_flush():
> 
>          for (i = 0; i < cmd->vars.max_reg_cmds; i++) {
>                  while (down_trylock(&cmd->vars.sem)) {
>                          mlx5_cmd_trigger_completions(dev);
>                          cond_resched();
>                  }
>          }
> 
> For the timed-out entry, mlx5_cmd_trigger_completions() takes and drops
> a temporary reference. The forced handler finds PENDING_COMP already
> clear and hits continue without a put. cmd_comp_notifier() ignores real
> EQEs once dev->state == MLX5_DEVICE_STATE_INTERNAL_ERROR, so the permit
> never comes back.
> 
> This loop is reached from the fatal health reporter, PCI err_detected,
> sync FW reset, and shutdown through
> mlx5_try_fast_unload()->mlx5_enter_error_state(). Those callers hold
> intf_state_mutex while it runs. On unload, the same held permit also
> blocks the down() loops in mlx5_cmd_allowed_opcode() and
> mlx5_cmd_change_mod().
> 
> Wouldn't recovery, unload and reboot all hang here?
> 
> [ ... ]
> 
>> @@ -2012,6 +2027,9 @@ static int cmd_exec(struct mlx5_core_dev *dev, void *in, int in_size, void *out,
>>        if (callback && !err)
>>                return 0;
>>
>> +     if (err == -ETIMEDOUT) /* the command entry owns the mailboxes now */
>> +             goto out_up;
>> +
> 
> [Severity: Medium]
> Does this cover every case where firmware still owns the mailboxes?
> Whether they are kept seems to depend on which reference is dropped
> last, not on whether the descriptor is still owned by hardware.
> 
> First, a forced completion in mlx5_cmd_comp_handler() still drops the
> FW reference when the device looks down:
> 
>                          if (!forced || /* Real FW completion */
>                               mlx5_cmd_is_down(dev) || /* No real FW completion is expected */
>                               !opcode_allowed(cmd, ent->op))
>                                  cmd_ent_put(ent);
> 
> mlx5_cmd_is_down() becomes true as soon as poll_health() sets
> MLX5_DEVICE_STATE_INTERNAL_ERROR on a fatal sensor (for example
> MLX5_SENSOR_FW_SYND_RFR). That happens before any reset or DMA
> quiescence.
> 
> If wait_func_handle_exec_timeout() fires in that state, the put in
> mlx5_cmd_invoke() is final. cmd_ent_put() then frees ent->in and ent->out
> straight away, even though own_msgs was just set. On the async path the
> put in cb_timeout_handler() is the final one.
> 
> Second, mlx5_cmd_flush()->mlx5_cmd_trigger_completions() force-completes
> every allocated slot with MLX5_TRIGGERED_CMD_COMP, so ent->ret becomes
> -ENXIO.
> 
> For sync commands, only -ETIMEDOUT is special-cased above. -ENXIO falls
> through to out_out, and mlx5_free_cmd_msg()/free_msg() release blocks
> for commands that have already been doorbelled. For callback commands,
> own_msgs is set, but the is_down check drops the FW reference and the
> trigger loop's cmd_ent_put() frees them.
> 
> If firmware completes one of these commands before the reset, couldn't
> the late write land in a freed block, or in pool pages freed by a later
> clean dma_pool_destroy()? That is the same corruption the commit message
> describes.
> 
> The new "firmware may still DMA into the mailboxes; keep them" comments
> promise a retention that these paths don't provide.
> 
> --
> Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260923190542.848049-1-dcostantino%40meta.com