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

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

[Danielle Costantino]

On a command timeout mlx5_cmd_comp_handler(forced) keeps the entry and its
queue slot, because firmware may still complete the command and write to
ent->lay. cmd_exec() and the callback path free the mailboxes anyway,
while ent->lay->{in_ptr,out_ptr} still point at them.

dma_pool keeps its free list node in the first 16 bytes of the block,
which for mlx5 is the start of the command payload, so a late firmware
write corrupts the allocator:

  Unable to handle kernel paging request at virtual address
  0007c830040001a0
  pc : dma_pool_alloc+0x48/0x430   lr : mlx5_alloc_cmd_msg+0x154/0x318
  Call trace:
   dma_pool_alloc+0x48/0x430 (P)
   mlx5_alloc_cmd_msg+0x154/0x318
   cmd_exec+0x24c/0xb28
   mlx5_access_reg+0xe8/0x1c8

Two crash dumps show the aliasing: 31 of 32 slots held an entry with
ret == -ETIMEDOUT, 28 of 31 shared one ent->lay->out_ptr while every
in_ptr was distinct, and pool->next_block held a non-kernel address.

Transfer mailbox ownership to the entry and release it on its final put,
on both the synchronous and the callback path. For a real completion that
put is normally the last one, so nothing moves in practice.

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.

Fixes: 73dd3a4839c1 ("net/mlx5: Avoid using pending command interface slots")
Cc: stable@vger.kernel.org
Signed-off-by: Danielle Costantino <dcostantino@meta.com>
---
 drivers/net/ethernet/mellanox/mlx5/core/cmd.c | 32 +++++++++++++++----
 include/linux/mlx5/driver.h                   |  1 +
 2 files changed, 26 insertions(+), 7 deletions(-)

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
@@ -185,6 +185,10 @@ static void cmd_free_index(struct mlx5_cmd *cmd, int idx)
 	set_bit(idx, &cmd->vars.bitmask);
 }
 
+static void free_msg(struct mlx5_core_dev *dev, struct mlx5_cmd_msg *msg);
+static void mlx5_free_cmd_msg(struct mlx5_core_dev *dev,
+			      struct mlx5_cmd_msg *msg);
+
 static void cmd_ent_get(struct mlx5_cmd_work_ent *ent)
 {
 	refcount_inc(&ent->refcnt);
@@ -193,8 +197,11 @@ static void cmd_ent_get(struct mlx5_cmd_work_ent *ent)
 static void cmd_ent_put(struct mlx5_cmd_work_ent *ent)
 {
 	struct mlx5_cmd *cmd = ent->cmd;
+	struct mlx5_core_dev *dev;
 	unsigned long flags;
 
+	dev = container_of(cmd, struct mlx5_core_dev, cmd);
+
 	spin_lock_irqsave(&cmd->alloc_lock, flags);
 	if (!refcount_dec_and_test(&ent->refcnt)) {
 		spin_unlock_irqrestore(&cmd->alloc_lock, flags);
@@ -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);
+	}
+
 	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 */
 }
 
-static void free_msg(struct mlx5_core_dev *dev, struct mlx5_cmd_msg *msg);
-static void mlx5_free_cmd_msg(struct mlx5_core_dev *dev,
-			      struct mlx5_cmd_msg *msg);
-
 static bool opcode_allowed(struct mlx5_cmd *cmd, u16 opcode)
 {
 	if (cmd->allowed_opcode == CMD_ALLOWED_OPCODE_ALL)
@@ -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;
+	}
+	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);
@@ -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;
+
 	if (err > 0) /* Failed in FW, command didn't execute */
 		err = deliv_status_to_err(err);
 
diff --git a/include/linux/mlx5/driver.h b/include/linux/mlx5/driver.h
index 83d0a83bbfbca..9fcbc6070869f 100644
--- a/include/linux/mlx5/driver.h
+++ b/include/linux/mlx5/driver.h
@@ -855,6 +855,7 @@ struct mlx5_cmd_work_ent {
 	u64			ts2;
 	u16			op;
 	bool			polling;
+	bool			own_msgs;
 	/* Track the max comp handlers */
 	refcount_t              refcnt;
 };

base-commit: 9c572a83037a7dcd653ba3a9cc468c16b857d0c9

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

[netdev-bot+sashiko]

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?

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

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

[Moshe Shemesh]

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