[PATCH net 0/2] net/mlx5: command mailbox use-after-free, and the teardown hang hiding it

[PATCH net 0/2] net/mlx5: command mailbox use-after-free, and the teardown hang hiding it

[Danielle Costantino]

Two defects in the mlx5 command interface, both reached through the same
door: a firmware command that times out.

1/2 bounds the drain in mlx5_cmd_allowed_opcode() and
mlx5_cmd_change_mod().  Both take every unit of cmd->vars.sem, and since
commit 8e715cd613a1 ("net/mlx5: Set command entry semaphore up once got
index free") a unit is only returned when the entry's refcount reaches
zero, which for a timed-out entry never happens.  One stalled slot blocks
them forever, and destroy_async_eqs() calls both from mlx5_unload(), so a
function with any stalled command cannot be removed: the task stays in D
state holding the devlink instance lock, and reboot hits the same path.

2/2 stops the driver handing a firmware-owned DMA mailbox back to
dev->cmd.pool.  When mlx5_cmd_comp_handler(forced) keeps an entry and its
queue slot because firmware may still complete the command, cmd_exec() and
the callback path free the mailboxes anyway.  dmapool 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 and a
subsequent dma_pool_alloc() follows a poisoned pointer.  That is the crash.

1/2 comes first because it is what makes 2/2's teardown cleanup reachable:
without it teardown deadlocks long before mlx5_cmd_disable(), so nothing in
this series would ever run there.  They are otherwise independent - 1/2
does not touch anything 2/2 changes - and each builds on its own.

Testing
=======

A 32-slot aarch64 device with 22 mlx5 functions, debug kernel (KASAN,
DEBUG_OBJECTS, DEBUG_LIST, DMA_API_DEBUG, DEBUG_SPINLOCK), driving real
-ETIMEDOUT by swallowing firmware completions with a kprobe, then
unbinding the function.

  control, unbind/rebind with no stalled slots
      unbind 10s, rebind 3s, no warnings, no splats - the drain refactor
      leaves the normal path alone

  1/2, with 13 stalled slots
      before  unbind never returns; task in D state in
              mlx5_cmd_allowed_opcode() with cmd->vars.sem at 0
      after   the drain gives up three times, once per quiesce call in
              destroy_async_eqs(), at 61.4s intervals - the command
              timeout - then the unbind completes in 195s and the
              function rebinds in 2s
      the partial drain declines to narrow the allowed opcode, once,
      and still widens it back afterwards

  2/2, same 13 stalled slots
      mailbox ownership taken on 13 of 13 timed-out entries
      slots sharing one lay->out_ptr ............. 0   (7 and 6 before)
      firmware-owned mailbox on pool free list ... 0   (8 and 7 before)
      pool->next_block .......................... valid, was garbage
      entries retired ........................... 13, matching the slots
      "dma_pool_destroy mlx5_cmd busy" .......... 0   (1 without the
                                                      reclaim)
      kmemleak reports of mlx5_cmd_work_ent ..... 0   (14 without it)

  across every run
      no KASAN, refcount_t, list corruption or dma_pool warnings

Danielle Costantino (2):
  net/mlx5: Bound the command interface drain so teardown cannot hang
  net/mlx5: Don't return firmware-owned command mailboxes to the DMA
    pool

 drivers/net/ethernet/mellanox/mlx5/core/cmd.c | 231 +++++++++++++++---
 .../net/ethernet/mellanox/mlx5/core/main.c    |  21 +-
 .../ethernet/mellanox/mlx5/core/mlx5_core.h   |   2 +-
 include/linux/mlx5/driver.h                   |   6 +
 4 files changed, 228 insertions(+), 32 deletions(-)


base-commit: 83a945a529d6e002dd7339c532288a931f463dba

[PATCH net 1/2] net/mlx5: Bound the command interface drain so teardown cannot hang

[Danielle Costantino]

mlx5_cmd_allowed_opcode() and mlx5_cmd_change_mod() drain the command
interface before updating a field that every in flight command reads, by
taking every unit of cmd->vars.sem:

	for (i = 0; i < cmd->vars.max_reg_cmds; i++)
		down(&cmd->vars.sem);
	down(&cmd->vars.pages_sem);

Since commit 8e715cd613a1 ("net/mlx5: Set command entry semaphore up once
got index free") a unit is handed back from cmd_ent_put(), under the
refcount that reaches zero:

	if (ent->idx >= 0) {
		cmd_free_index(cmd, ent->idx);
		up(ent->page_queue ? &cmd->vars.pages_sem : &cmd->vars.sem);
	}

A command that timed out never gets there.  mlx5_cmd_comp_handler(forced)
deliberately keeps the entry and its index allocated because firmware may
still complete the command, so the entry keeps a reference, the refcount
never reaches zero, and the unit is never returned.  Before that change the
up() ran unconditionally at the end of the completion loop and a timed out
entry did give its unit back; only the index was withheld.

So a single stalled command slot makes both functions block forever.

That is not a corner case, because destroy_async_eqs() calls both while
tearing the device down:

	mlx5_cmd_allowed_opcode(dev, MLX5_CMD_OP_DESTROY_EQ);
	mlx5_cmd_use_polling(dev);		/* mlx5_cmd_change_mod() */
	cleanup_async_eq(dev, &table->cmd_eq, "cmd");
	mlx5_cmd_allowed_opcode(dev, CMD_ALLOWED_OPCODE_ALL);

and that runs from mlx5_eq_table_destroy() <- mlx5_unload(), so a function
with any stalled command cannot be removed:

  mlx5_cmd_allowed_opcode+0x70/0x188
  destroy_async_eqs+0x168/0x440
  mlx5_eq_table_destroy+0x29c/0x2e0
  mlx5_unload+0xa8/0xe8
  mlx5_uninit_one+0xa0/0x190
  remove_one+0x80/0x100
  pci_device_remove+0x9c/0x1c0
  device_release_driver_internal+0x358/0x5a8
  unbind_store+0x14c/0x188

The task stays in D state indefinitely and holds the devlink instance lock
while it does, so concurrent devlink users pile up behind it.  Reboot does
not recover it quickly either, since the same teardown runs on the way
down.

Reproduced by swallowing firmware completions with a kprobe so that
commands take the real -ETIMEDOUT path, then unbinding the function.  With
20 of the 31 register slots stalled, inspecting the hung device shows
cmd->vars.sem drained to 0 while cmd->vars.bitmask still reports 11 slots
free: mlx5_cmd_allowed_opcode() took the 11 units that were available and
then blocked on the 20 that are never coming back.

Bound the wait by the command timeout, which is the longest a command that
is merely in flight can legitimately take, and update the field without a
full drain if it expires.  Give the two callers a shared helper so the
unwind releases exactly what was acquired.  Failing to drain is worth a
warning but not a hang: the entries still holding units have already timed
out, so they are the least likely to be disturbed by the update, and the
alternative is an unrecoverable teardown.

mlx5_cmd_invoke() already bounds the same semaphore this way, see
commit 485d65e13571 ("net/mlx5: Add a timeout to acquire the command
queue semaphore").

Fixes: 8e715cd613a1 ("net/mlx5: Set command entry semaphore up once got index free")
Cc: stable@vger.kernel.org
Signed-off-by: Danielle Costantino <dcostantino@meta.com>
---
 drivers/net/ethernet/mellanox/mlx5/core/cmd.c | 77 +++++++++++++++----
 1 file changed, 61 insertions(+), 16 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/cmd.c b/drivers/net/ethernet/mellanox/mlx5/core/cmd.c
index 84583dc5eb1c0..571ed540957b1 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/cmd.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/cmd.c
@@ -1656,36 +1656,81 @@ static void create_debugfs_files(struct mlx5_core_dev *dev)
 	debugfs_create_file("run", 0200, dbg->dbg_root, dev, &fops);
 }
 
-void mlx5_cmd_allowed_opcode(struct mlx5_core_dev *dev, u16 opcode)
+/* Drain the command interface so that cmd->allowed_opcode and cmd->mode can be
+ * updated without an in flight command straddling the change.  A command that
+ * timed out keeps its index, and with it its semaphore unit, until firmware
+ * completes it - which may never happen - so bound the wait by the command
+ * timeout instead of blocking forever.  Returns the number of cmd->vars.sem
+ * units taken, and reports separately whether the page queue unit was taken;
+ * both have to be handed back by cmd_sem_up_all().
+ */
+static int cmd_sem_down_all(struct mlx5_core_dev *dev, bool *pages_sem)
 {
+	unsigned long end = jiffies + msecs_to_jiffies(mlx5_tout_ms(dev, CMD));
 	struct mlx5_cmd *cmd = &dev->cmd;
+	long left;
 	int i;
 
-	for (i = 0; i < cmd->vars.max_reg_cmds; i++)
-		down(&cmd->vars.sem);
-	down(&cmd->vars.pages_sem);
+	for (i = 0; i < cmd->vars.max_reg_cmds; i++) {
+		left = end - jiffies;
+		if (left <= 0 || down_timeout(&cmd->vars.sem, left))
+			break;
+	}
 
-	cmd->allowed_opcode = opcode;
+	left = end - jiffies;
+	*pages_sem = left > 0 && !down_timeout(&cmd->vars.pages_sem, left);
 
-	up(&cmd->vars.pages_sem);
-	for (i = 0; i < cmd->vars.max_reg_cmds; i++)
+	if (i < cmd->vars.max_reg_cmds)
+		mlx5_core_warn(dev, "command interface did not drain, %d of %d slots still busy\n",
+			       cmd->vars.max_reg_cmds - i, cmd->vars.max_reg_cmds);
+	if (!*pages_sem)
+		mlx5_core_warn(dev, "command interface did not drain, page queue slot still busy\n");
+
+	return i;
+}
+
+static void cmd_sem_up_all(struct mlx5_core_dev *dev, int nr, bool pages_sem)
+{
+	struct mlx5_cmd *cmd = &dev->cmd;
+
+	if (pages_sem)
+		up(&cmd->vars.pages_sem);
+	while (nr--)
 		up(&cmd->vars.sem);
 }
 
-static void mlx5_cmd_change_mod(struct mlx5_core_dev *dev, int mode)
+void mlx5_cmd_allowed_opcode(struct mlx5_core_dev *dev, u16 opcode)
 {
 	struct mlx5_cmd *cmd = &dev->cmd;
-	int i;
+	bool pages_sem;
+	int nr;
 
-	for (i = 0; i < cmd->vars.max_reg_cmds; i++)
-		down(&cmd->vars.sem);
-	down(&cmd->vars.pages_sem);
+	nr = cmd_sem_down_all(dev, &pages_sem);
 
-	cmd->mode = mode;
+	/* Narrowing the set is only safe once the interface has drained.
+	 * mlx5_cmd_comp_handler() reads !opcode_allowed() as "no real
+	 * firmware completion is expected" and releases the entry, so
+	 * narrowing while a command is still posted would hand its mailboxes
+	 * back to dev->cmd.pool with firmware still able to write them.
+	 * Widening back to CMD_ALLOWED_OPCODE_ALL is always safe.
+	 */
+	if (opcode == CMD_ALLOWED_OPCODE_ALL ||
+	    (nr == cmd->vars.max_reg_cmds && pages_sem))
+		cmd->allowed_opcode = opcode;
+	else
+		mlx5_core_warn(dev, "leaving command opcodes unrestricted, interface did not drain\n");
 
-	up(&cmd->vars.pages_sem);
-	for (i = 0; i < cmd->vars.max_reg_cmds; i++)
-		up(&cmd->vars.sem);
+	cmd_sem_up_all(dev, nr, pages_sem);
+}
+
+static void mlx5_cmd_change_mod(struct mlx5_core_dev *dev, int mode)
+{
+	bool pages_sem;
+	int nr;
+
+	nr = cmd_sem_down_all(dev, &pages_sem);
+	dev->cmd.mode = mode;
+	cmd_sem_up_all(dev, nr, pages_sem);
 }
 
 static int cmd_comp_notifier(struct notifier_block *nb,

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

[Danielle Costantino]

When a firmware command times out, mlx5_cmd_comp_handler(forced=true)
deliberately skips cmd_ent_put():

	if (!forced ||                 /* real FW completion */
	     mlx5_cmd_is_down(dev) ||  /* no FW completion expected */
	     !opcode_allowed(cmd, ent->op))
		cmd_ent_put(ent);

so the command entry and its command queue slot stay allocated - the
documented "Will cause a leak of a command resource" behavior, which exists
precisely because firmware may still complete the command and write to
ent->lay.

Neither free site honors that for the mailboxes.  cmd_exec() falls through
to out_out:/out_in: on any error from mlx5_cmd_invoke(), including
-ETIMEDOUT, and the callback branch of mlx5_cmd_comp_handler() frees
ent->out and ent->in unconditionally - in both cases while ent->lay->in_ptr
and ent->lay->out_ptr still point at those buffers, and while the entry is
deliberately being kept alive for exactly that reason.

dma_pool_free() is explicit about the contract it is given:

 * Caller promises neither device nor driver will again touch this
 * block unless it is first re-allocated.

and pool_block_push() stores the free list node (struct dma_block:
next_block, dma) in the first 16 bytes of the DMA coherent block itself.
For mlx5 that is block->data[0..15] of struct mlx5_cmd_prot_block - the
start of the command payload firmware writes.  A late firmware completion
therefore overwrites dma_pool's free list pointer, and a subsequent
dma_pool_alloc() from that pool dereferences it:

  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)   // pool_block_pop(), mm/dmapool.c:187
   mlx5_alloc_cmd_msg+0x154/0x318
   cmd_exec+0x24c/0xb28
   mlx5_cmd_do+0x34/0x70
   mlx5_access_reg+0xe8/0x1c8
   mlx5_health_log_ts_update+0x80/0xb0

Two crash dumps taken after a firmware stall show the aliasing directly.
On each, 31 of the 32 command slots held an entry with ret == -ETIMEDOUT,
and 28 of 31 (30 of 31 on the second) carried the same ent->lay->out_ptr,
while all 31 ent->lay->in_ptr were distinct - the outbox is always a fresh
dma_pool_zalloc() and the pool free list is LIFO, so the mailbox freed by
the command that just timed out is handed straight to the next one, whereas
inboxes come from cmd->cache[].  pool->next_block held a value that is not
a kernel address.  The driver also logged "Command completion arrived after
timeout" hundreds of times, i.e. firmware really did complete those
commands after their mailboxes had been released.

Transfer ownership of the mailboxes to the command entry instead, on both
the synchronous and the callback path, and release them on its final put.
For a real completion nothing moves in practice, because that put is
normally the last one.

A command firmware never completes would then keep its mailboxes for the
lifetime of the device, and dma_pool_destroy() checks pool wide and all or
nothing: one stranded block makes it skip dma_free_coherent() for every
page and then free the descriptors anyway, stranding the whole ~4 MB
command pool - and its IOVA range under an IOMMU - on every teardown that
had a stalled slot.  So retire those entries in mlx5_cmd_disable(), before
destroy_msg_cache() so a reclaimed inbox lands back on its cache list and
is freed there.

Return the mailboxes to the pool only once firmware has confirmed it is
finished with them.  A successful DISABLE_HCA is that confirmation, so
check its result, which mlx5_function_disable() until now discarded, and
pass it to mlx5_cmd_disable().  Five other paths reach mlx5_cmd_disable()
without having issued it at all, among them the mlx5_function_enable()
error path and the light init and unload paths; those pass false, the
mailboxes stay allocated, and dma_pool_destroy() leaves the pool's pages
mapped so a late write lands in memory the driver still owns.  There is no
hardware backstop to fall back on: remove_one() runs mlx5_uninit_one(), and
with it the whole teardown, before mlx5_pci_close(), so bus mastering is
still enabled here.  Note free_cmd_page() just below makes the opposite
choice for the command queue page, which firmware writes lay->status_own
into, and has since the driver was merged; this does not change that.

The entry itself is retired unconditionally, because firmware is never
given its address: the only addresses the driver hands the device are the
mailbox DMA addresses in lay->in_ptr and lay->out_ptr and the command queue
page in cmdq_addr_h/l, and ent->lay points into that page rather than into
the entry.

Retiring it means dropping the reference firmware owes it, and nothing
tracked that.  Add MLX5_CMD_ENT_STATE_FW_REF, minted in cmd_work_handler()
together with the cmd_ent_get() it stands for and published before
MLX5_CMD_ENT_STATE_PENDING_COMP.  Every completion handler that acts on an
entry first clears PENDING_COMP, so ordering it last guarantees such a
handler sees FW_REF already set - including mlx5_cmd_trigger_completions(),
which can walk the slot as soon as cmd_alloc_index() has published it and
does not wait for the doorbell.  All three places that may drop the
reference - the retention condition when it declines to keep the entry, a
late real completion, and the reclaim - consume it with
test_and_clear_bit(), so exactly one of them wins.

A separate marker is needed rather than reusing own_msgs: the callback path
sets own_msgs whenever it hands the mailboxes over, while the reference is
only retained conditionally, so own_msgs alone would over put an entry
whose reference the retention condition had already dropped.  Gating
own_msgs on FW_REF too stops the driver withholding mailboxes it has
already decided firmware is finished with.

As a side effect the existing put on the "Command completion arrived after
timeout" path becomes paired with the reference rather than unconditional,
which closes a pre-existing over put: a force-polling command can be
completed both by cmd_work_handler() and by the event queue, and the second
one used to put a reference it did not own.

cb_timeout_work runs on the system workqueue, which the
flush_workqueue(cmd->wq) at the top of mlx5_cmd_disable() does not cover,
so drain it before the firmware reference is dropped.

Every other error path is unchanged: -EBUSY, -EAGAIN, -ECANCELED and
-EALREADY all fail before the doorbell is rung, so returning their
mailboxes immediately is correct.

This is distinct from commit fbb9933666e3 ("net/mlx5: Abort new commands if
all command slots are stalled"), which stops new commands once every slot
is stalled.  That limits how long the driver spins, but it does not gate
the mailbox free, and it does not help a device that corrupts the pool
before its slots are exhausted - one of the affected machines crashed after
only 18 timeouts, with slots still free.

One related hole is left in place, pre-existing.  A command that was posted
and then force completed with -ENXIO - by mlx5_cmd_comp_handler() under
MLX5_TRIGGERED_CMD_COMP, or by poll_timeout() when the interface goes down
- has its mailboxes freed immediately.  Both run only once the device is on
its way to a reset, where the driver already assumes firmware is finished
with the command queue.

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 | 154 ++++++++++++++++--
 .../net/ethernet/mellanox/mlx5/core/main.c    |  21 ++-
 .../ethernet/mellanox/mlx5/core/mlx5_core.h   |   2 +-
 include/linux/mlx5/driver.h                   |   6 +
 4 files changed, 167 insertions(+), 16 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/cmd.c b/drivers/net/ethernet/mellanox/mlx5/core/cmd.c
index 571ed540957b1..890a2a2613ca0 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,15 @@ static void cmd_ent_put(struct mlx5_cmd_work_ent *ent)
 	}
 	spin_unlock_irqrestore(&cmd->alloc_lock, flags);
 
+	/* These were withheld from dev->cmd.pool because firmware still owned
+	 * them.  Nothing references the entry any more, so whoever dropped the
+	 * last reference has established that firmware is done with them.
+	 */
+	if (ent->own_msgs) {
+		mlx5_free_cmd_msg(dev, ent->out);
+		free_msg(dev, ent->in);
+	}
+
 	cmd_free_ent(ent);
 }
 
@@ -958,10 +974,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)
@@ -1059,9 +1071,19 @@ static void cmd_work_handler(struct work_struct *work)
 
 	if (ent->callback && schedule_delayed_work(&ent->cb_timeout_work, timeout))
 		cmd_ent_get(ent);
-	set_bit(MLX5_CMD_ENT_STATE_PENDING_COMP, &ent->state);
 
 	cmd_ent_get(ent); /* for the _real_ FW event on completion */
+	/* Publish the reference, then its marker, and only then
+	 * MLX5_CMD_ENT_STATE_PENDING_COMP.  Whoever clears PENDING_COMP owns
+	 * the completion, so it has to see FW_REF already set or it will fail
+	 * to drop the reference.  Ordering PENDING_COMP last is what
+	 * guarantees that: mlx5_cmd_trigger_completions() can walk this slot
+	 * as soon as cmd_alloc_index() published it, long before the doorbell.
+	 */
+	set_bit(MLX5_CMD_ENT_STATE_FW_REF, &ent->state);
+	/* order FW_REF before PENDING_COMP, see the comment above */
+	smp_mb__before_atomic();
+	set_bit(MLX5_CMD_ENT_STATE_PENDING_COMP, &ent->state);
 	/* Skip sending command to fw if internal error */
 	if (mlx5_cmd_is_down(dev) || !opcode_allowed(&dev->cmd, ent->op)) {
 		ent->ret = -ENXIO;
@@ -1313,7 +1335,19 @@ 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) {
+		/* The command was posted to firmware and firmware never
+		 * completed it.  Unless the device is already down,
+		 * mlx5_cmd_comp_handler(forced) keeps this entry and its
+		 * queue slot allocated because firmware may still complete
+		 * it, so ent->lay->{in_ptr,out_ptr} keep referencing the
+		 * mailboxes.  Take ownership of them so that cmd_exec()
+		 * cannot hand them back to dev->cmd.pool.
+		 */
+		ent->own_msgs = true;
+		goto out_free;
+	}
+	if (err == -ECANCELED || err == -EBUSY)
 		goto out_free;
 
 	ds = ent->ts2 - ent->ts1;
@@ -1810,7 +1844,9 @@ static void mlx5_cmd_comp_handler(struct mlx5_core_dev *dev, u64 vec, bool force
 				if (!forced) {
 					mlx5_core_err(dev, "Command completion arrived after timeout (entry idx = %d).\n",
 						      ent->idx);
-					cmd_ent_put(ent);
+					if (test_and_clear_bit(MLX5_CMD_ENT_STATE_FW_REF,
+							       &ent->state))
+						cmd_ent_put(ent);
 				}
 				continue;
 			}
@@ -1818,9 +1854,11 @@ static void mlx5_cmd_comp_handler(struct mlx5_core_dev *dev, u64 vec, bool force
 			if (ent->callback && cancel_delayed_work(&ent->cb_timeout_work))
 				cmd_ent_put(ent); /* timeout work was canceled */
 
-			if (!forced || /* Real FW completion */
+			if ((!forced || /* Real FW completion */
 			     mlx5_cmd_is_down(dev) || /* No real FW completion is expected */
-			     !opcode_allowed(cmd, ent->op))
+			     !opcode_allowed(cmd, ent->op)) &&
+			    test_and_clear_bit(MLX5_CMD_ENT_STATE_FW_REF,
+					       &ent->state))
 				cmd_ent_put(ent);
 
 			ent->ts2 = ktime_get_ns();
@@ -1861,8 +1899,22 @@ 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);
+				/* Same reasoning as the -ETIMEDOUT path in
+				 * mlx5_cmd_invoke(): when the entry was
+				 * retained just above, firmware may still DMA
+				 * into ent->lay->{in_ptr,out_ptr}, so hand the
+				 * mailboxes to the entry and let its last
+				 * cmd_ent_put() release them.  Otherwise the
+				 * driver has decided firmware is done and they
+				 * go straight back to the pool, as before.
+				 */
+				if (test_bit(MLX5_CMD_ENT_STATE_FW_REF,
+					     &ent->state)) {
+					ent->own_msgs = true;
+				} else {
+					mlx5_free_cmd_msg(dev, ent->out);
+					free_msg(dev, ent->in);
+				}
 
 				/* final consumer is done, release ent */
 				cmd_ent_put(ent);
@@ -2057,6 +2109,10 @@ static int cmd_exec(struct mlx5_core_dev *dev, void *in, int in_size, void *out,
 	if (callback && !err)
 		return 0;
 
+	/* The mailboxes are owned by the command entry now */
+	if (err == -ETIMEDOUT)
+		goto out_up;
+
 	if (err > 0) /* Failed in FW, command didn't execute */
 		err = deliv_status_to_err(err);
 
@@ -2590,12 +2646,86 @@ int mlx5_cmd_enable(struct mlx5_core_dev *dev)
 	return err;
 }
 
-void mlx5_cmd_disable(struct mlx5_core_dev *dev)
+/* Retire the entries that timed out and were never completed by firmware.
+ * Each still holds the reference a real completion would have dropped, and
+ * with it a command queue slot, a semaphore unit, its own allocation, and -
+ * since the timeout paths hand mailbox ownership to the entry - blocks of
+ * cmd->pool.  The mailboxes matter most: dma_pool_destroy() checks pool wide
+ * and all or nothing, so one stranded block makes it skip dma_free_coherent()
+ * for every page and then free the descriptors anyway, stranding the whole
+ * command pool - and its IOVA range under an IOMMU - on every teardown.
+ */
+static void cmd_reclaim_stalled_ents(struct mlx5_core_dev *dev, bool fw_stopped)
+{
+	struct mlx5_cmd_work_ent *ent;
+	struct mlx5_cmd *cmd = &dev->cmd;
+	unsigned long flags;
+	int i;
+
+	for (i = 0; i < (1 << cmd->vars.log_sz); i++) {
+		/* Take a reference before dropping alloc_lock, otherwise a
+		 * concurrent last put could free the entry under us.  Holding
+		 * alloc_lock with the bit clear guarantees the refcount has
+		 * not reached zero, because cmd_ent_put() sets that bit under
+		 * the same lock before it frees anything.
+		 */
+		spin_lock_irqsave(&cmd->alloc_lock, flags);
+		ent = test_bit(i, &cmd->vars.bitmask) ? NULL : cmd->ent_arr[i];
+		if (ent)
+			cmd_ent_get(ent);
+		spin_unlock_irqrestore(&cmd->alloc_lock, flags);
+
+		if (!ent)
+			continue;
+
+		/* cb_timeout_work sits on the system workqueue, which the
+		 * flush_workqueue(cmd->wq) above does not cover.  Left queued
+		 * it would run cb_timeout_handler() once the async EQs are
+		 * gone and the caller has destroyed the message cache and the
+		 * pool.  If it was still pending, drop the reference taken
+		 * when it was scheduled.
+		 */
+		if (ent->callback &&
+		    cancel_delayed_work_sync(&ent->cb_timeout_work))
+			cmd_ent_put(ent);
+
+		/* The entry itself is never visible to firmware - the only
+		 * addresses it is ever given are the mailbox and command
+		 * queue DMA addresses - so it can always be retired.  Its
+		 * mailboxes are a different matter: hand those back only once
+		 * firmware has acknowledged it released the function.  If it
+		 * has not, drop ownership without freeing.  dma_pool_destroy()
+		 * then reports the pool busy and skips dma_free_coherent(),
+		 * so the pages stay mapped and are never handed back to the
+		 * allocator; a late write lands there harmlessly.
+		 */
+		if (!fw_stopped)
+			ent->own_msgs = false;
+
+		/* Only drop the firmware reference if it is really owed;
+		 * test_and_clear_bit() makes this safe against a late real
+		 * completion that got there first.
+		 */
+		if (test_and_clear_bit(MLX5_CMD_ENT_STATE_FW_REF, &ent->state)) {
+			mlx5_core_warn(dev, "reclaiming outstanding cmd[%d]: %s(0x%x)\n",
+				       i, mlx5_command_str(ent->op), ent->op);
+			cmd_ent_put(ent);
+		}
+
+		cmd_ent_put(ent);
+	}
+}
+
+void mlx5_cmd_disable(struct mlx5_core_dev *dev, bool fw_stopped)
 {
 	struct mlx5_cmd *cmd = &dev->cmd;
 
 	flush_workqueue(cmd->wq);
 	clean_debug_files(dev);
+	/* Before destroy_msg_cache(): a reclaimed inbox goes back on its cache
+	 * list via free_msg(), and destroy_msg_cache() then frees it.
+	 */
+	cmd_reclaim_stalled_ents(dev, fw_stopped);
 	destroy_msg_cache(dev);
 	free_cmd_page(dev, cmd);
 	dma_pool_destroy(cmd->pool);
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/main.c b/drivers/net/ethernet/mellanox/mlx5/core/main.c
index 5f28d906c35b6..88f0a8089a0e1 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/main.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/main.c
@@ -1164,18 +1164,33 @@ static int mlx5_function_enable(struct mlx5_core_dev *dev, bool boot, u64 timeou
 	mlx5_core_disable_hca(dev, 0);
 err_cmd_cleanup:
 	mlx5_cmd_set_state(dev, MLX5_CMDIF_STATE_DOWN);
-	mlx5_cmd_disable(dev);
+	/* Nothing here has confirmed that firmware released the function */
+	mlx5_cmd_disable(dev, false);
 
 	return err;
 }
 
 static void mlx5_function_disable(struct mlx5_core_dev *dev, bool boot)
 {
+	bool fw_stopped;
+
 	mlx5_reclaim_startup_pages(dev);
 	mlx5_stop_health_poll(dev, boot);
-	mlx5_core_disable_hca(dev, 0);
+	/* A DISABLE_HCA that firmware really completed is it acknowledging
+	 * that it has released the function, and so that it is done with the
+	 * buffers the driver gave it.  mlx5_cmd_disable() needs to know,
+	 * because it can only hand command mailboxes back to the DMA pool
+	 * once that holds.
+	 *
+	 * A zero return is not sufficient on its own: mlx5_cmd_check() turns
+	 * the -ENXIO from an interface that is already down into success for
+	 * DISABLE_HCA, deliberately, so that reset flows proceed smoothly.
+	 * That is exactly the case where nothing was posted and firmware
+	 * acknowledged nothing, so require the interface to still be up.
+	 */
+	fw_stopped = !mlx5_core_disable_hca(dev, 0) && !mlx5_cmd_is_down(dev);
 	mlx5_cmd_set_state(dev, MLX5_CMDIF_STATE_DOWN);
-	mlx5_cmd_disable(dev);
+	mlx5_cmd_disable(dev, fw_stopped);
 }
 
 static int mlx5_function_open(struct mlx5_core_dev *dev)
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/mlx5_core.h b/drivers/net/ethernet/mellanox/mlx5/core/mlx5_core.h
index d6713a2ce6769..c736537ecd81f 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/mlx5_core.h
+++ b/drivers/net/ethernet/mellanox/mlx5/core/mlx5_core.h
@@ -223,7 +223,7 @@ int mlx5_query_module_num(struct mlx5_core_dev *dev, int *module_num);
 int mlx5_cmd_init(struct mlx5_core_dev *dev);
 void mlx5_cmd_cleanup(struct mlx5_core_dev *dev);
 int mlx5_cmd_enable(struct mlx5_core_dev *dev);
-void mlx5_cmd_disable(struct mlx5_core_dev *dev);
+void mlx5_cmd_disable(struct mlx5_core_dev *dev, bool fw_stopped);
 void mlx5_cmd_set_state(struct mlx5_core_dev *dev,
 			enum mlx5_cmdif_state cmdif_state);
 int mlx5_cmd_init_hca(struct mlx5_core_dev *dev, u32 *sw_owner_id);
diff --git a/include/linux/mlx5/driver.h b/include/linux/mlx5/driver.h
index 83d0a83bbfbca..4c3812dfcc61b 100644
--- a/include/linux/mlx5/driver.h
+++ b/include/linux/mlx5/driver.h
@@ -829,6 +829,11 @@ typedef void (*mlx5_cmd_cbk_t)(int status, void *context);
 enum {
 	MLX5_CMD_ENT_STATE_PENDING_COMP,
 	MLX5_CMD_ENT_STATE_TIMEDOUT,
+	/* Firmware still owes this entry the reference that a real completion
+	 * would drop.  Consume it with test_and_clear_bit() so that exactly
+	 * one of the possible consumers takes it.
+	 */
+	MLX5_CMD_ENT_STATE_FW_REF,
 };
 
 struct mlx5_cmd_work_ent {
@@ -855,6 +860,7 @@ struct mlx5_cmd_work_ent {
 	u64			ts2;
 	u16			op;
 	bool			polling;
+	bool			own_msgs;
 	/* Track the max comp handlers */
 	refcount_t              refcnt;
 };

Re: [PATCH net 0/2] net/mlx5: command mailbox use-after-free, and the teardown hang hiding it

[Leon Romanovsky]

On Tue, Sep 15, 2026 at 03:59:38PM -0700, Danielle Costantino wrote:
> Two defects in the mlx5 command interface, both reached through the same
> door: a firmware command that times out.

1. Health recovery should handle this. If it does not, the bug is there.
2. Please review the cover letter, code, and commit messages, and trim
Keep AI-generated prose to a bare minimum. The current series is
not reviewable in its current form.

Thanks

Re: [PATCH net 1/2] net/mlx5: Bound the command interface drain so teardown cannot hang

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 8 potential
issue(s) to consider:

Critical: 0 · High: 3 · Medium: 5 · Low: 0

- [High] mlx5_cmd_change_mod() writes dev->cmd.mode unconditionally
  (cmd.c:1732) even when cmd_sem_down_all() has just reported that the…
- [High] With the drain bounded, teardown now proceeds while firmware
  still owns command-interface DMA, and two frees on that path are not…
- [High] The partial-drain path lets a genuinely live command remain in
  flight when destroy_async_eqs() proceeds to destroy the cmd EQ, which…
- [Medium] Commit message contradicts the code (and the code's own
  comment) for mlx5_cmd_allowed_opcode().
- [Medium] mlx5_cmd_allowed_opcode() can now silently decline to apply a
  narrowing request (cmd.c:1717-1721) but still returns void, and…
- [Medium] In cmd_sem_down_all() the page-queue semaphore is never even
  attempted once the register-slot loop exhausts the shared deadline,…
- [Medium] cmd_sem_down_all() budgets a single command timeout for up to
  max_reg_cmds + 1 sequential semaphore acquisitions (one `end` computed…
- [Medium] mlx5_cmd_allowed_opcode() writes cmd->allowed_opcode =
  CMD_ALLOWED_OPCODE_ALL unconditionally on the failed-drain path, on…

--

net/mlx5: Bound the command interface drain so teardown cannot hang

This replaces the unbounded down() loops in mlx5_cmd_allowed_opcode() and
mlx5_cmd_change_mod() with a shared helper, cmd_sem_down_all(), that bounds
each acquisition by the command timeout and warns instead of blocking when
the interface does not drain.  A matching cmd_sem_up_all() releases only
what was acquired.

A few questions about the changelog and the new code below.

>From the changelog:

> Bound the wait by the command timeout, which is the longest a command that
> is merely in flight can legitimately take, and update the field without a
> full drain if it expires.

and

> Failing to drain is worth a warning but not a hang: the entries still
> holding units have already timed out, so they are the least likely to be
> disturbed by the update, and the alternative is an unrecoverable teardown.

[Severity: Medium]
Does the code match this description for mlx5_cmd_allowed_opcode()?

For any opcode other than CMD_ALLOWED_OPCODE_ALL the field is not updated
when the drain expires, only a warning is printed, so the narrowing
requested by create_async_eqs() (MLX5_CMD_OP_CREATE_EQ) and
destroy_async_eqs() (MLX5_CMD_OP_DESTROY_EQ) is dropped rather than
applied.

The in-code comment added by the same hunk argues the opposite safety model
from the changelog, namely that updating without a drain would hand
firmware-owned mailboxes back to dev->cmd.pool.

Meanwhile mlx5_cmd_change_mod() does write dev->cmd.mode unconditionally
after a failed drain, so the two callers behave differently on the timeout
path.

Could the changelog describe that asymmetry, and note that a
mlx5_cmd_allowed_opcode() call can now become a no-op?  This matters for
the stable backport this is tagged for.

> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/cmd.c b/drivers/net/ethernet/mellanox/mlx5/core/cmd.c
> index 84583dc5eb1c0..571ed540957b1 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/cmd.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/cmd.c
> @@ -1656,36 +1656,81 @@ static void create_debugfs_files(struct mlx5_core_dev *dev)
>  	debugfs_create_file("run", 0200, dbg->dbg_root, dev, &fops);
>  }
>  
> -void mlx5_cmd_allowed_opcode(struct mlx5_core_dev *dev, u16 opcode)
> +/* Drain the command interface so that cmd->allowed_opcode and cmd->mode can be
> + * updated without an in flight command straddling the change.  A command that
> + * timed out keeps its index, and with it its semaphore unit, until firmware
> + * completes it - which may never happen - so bound the wait by the command
> + * timeout instead of blocking forever.  Returns the number of cmd->vars.sem
> + * units taken, and reports separately whether the page queue unit was taken;
> + * both have to be handed back by cmd_sem_up_all().
> + */
> +static int cmd_sem_down_all(struct mlx5_core_dev *dev, bool *pages_sem)
>  {
> +	unsigned long end = jiffies + msecs_to_jiffies(mlx5_tout_ms(dev, CMD));

[Severity: Medium]
Is one command timeout enough budget for max_reg_cmds + 1 sequential
acquisitions?  A single deadline is computed here and shared by every
down_timeout() below.

mlx5_tout_ms(dev, CMD) is also the per-command bound used by
cmd_work_handler() and wait_func(), so a device with no stalled command at
all can fail this drain: a command posted shortly after the drain started
may legitimately still be in flight when the shared deadline expires.

Doesn't that make the degraded paths reachable in normal operation, and
contradict the changelog claim that "the entries still holding units have
already timed out"?  Would a per-acquisition deadline, or a budget scaled
by the slot count, bound the wait without failing on a busy but healthy
interface?

>  	struct mlx5_cmd *cmd = &dev->cmd;
> +	long left;
>  	int i;
>  
> -	for (i = 0; i < cmd->vars.max_reg_cmds; i++)
> -		down(&cmd->vars.sem);
> -	down(&cmd->vars.pages_sem);
> +	for (i = 0; i < cmd->vars.max_reg_cmds; i++) {
> +		left = end - jiffies;
> +		if (left <= 0 || down_timeout(&cmd->vars.sem, left))
> +			break;
> +	}

[Severity: High]
Can returning here without a full drain let a genuinely live command
coexist with destruction of the command EQ, and turn into a read of freed
DMA memory?

If such a command's waiter took the events branch in wait_func() (it read
cmd->mode before the flip), it expires and calls:

  wait_func_handle_exec_timeout()
    mlx5_cmd_eq_recover()
      mlx5_eq_async_int(&eq->irq_nb, ASYNC_EQ_RECOVER, NULL)
        next_eqe_sw(eq)          /* walks eq->frag_buf */

while the teardown thread runs:

  destroy_async_eqs()
    cleanup_async_eq(dev, &table->cmd_eq, "cmd")
      destroy_async_eq()
        destroy_unmap_eq()
          mlx5_frag_buf_free(dev, &eq->frag_buf);

mlx5_cmd_eq_recover() uses &dev->priv.eq_table->cmd_eq unconditionally and
serializes only on eq_async->lock, which destroy_unmap_eq() does not take.
The garbage read out of the freed ring is then dispatched through
atomic_notifier_call_chain(&eqt->nh[eqe->type], ...), so a bogus
MLX5_EVENT_TYPE_CMD could reach mlx5_cmd_comp_handler().

cb_timeout_handler() reaches mlx5_cmd_eq_recover() the same way for
asynchronous commands, and teardown does not flush that delayed work
before destroying the EQ.

Before this patch the unbounded drain guaranteed no command was in flight
past the update, so neither entry point could run concurrently with cmd EQ
destruction.  The follow-up patch in the series does not address this
either, since its cancel_delayed_work_sync() runs later, in
mlx5_cmd_disable(), after the EQs are gone.

>  
> -	cmd->allowed_opcode = opcode;
> +	left = end - jiffies;
> +	*pages_sem = left > 0 && !down_timeout(&cmd->vars.pages_sem, left);

[Severity: Medium]
Is this pages_sem attempt reachable at all after the loop above breaks?

down_timeout() only returns non-zero once it has consumed the whole
remaining timeout (___down_common() returns -ETIME after schedule_timeout()
has expired), so on the break path jiffies has already reached end and the
recomputed left is <= 0.

That makes *pages_sem unconditionally false whenever a register slot timed
out: cmd->vars.pages_sem is never touched even if the page queue slot was
completely idle, and mlx5_cmd_change_mod() then flips dev->cmd.mode with
the page queue not drained.

>  
> -	up(&cmd->vars.pages_sem);
> -	for (i = 0; i < cmd->vars.max_reg_cmds; i++)
> +	if (i < cmd->vars.max_reg_cmds)
> +		mlx5_core_warn(dev, "command interface did not drain, %d of %d slots still busy\n",
> +			       cmd->vars.max_reg_cmds - i, cmd->vars.max_reg_cmds);
> +	if (!*pages_sem)
> +		mlx5_core_warn(dev, "command interface did not drain, page queue slot still busy\n");

[Severity: Medium]
Following on from the above, does this second warning misattribute the
failure?  Given left is provably <= 0 after the loop breaks, "page queue
slot still busy" is printed whenever a register slot timed out, even
though pages_sem was never contended and never even sampled.

[Severity: High]
With the drain now warning and continuing, does teardown reach two frees
that are not gated on firmware being finished with the memory?

destroy_async_eqs() proceeds to cleanup_async_eq(dev, &table->cmd_eq),
and with the register slots stalled the DESTROY_EQ command fails
(-ETIMEDOUT, -EAGAIN from mlx5_cmd_all_stalled(), or -ENXIO), yet
destroy_unmap_eq() frees the buffer anyway:

  err = mlx5_cmd_destroy_eq(dev, eq->eqn);
  if (err)
          mlx5_core_warn(dev, "failed to destroy a previously created eq: eqn %d\n",
                         eq->eqn);

  mlx5_frag_buf_free(dev, &eq->frag_buf);

so firmware is left with a live EQ programmed at DMA addresses that have
just been released.

Teardown then continues into mlx5_cmd_disable() -> free_cmd_page(), which
dma_free_coherent()s cmd->cmd_alloc_buf.  That page still holds the
mlx5_cmd_layout of every retained (timed out) entry with status_own ==
CMD_OWNER_HW, and remove_one() has not yet disabled bus mastering.

Before this patch, teardown of a device with a stalled slot hung inside
mlx5_cmd_allowed_opcode(), so mlx5_unload() never reached these frees with
retained entries.

The follow-up patch in the series ("net/mlx5: Don't return firmware-owned
command mailboxes to the DMA pool") does close the mailbox, entry-leak and
cb_timeout_work parts of this via cmd_reclaim_stalled_ents(), but it does
not gate destroy_unmap_eq()'s frag_buf free and keeps free_cmd_page()
unconditional, so these two remain at the end of the series.

> +
> +	return i;
> +}
> +
> +static void cmd_sem_up_all(struct mlx5_core_dev *dev, int nr, bool pages_sem)
> +{
> +	struct mlx5_cmd *cmd = &dev->cmd;
> +
> +	if (pages_sem)
> +		up(&cmd->vars.pages_sem);
> +	while (nr--)
>  		up(&cmd->vars.sem);
>  }
>  
> -static void mlx5_cmd_change_mod(struct mlx5_core_dev *dev, int mode)
> +void mlx5_cmd_allowed_opcode(struct mlx5_core_dev *dev, u16 opcode)
>  {
>  	struct mlx5_cmd *cmd = &dev->cmd;
> -	int i;
> +	bool pages_sem;
> +	int nr;
>  
> -	for (i = 0; i < cmd->vars.max_reg_cmds; i++)
> -		down(&cmd->vars.sem);
> -	down(&cmd->vars.pages_sem);
> +	nr = cmd_sem_down_all(dev, &pages_sem);
>  
> -	cmd->mode = mode;
> +	/* Narrowing the set is only safe once the interface has drained.
> +	 * mlx5_cmd_comp_handler() reads !opcode_allowed() as "no real
> +	 * firmware completion is expected" and releases the entry, so
> +	 * narrowing while a command is still posted would hand its mailboxes
> +	 * back to dev->cmd.pool with firmware still able to write them.
> +	 * Widening back to CMD_ALLOWED_OPCODE_ALL is always safe.
> +	 */

[Severity: Medium]
Is widening really always safe?  opcode_allowed() is evaluated twice for
the same entry and both evaluations have to agree.

First in cmd_work_handler(), which takes a reference and then, because the
opcode is not allowed, completes the entry without ringing the doorbell:

  cmd_ent_get(ent); /* for the _real_ FW event on completion */
  /* Skip sending command to fw if internal error */
  if (mlx5_cmd_is_down(dev) || !opcode_allowed(&dev->cmd, ent->op)) {
          ent->ret = -ENXIO;
          mlx5_cmd_comp_handler(dev, 1ULL << ent->idx, true);
          return;
  }

and again in mlx5_cmd_comp_handler(), where dropping that reference is
conditional:

  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);

If the unconditional widening store below lands between the two, all three
terms are false and cmd_ent_put() is skipped for a command firmware never
saw.  The refcount then never reaches zero, so cmd_free_index() and
up(&cmd->vars.sem) never run and the slot, its semaphore unit, the
mlx5_cmd_work_ent and its mailboxes are lost.

Doesn't that recreate the never-returned-unit condition this commit exists
to fix, for the drain paths and mlx5_cmd_flush()?  Before this patch the
store required every sem unit while cmd_work_handler() holds one for this
entry, so the window did not exist.  The follow-up patch reclaims such
entries at mlx5_cmd_disable(), so the loss lasts until unload rather than
forever, but the race window itself stays open at the end of the series.

> +	if (opcode == CMD_ALLOWED_OPCODE_ALL ||
> +	    (nr == cmd->vars.max_reg_cmds && pages_sem))
> +		cmd->allowed_opcode = opcode;
> +	else
> +		mlx5_core_warn(dev, "leaving command opcodes unrestricted, interface did not drain\n");

[Severity: Medium]
Should the callers be able to see that the narrowing was declined?
mlx5_cmd_allowed_opcode() still returns void and neither in-tree call site
was updated.

create_async_eqs() and destroy_async_eqs() install the restriction
precisely so that, while the command EQ is being created or destroyed, only
MLX5_CMD_OP_CREATE_EQ / MLX5_CMD_OP_DESTROY_EQ can be posted.  That
restriction is what makes cmd_work_handler() and cmd_exec() short-circuit
every other opcode to -ENXIO via opcode_allowed().

After this hunk the invariant can quietly not hold, and destroy_async_eqs()
continues into mlx5_cmd_use_polling() and cleanup_async_eq(dev,
&table->cmd_eq) with unrelated commands still admitted to firmware.

>  
> -	up(&cmd->vars.pages_sem);
> -	for (i = 0; i < cmd->vars.max_reg_cmds; i++)
> -		up(&cmd->vars.sem);
> +	cmd_sem_up_all(dev, nr, pages_sem);
> +}
> +
> +static void mlx5_cmd_change_mod(struct mlx5_core_dev *dev, int mode)
> +{
> +	bool pages_sem;
> +	int nr;
> +
> +	nr = cmd_sem_down_all(dev, &pages_sem);
> +	dev->cmd.mode = mode;

[Severity: High]
Can writing dev->cmd.mode here after a partial drain reintroduce the
indefinite hang this patch sets out to remove?

The full drain was the only thing serializing cmd->mode against the two
reads that belong to one command's lifetime.  cmd_work_handler() latches it
before the doorbell:

  cmd_mode = cmd->mode;
  ...
  /* a command latched as CMD_MODE_EVENTS is never polled here */

and wait_func() re-reads the live value:

  if (cmd->mode == CMD_MODE_POLLING || ent->polling)
          wait_for_completion(&ent->done);

so an events-mode command whose waiter observes CMD_MODE_POLLING takes the
untimed wait_for_completion().

CPU0                                    CPU1
cmd_work_handler()
  cmd_mode = CMD_MODE_EVENTS
  ring doorbell
                                        destroy_async_eqs()
                                          mlx5_cmd_use_polling()
                                            mlx5_cmd_change_mod()
                                              partial drain
                                              dev->cmd.mode = CMD_MODE_POLLING
                                            mlx5_eq_notifier_unregister()
                                          cleanup_async_eq(cmd_eq)
wait_func()
  reads CMD_MODE_POLLING
  wait_for_completion(&ent->done)  /* nothing left to complete it */

The notifier is unregistered right after the store and the command EQ is
destroyed next, so no completion source remains and the submitter stays in
D state, which is the state the changelog is trying to eliminate.

Before this patch the store required every cmd->vars.sem unit, and
mlx5_cmd_invoke() holds the entry's reference (hence its unit) across all
of wait_func(), so no in-flight command could straddle the change.

On a related note, cmd->mode and cmd->allowed_opcode are now plain
unannotated loads and stores shared between process context, the cmd
workqueue and the EQ path with no lock at all on the timeout path.

The follow-up patch in the series adds cmd_reclaim_stalled_ents() in
mlx5_cmd_disable() but never complete()s ent->done, so this waiter still
hangs at the end of the series.

> +	cmd_sem_up_all(dev, nr, pages_sem);
>  }
>  
>  static int cmd_comp_notifier(struct notifier_block *nb,

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260915225941.554568-1-dcostantino%40meta.com

Re: [PATCH net 2/2] 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 10 potential
issue(s) to consider:

Critical: 1 · High: 3 (4) · Medium: 5 · Low: 1

- [Critical] net/mlx5: data race on own_msgs causes DMA pool UAF
- [High] cmd_reclaim_stalled_ents() handles a still-pending
  cb_timeout_work by cancelling it and dropping only the reference it…
- [High] cmd_reclaim_stalled_ents() assumes the reference it drops is the
  last one — cmd_ent_put()'s new comment states "Nothing references the…
- [High] mlx5_cmd_change_mod() now updates dev->cmd.mode unconditionally
  even when cmd_sem_down_all() failed to drain the interface (it returns…
- [Medium] The commit message states "Five other paths reach
  mlx5_cmd_disable() without having issued it at all, among them the…
- [Medium] When cmd_reclaim_stalled_ents() runs with fw_stopped == false
  it clears ent->own_msgs, so cmd_ent_put()'s new release block is…
- [Medium] mlx5_function_disable() promotes a single successful
  DISABLE_HCA (plus !mlx5_cmd_is_down()) into a global verdict that…
- [Medium] The new mailbox retention is gated on the same retention
  condition as the entry reference, which contains the…
- [Medium] The new comment on mlx5_function_enable()'s unwind states
  "Nothing here has confirmed that firmware released the function" and…
- [Low] mlx5_cmd_invoke() now transfers ownership of the caller-supplied
  in/out mailbox messages to the command entry when it returns…

Pre-existing issues:
- [High] mlx5_cmd_flush() spins forever when a command timed out before
  the device entered the error state.
- [High] The newly added cancel_delayed_work_sync(&ent->cb_timeout_work)
  in cmd_reclaim_stalled_ents() runs far too late in teardown to make…
- [High] MLX5_CMD_ENT_STATE_FW_REF prevents a duplicate refcount
  decrement but does not keep the entry alive for the losing completion…
- [High] On the path where the patch deliberately withholds the mailboxes
  because firmware may still DMA (cmd_reclaim_stalled_ents(dev, false),…

--

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

This transfers ownership of the in/out command mailboxes to the command
entry when a command times out, so they are released only on the entry's
final put, and adds cmd_reclaim_stalled_ents() plus a new fw_stopped
argument to mlx5_cmd_disable() to retire entries firmware never completed.

A few questions below, first on the commit message and then inline.

> Return the mailboxes to the pool only once firmware has confirmed it is
> finished with them.  A successful DISABLE_HCA is that confirmation, so
> check its result, which mlx5_function_disable() until now discarded, and
> pass it to mlx5_cmd_disable().  Five other paths reach mlx5_cmd_disable()
> without having issued it at all, among them the mlx5_function_enable()
> error path and the light init and unload paths; those pass false, the
> mailboxes stay allocated, and dma_pool_destroy() leaves the pool's pages
> mapped so a late write lands in memory the driver still owns.

[Severity: Medium]
Is this description of the call sites accurate?  After the patch there are
only two callers of mlx5_cmd_disable(), both in main.c: the err_cmd_cleanup
label of mlx5_function_enable(), which passes false, and
mlx5_function_disable(), which passes the computed fw_stopped.

The paths named as conservative here - mlx5_init_one_light()'s
query_hca_caps_err unwind, mlx5_uninit_one_light(), mlx5_unload_one_light(),
and mlx5_function_setup()/mlx5_function_teardown() - all reach
mlx5_cmd_disable() through mlx5_function_disable(), which does issue
DISABLE_HCA and can therefore pass fw_stopped == true and hand the stalled
commands' mailboxes back to the pool.

Note also that the false in mlx5_unload_one_light():

	mlx5_function_disable(dev, false);

is mlx5_function_disable()'s boot argument, not fw_stopped.  Could the
paragraph be reworded so a backporter reading it does not conclude those
paths keep the mailboxes?

> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/cmd.c b/drivers/net/ethernet/mellanox/mlx5/core/cmd.c
> index 571ed540957b1..890a2a2613ca0 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/cmd.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/cmd.c

[ ... ]

> @@ -207,6 +214,15 @@ static void cmd_ent_put(struct mlx5_cmd_work_ent *ent)
>  	}
>  	spin_unlock_irqrestore(&cmd->alloc_lock, flags);
>  
> +	/* These were withheld from dev->cmd.pool because firmware still owned
> +	 * them.  Nothing references the entry any more, so whoever dropped the
> +	 * last reference has established that firmware is done with them.
> +	 */
> +	if (ent->own_msgs) {
> +		mlx5_free_cmd_msg(dev, ent->out);
> +		free_msg(dev, ent->in);
> +	}
> +
>  	cmd_free_ent(ent);
>  }

[Severity: High]
Can this release block run after the pool has already been destroyed?

cmd_reclaim_stalled_ents() only consumes MLX5_CMD_ENT_STATE_FW_REF plus its
own temporary reference, so it cannot know it dropped the last one.
mlx5_cmd_invoke() holds the cmd_alloc_ent() reference until its out_free:
put, and now sets ent->own_msgs = true before that put:

	if (err == -ETIMEDOUT) {
		...
		ent->own_msgs = true;
		goto out_free;
	}
	...
out_free:
	status = ent->status;
	cmd_ent_put(ent);

flush_workqueue(cmd->wq) at the top of mlx5_cmd_disable() waits for
cmd_work_handler(), not for that invoker, so:

thread A (mlx5_cmd_invoke)		thread B (mlx5_cmd_disable)
  wait_func() returns -ETIMEDOUT
  ent->own_msgs = true
					  cmd_reclaim_stalled_ents()
					    clears FW_REF, puts (refcnt >= 1)
					  destroy_msg_cache()
					  free_cmd_page()
					  dma_pool_destroy(cmd->pool)
  cmd_ent_put()  /* final */
    free_msg(dev, ent->in)
    mlx5_free_cmd_msg(dev, ent->out)
      free_cmd_box() ->
        dma_pool_free(dev->cmd.pool, ...)

dma_pool_destroy() ends with kfree(pool), so does that dma_pool_free() touch
a freed struct dma_pool - or, if mlx5_cmd_enable() has already installed a
new pool, return old-pool blocks into the new one?  free_msg() would also
push the inbox onto a cmd->cache[] list that destroy_msg_cache() already
drained and create_msg_cache() re-initialises.

On the same resumption, mlx5_cmd_comp_handler() does
memcpy(ent->out->first.data, ent->lay->out, ...) with ent->lay pointing into
the page free_cmd_page() just released.

> @@ -1059,9 +1071,19 @@ static void cmd_work_handler(struct work_struct *work)
>  
>  	if (ent->callback && schedule_delayed_work(&ent->cb_timeout_work, timeout))
>  		cmd_ent_get(ent);
> -	set_bit(MLX5_CMD_ENT_STATE_PENDING_COMP, &ent->state);
>  
>  	cmd_ent_get(ent); /* for the _real_ FW event on completion */
> +	/* Publish the reference, then its marker, and only then
> +	 * MLX5_CMD_ENT_STATE_PENDING_COMP.  Whoever clears PENDING_COMP owns
> +	 * the completion, so it has to see FW_REF already set or it will fail
> +	 * to drop the reference.  Ordering PENDING_COMP last is what
> +	 * guarantees that: mlx5_cmd_trigger_completions() can walk this slot
> +	 * as soon as cmd_alloc_index() published it, long before the doorbell.
> +	 */
> +	set_bit(MLX5_CMD_ENT_STATE_FW_REF, &ent->state);
> +	/* order FW_REF before PENDING_COMP, see the comment above */
> +	smp_mb__before_atomic();
> +	set_bit(MLX5_CMD_ENT_STATE_PENDING_COMP, &ent->state);

[Severity: High]
Slightly off to the side of this hunk, but in the same function: can the
mode snapshot taken here straddle a mode change and leave the caller waiting
forever?

cmd_work_handler() snapshots the mode when it posts:

	cmd_mode = cmd->mode;

while wait_func() reads the current global value afterwards:

	if (cmd->mode == CMD_MODE_POLLING || ent->polling)
		wait_for_completion(&ent->done);

With the bounded drain added by the earlier patch in this series,
"net/mlx5: Bound the command interface drain so teardown cannot hang",
mlx5_cmd_change_mod() now updates dev->cmd.mode even when cmd_sem_down_all()
timed out with units outstanding:

	nr = cmd_sem_down_all(dev, &pages_sem);
	dev->cmd.mode = mode;
	cmd_sem_up_all(dev, nr, pages_sem);

If the worker snapshotted CMD_MODE_EVENTS (so it does not poll) and the mode
flips to CMD_MODE_POLLING before the descheduled synchronous caller reads
it, the caller takes the unbounded wait_for_completion(&ent->done) branch
while nobody polls, and mlx5_cmd_use_polling() then unregisters the command
EQ notifier and destroy_async_eqs() destroys the EQ.

The sibling mlx5_cmd_allowed_opcode() got an explicit guard for the
incomplete-drain case:

	if (opcode == CMD_ALLOWED_OPCODE_ALL ||
	    (nr == cmd->vars.max_reg_cmds && pages_sem))

Should mlx5_cmd_change_mod() have the same guard, given this series is
otherwise about making teardown recoverable?

> @@ -1313,7 +1335,19 @@ 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) {
> +		/* The command was posted to firmware and firmware never
> +		 * completed it.  Unless the device is already down,
> +		 * mlx5_cmd_comp_handler(forced) keeps this entry and its
> +		 * queue slot allocated because firmware may still complete
> +		 * it, so ent->lay->{in_ptr,out_ptr} keep referencing the
> +		 * mailboxes.  Take ownership of them so that cmd_exec()
> +		 * cannot hand them back to dev->cmd.pool.
> +		 */
> +		ent->own_msgs = true;
> +		goto out_free;
> +	}

[Severity: Critical]
What serialises ent->own_msgs between the invoker and the reclaim?

Both sides write it as a plain bool with no lock and no ordering
against the refcount:

  - mlx5_cmd_invoke() sets ent->own_msgs = true here, after
    wait_func() has returned -ETIMEDOUT, and only then drops the
    cmd_alloc_ent() reference at out_free:.
  - cmd_reclaim_stalled_ents() clears ent->own_msgs when
    !fw_stopped, deliberately, so that nothing is handed back to
    dev->cmd.pool.

flush_workqueue(cmd->wq) at the top of mlx5_cmd_disable() waits for
cmd_work_handler(), not for the synchronous issuer sitting in
mlx5_cmd_invoke(), so the two can interleave the other way round:

thread A (mlx5_cmd_invoke)		thread B (mlx5_cmd_disable)
  wait_func() returns -ETIMEDOUT
					  cmd_reclaim_stalled_ents()
					    ent->own_msgs = false
					    clears FW_REF, puts
					  destroy_msg_cache()
					  dma_pool_destroy(cmd->pool)
  ent->own_msgs = true
  cmd_ent_put()  /* final */
    mlx5_free_cmd_msg(dev, ent->out)
    free_msg(dev, ent->in)

The reclaim's put is not the final one here - thread A still holds the
allocation reference - so the decision the reclaim made is simply
overwritten, and thread A's put becomes the last one and runs the new
release block in cmd_ent_put().  That reaches free_cmd_box() ->
dma_pool_free() on a struct dma_pool that dma_pool_destroy() has
already kfree()d, or, if mlx5_cmd_enable() has meanwhile installed a
fresh pool, returns old-pool blocks into the new one.  free_msg() has
the matching problem for a cache-parented inbox: it pushes onto a
cmd->cache[] list that destroy_msg_cache() already drained and
create_msg_cache() re-initialised.

The same unordered write also defeats the mitigation in the other
direction: on a !fw_stopped teardown the reclaim clears own_msgs
precisely so a late firmware write lands in memory the driver still
owns, and this assignment silently puts the mailboxes back on the
free list where dma_pool_destroy() will dma_free_coherent() them.

Would it be cleaner to make ownership follow the same single-consumer
rule as the reference - i.e. set own_msgs only under cmd->alloc_lock,
or only in the path that wins the test_and_clear_bit() on
MLX5_CMD_ENT_STATE_FW_REF - so that exactly one side decides?  Or
does the reclaim need to wait for outstanding synchronous issuers
before mlx5_cmd_disable() tears the pool down at all?

[Severity: Low]
Would it be worth stating the new ownership rule in the comment block above
mlx5_cmd_invoke()?  That block enumerates the meaning of every return value:

 * return value in case (!callback):
 *	ret < 0 : Command execution couldn't be submitted by driver
 *	ret > 0 : Command execution couldn't be performed by firmware
 *	ret == 0: Command was executed by FW, Caller must check FW outbox status.

but says nothing about who owns in/out afterwards, and -ETIMEDOUT is also
the case where err < 0 no longer means "couldn't be submitted".  A second
caller written against that block would dma_pool_free() mailboxes the entry
still owns and whose final put frees again.

> @@ -1810,7 +1844,9 @@ static void mlx5_cmd_comp_handler(struct mlx5_core_dev *dev, u64 vec, bool force
>  				if (!forced) {
>  					mlx5_core_err(dev, "Command completion arrived after timeout (entry idx = %d).\n",
>  						      ent->idx);
> -					cmd_ent_put(ent);
> +					if (test_and_clear_bit(MLX5_CMD_ENT_STATE_FW_REF,
> +							       &ent->state))
> +						cmd_ent_put(ent);
>  				}
>  				continue;
>  			}

[Severity: High]
This is a pre-existing issue, not introduced by the patch, but does the new
marker fully close the double-completion the commit message describes?

Both executors start from a bare lookup with no reference:

	if (test_bit(i, &vector)) {
		ent = cmd->ent_arr[i];

The winner clears PENDING_COMP, consumes FW_REF, puts, and completes
ent->done; the synchronous caller then drops the allocation reference in
mlx5_cmd_invoke()'s out_free:, so cmd_ent_put() runs cmd_free_index() (which
NULLs cmd->ent_arr[idx]) and kfree()s the entry.

The loser then runs clear_bit(MLX5_CMD_ENT_STATE_TIMEDOUT, &ent->state),
test_and_clear_bit(MLX5_CMD_ENT_STATE_PENDING_COMP, &ent->state) and the new
test_and_clear_bit(MLX5_CMD_ENT_STATE_FW_REF, &ent->state) on that freed
entry, or on NULL if it read ent_arr[i] later.  The atomic marker protects
the refcount but not the object holding it - would the completion executors
need the reference-protected lookup mlx5_cmd_trigger_completions() already
uses?

	for_each_set_bit(i, &bitmask, (1 << cmd->vars.log_sz))
		cmd_ent_get(cmd->ent_arr[i]);

[Severity: High]
This isn't introduced by this patch either, but it is the same class of hang
the series is addressing, so worth raising here: mlx5_cmd_flush() can spin
forever on a slot that timed out while the device was still up.

Such an entry keeps its reference (the retention condition below is false
because mlx5_cmd_is_down() is false and the opcode is allowed), so
cmd_ent_put() never reaches cmd_free_index()/up(&cmd->vars.sem) and the
semaphore unit is gone for good.  enter_error_state() then does:

	dev->state = MLX5_DEVICE_STATE_INTERNAL_ERROR;
	mlx5_cmd_flush(dev);

and mlx5_cmd_flush() loops:

	while (down_trylock(&cmd->vars.sem)) {
		mlx5_cmd_trigger_completions(dev);
		cond_resched();
	}

For the timed-out slot, the forced completion finds PENDING_COMP already
clear and just continues at the hunk above, so the unit is never released
and the loop has no bound.  The first patch in the series bounded
mlx5_cmd_allowed_opcode() and mlx5_cmd_change_mod(); should mlx5_cmd_flush()
be bounded the same way?

> @@ -1818,9 +1854,11 @@ static void mlx5_cmd_comp_handler(struct mlx5_core_dev *dev, u64 vec, bool force
>  			if (ent->callback && cancel_delayed_work(&ent->cb_timeout_work))
>  				cmd_ent_put(ent); /* timeout work was canceled */
>  
> -			if (!forced || /* Real FW completion */
> +			if ((!forced || /* Real FW completion */
>  			     mlx5_cmd_is_down(dev) || /* No real FW completion is expected */
> -			     !opcode_allowed(cmd, ent->op))
> +			     !opcode_allowed(cmd, ent->op)) &&
> +			    test_and_clear_bit(MLX5_CMD_ENT_STATE_FW_REF,
> +					       &ent->state))
>  				cmd_ent_put(ent);

[Severity: Medium]
The mailbox retention is now gated on this same condition, which includes
mlx5_cmd_is_down(dev) - pure software state (INTERNAL_ERROR set by the health
poller, MLX5_CMDIF_STATE_DOWN set just before mlx5_cmd_disable(), or
pci_channel_offline()).

For a command that was posted and force-completed with ent->ret ==
-ETIMEDOUT while that is true, FW_REF is consumed here, so on the callback
path the else branch below frees both mailboxes immediately, and on the
synchronous path ent->own_msgs = true gives no protection because
mlx5_cmd_invoke()'s put becomes the final one and frees them straight away.
With the pool then clean, dma_pool_destroy() does dma_free_coherent() those
pages back to the page allocator with bus mastering still on.

The commit message covers only the -ENXIO variants:

> One related hole is left in place, pre-existing.  A command that was posted
> and then force completed with -ENXIO [...]

Does the -ETIMEDOUT plus mlx5_cmd_is_down() case take the same free path,
and if so should it be gated too or at least listed alongside the -ENXIO
holes?

> @@ -1861,8 +1899,22 @@ 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);
> +				/* Same reasoning as the -ETIMEDOUT path in
> +				 * mlx5_cmd_invoke(): when the entry was
> +				 * retained just above, firmware may still DMA
> +				 * into ent->lay->{in_ptr,out_ptr}, so hand the
> +				 * mailboxes to the entry and let its last
> +				 * cmd_ent_put() release them.  Otherwise the
> +				 * driver has decided firmware is done and they
> +				 * go straight back to the pool, as before.
> +				 */
> +				if (test_bit(MLX5_CMD_ENT_STATE_FW_REF,
> +					     &ent->state)) {
> +					ent->own_msgs = true;
> +				} else {
> +					mlx5_free_cmd_msg(dev, ent->out);
> +					free_msg(dev, ent->in);
> +				}
>  
>  				/* final consumer is done, release ent */
>  				cmd_ent_put(ent);

[ ... ]

> @@ -2057,6 +2109,10 @@ static int cmd_exec(struct mlx5_core_dev *dev, void *in, int in_size, void *out,
>  	if (callback && !err)
>  		return 0;
>  
> +	/* The mailboxes are owned by the command entry now */
> +	if (err == -ETIMEDOUT)
> +		goto out_up;
> +
>  	if (err > 0) /* Failed in FW, command didn't execute */
>  		err = deliv_status_to_err(err);

> @@ -2590,12 +2646,86 @@ int mlx5_cmd_enable(struct mlx5_core_dev *dev)
>  	return err;
>  }
>  
> -void mlx5_cmd_disable(struct mlx5_core_dev *dev)
> +/* Retire the entries that timed out and were never completed by firmware.
> + * Each still holds the reference a real completion would have dropped, and
> + * with it a command queue slot, a semaphore unit, its own allocation, and -
> + * since the timeout paths hand mailbox ownership to the entry - blocks of
> + * cmd->pool.  The mailboxes matter most: dma_pool_destroy() checks pool wide
> + * and all or nothing, so one stranded block makes it skip dma_free_coherent()
> + * for every page and then free the descriptors anyway, stranding the whole
> + * command pool - and its IOVA range under an IOMMU - on every teardown.
> + */
> +static void cmd_reclaim_stalled_ents(struct mlx5_core_dev *dev, bool fw_stopped)
> +{
> +	struct mlx5_cmd_work_ent *ent;
> +	struct mlx5_cmd *cmd = &dev->cmd;
> +	unsigned long flags;
> +	int i;
> +
> +	for (i = 0; i < (1 << cmd->vars.log_sz); i++) {
> +		/* Take a reference before dropping alloc_lock, otherwise a
> +		 * concurrent last put could free the entry under us.  Holding
> +		 * alloc_lock with the bit clear guarantees the refcount has
> +		 * not reached zero, because cmd_ent_put() sets that bit under
> +		 * the same lock before it frees anything.
> +		 */
> +		spin_lock_irqsave(&cmd->alloc_lock, flags);
> +		ent = test_bit(i, &cmd->vars.bitmask) ? NULL : cmd->ent_arr[i];
> +		if (ent)
> +			cmd_ent_get(ent);
> +		spin_unlock_irqrestore(&cmd->alloc_lock, flags);
> +
> +		if (!ent)
> +			continue;
> +
> +		/* cb_timeout_work sits on the system workqueue, which the
> +		 * flush_workqueue(cmd->wq) above does not cover.  Left queued
> +		 * it would run cb_timeout_handler() once the async EQs are
> +		 * gone and the caller has destroyed the message cache and the
> +		 * pool.  If it was still pending, drop the reference taken
> +		 * when it was scheduled.
> +		 */
> +		if (ent->callback &&
> +		    cancel_delayed_work_sync(&ent->cb_timeout_work))
> +			cmd_ent_put(ent);

[Severity: High]
Can this leave an async entry stranded with nobody left to complete it?

cancel_delayed_work_sync() returning true means the work was still queued,
which means the entry has not been completed: PENDING_COMP is still set, the
callback has not run, and ent->in/ent->out are still allocated.

Reference accounting for such an entry: cmd_alloc_ent() = 1,
schedule_delayed_work() = 2, the firmware-event get = 3.  Here the reclaim
adds one (4), the cancel puts (3), the FW_REF test_and_clear puts (2), the
trailing put (1).  The remaining one is the allocation reference, which for
callback commands is dropped only by:

				/* final consumer is done, release ent */
				cmd_ent_put(ent);
				callback(err, context);

inside mlx5_cmd_comp_handler().  By this point mlx5_unload() has already run
mlx5_eq_table_destroy() -> destroy_async_eqs(), so no completion can arrive,
and the work that would have force-completed it:

	ent->ret = -ETIMEDOUT;
	...
	mlx5_cmd_comp_handler(dev, 1ULL << ent->idx, true);

has just been cancelled.  So the callback is never invoked (a later
mlx5_cmd_cleanup_async_ctx() then waits forever in
wait_for_completion(&ctx->inflight_done)), the entry is never freed,
cmd_free_index() never runs, and because own_msgs is false for a
never-completed entry its mailboxes are never freed by anyone - leaving
pool->nr_active non-zero at the dma_pool_destroy() below and stranding the
pool even when fw_stopped is true.  Would force-completing the entry here,
as cb_timeout_handler() does, be the safer shape?

[Severity: High]
This isn't a bug introduced by this patch - cb_timeout_work was not
cancelled at teardown at all before it - but does the cancel land late
enough that it cannot prevent the use-after-free it is aimed at?

cb_timeout_handler() calls mlx5_cmd_eq_recover(dev) unconditionally, before
it checks PENDING_COMP, and mlx5_cmd_eq_recover() does:

	struct mlx5_eq_async *eq = &dev->priv.eq_table->cmd_eq;

	eqes = mlx5_eq_async_int(&eq->irq_nb, ASYNC_EQ_RECOVER, NULL);

The teardown order is mlx5_uninit_one() -> mlx5_unload() ->
mlx5_eq_table_destroy() -> destroy_async_eqs() (command EQ destroyed), then
mlx5_cleanup_once() -> mlx5_eq_table_cleanup() (kvfree of the table), then
mlx5_function_teardown() -> mlx5_function_disable() -> mlx5_cmd_disable() ->
cmd_reclaim_stalled_ents().

A cb_timeout_work firing anywhere in that window locks eq->lock and reads
EQEs out of freed memory, and the new synchronous cancel only waits for that
handler.  Should the cancellation happen before mlx5_eq_table_destroy() /
mlx5_eq_table_cleanup() rather than before dma_pool_destroy()?

> +
> +		/* The entry itself is never visible to firmware - the only
> +		 * addresses it is ever given are the mailbox and command
> +		 * queue DMA addresses - so it can always be retired.  Its
> +		 * mailboxes are a different matter: hand those back only once
> +		 * firmware has acknowledged it released the function.  If it
> +		 * has not, drop ownership without freeing.  dma_pool_destroy()
> +		 * then reports the pool busy and skips dma_free_coherent(),
> +		 * so the pages stay mapped and are never handed back to the
> +		 * allocator; a late write lands there harmlessly.
> +		 */
> +		if (!fw_stopped)
> +			ent->own_msgs = false;

[Severity: Medium]
Does clearing own_msgs here leak more than the DMA blocks, and is the leak
bounded?

cmd_ent_put() gates all mailbox cleanup on own_msgs, so with it cleared the
kmalloc'd struct mlx5_cmd_msg and struct mlx5_cmd_mailbox wrappers - which
firmware never sees, only mailbox->dma - lose their last pointer, and a
cache-parented inbox never gets back onto cmd->cache[] for
destroy_msg_cache() either.

dma_pool_destroy() is then all or nothing:

	if (pool->nr_active) {
		dev_err(pool->dev, "%s %s busy\n", __func__, pool->name);
		busy = true;
	}

	list_for_each_entry_safe(page, tmp, &pool->page_list, page_list) {
		if (!busy)
			dma_free_coherent(...);

so every page stays allocated while the descriptors and the pool are freed
anyway, and mlx5_cmd_enable() creates a fresh pool on the next load.  Since
mlx5_cmd_is_down() is true on health-recovery teardowns, fw_stopped == false
looks like the common case there - does that make this repeat once per
recovery or devlink reload with nothing bounding it?  Would freeing at least
the host-side descriptors be safe, given firmware only ever sees
mailbox->dma?

> +
> +		/* Only drop the firmware reference if it is really owed;
> +		 * test_and_clear_bit() makes this safe against a late real
> +		 * completion that got there first.
> +		 */
> +		if (test_and_clear_bit(MLX5_CMD_ENT_STATE_FW_REF, &ent->state)) {
> +			mlx5_core_warn(dev, "reclaiming outstanding cmd[%d]: %s(0x%x)\n",
> +				       i, mlx5_command_str(ent->op), ent->op);
> +			cmd_ent_put(ent);
> +		}
> +
> +		cmd_ent_put(ent);
> +	}
> +}
> +
> +void mlx5_cmd_disable(struct mlx5_core_dev *dev, bool fw_stopped)
>  {
>  	struct mlx5_cmd *cmd = &dev->cmd;
>  
>  	flush_workqueue(cmd->wq);
>  	clean_debug_files(dev);
> +	/* Before destroy_msg_cache(): a reclaimed inbox goes back on its cache
> +	 * list via free_msg(), and destroy_msg_cache() then frees it.
> +	 */
> +	cmd_reclaim_stalled_ents(dev, fw_stopped);
>  	destroy_msg_cache(dev);
>  	free_cmd_page(dev, cmd);
>  	dma_pool_destroy(cmd->pool);

[Severity: High]
This is a pre-existing choice that the commit message explicitly declines to
change:

> Note free_cmd_page() just below makes the opposite
> choice for the command queue page, which firmware writes lay->status_own
> into, and has since the driver was merged; this does not change that.

but does it leave the new mitigation inconsistent within one function?  On
the fw_stopped == false path the mailboxes are deliberately withheld (at the
cost of the whole pool), and two lines later free_cmd_page() does:

	dma_free_coherent(mlx5_core_dma_dev(dev), cmd->alloc_size, cmd->cmd_alloc_buf,
			  cmd->alloc_dma);

on the page firmware was given via cmdq_addr_h/l and writes lay->status_own
and the inline lay->out into, for the very same stalled slots.  For commands
whose output fits inline there is no out mailbox at all, so nothing is
withheld for them.  remove_one() runs mlx5_uninit_one() before
mlx5_pci_close(), so bus mastering is still on.

Would deferring or skipping free_cmd_page() on the !fw_stopped paths (or
gating both releases on the per-slot CMD_OWNER_HW bit) make the two
decisions agree?

> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/main.c b/drivers/net/ethernet/mellanox/mlx5/core/main.c
> index 5f28d906c35b6..88f0a8089a0e1 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/main.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/main.c
> @@ -1164,18 +1164,33 @@ static int mlx5_function_enable(struct mlx5_core_dev *dev, bool boot, u64 timeou
>  	mlx5_core_disable_hca(dev, 0);
>  err_cmd_cleanup:
>  	mlx5_cmd_set_state(dev, MLX5_CMDIF_STATE_DOWN);
> -	mlx5_cmd_disable(dev);
> +	/* Nothing here has confirmed that firmware released the function */
> +	mlx5_cmd_disable(dev, false);

[Severity: Medium]
Is this comment consistent with the code just above it?  The unwind is a
fall-through ladder:

reclaim_boot_pages:
	mlx5_reclaim_startup_pages(dev);
stop_health_poll:
	mlx5_stop_health_poll(dev, boot);
	mlx5_core_disable_hca(dev, 0);
err_cmd_cleanup:
	mlx5_cmd_set_state(dev, MLX5_CMDIF_STATE_DOWN);
	mlx5_cmd_disable(dev, false);

so for failures of mlx5_core_set_issi(), mlx5_satisfy_startup_pages() and
mlx5_tout_query_dtor(), DISABLE_HCA is issued three lines above and its
result discarded - the same discard mlx5_function_disable() now stops doing
with the identical call.  The two callers of the new parameter therefore
treat the same evidence in opposite ways, and with false the reclaim strands
the whole command pool on every failed probe or reload attempt.  Should the
existing return value be captured here too, or the comment say why
DISABLE_HCA at this point is not meaningful?

>  
>  	return err;
>  }
>  
>  static void mlx5_function_disable(struct mlx5_core_dev *dev, bool boot)
>  {
> +	bool fw_stopped;
> +
>  	mlx5_reclaim_startup_pages(dev);
>  	mlx5_stop_health_poll(dev, boot);
> -	mlx5_core_disable_hca(dev, 0);
> +	/* A DISABLE_HCA that firmware really completed is it acknowledging
> +	 * that it has released the function, and so that it is done with the
> +	 * buffers the driver gave it.  mlx5_cmd_disable() needs to know,
> +	 * because it can only hand command mailboxes back to the DMA pool
> +	 * once that holds.
> +	 *
> +	 * A zero return is not sufficient on its own: mlx5_cmd_check() turns
> +	 * the -ENXIO from an interface that is already down into success for
> +	 * DISABLE_HCA, deliberately, so that reset flows proceed smoothly.
> +	 * That is exactly the case where nothing was posted and firmware
> +	 * acknowledged nothing, so require the interface to still be up.
> +	 */
> +	fw_stopped = !mlx5_core_disable_hca(dev, 0) && !mlx5_cmd_is_down(dev);
>  	mlx5_cmd_set_state(dev, MLX5_CMDIF_STATE_DOWN);
> -	mlx5_cmd_disable(dev);
> +	mlx5_cmd_disable(dev, fw_stopped);
>  }

[Severity: Medium]
Does one command's completion say anything about the mailboxes of the other
stalled slots?  With fw_stopped true, cmd_reclaim_stalled_ents() leaves
own_msgs set, so the final cmd_ent_put() dma_pool_free()s the mailboxes of
commands firmware never completed; pool->nr_active then reaches zero, so
dma_pool_destroy() really does dma_free_coherent() every page back to the
page allocator - while ent->lay->in_ptr/out_ptr still hold those addresses,
cmdq_addr_h/l is still programmed, and bus mastering is still enabled
(remove_one() runs mlx5_uninit_one() before mlx5_pci_close(), as the commit
message notes).

The patch's own evidence is firmware completing commands long after the
driver gave up ("Command completion arrived after timeout" logged hundreds of
times).  Is there a documented flush semantic tying DISABLE_HCA to
previously posted, uncompleted commands?

The driver already has a per-slot signal, used by poll_timeout():

		own = READ_ONCE(ent->lay->status_own);
		if (!(own & CMD_OWNER_HW)) {

Would gating the release on CMD_OWNER_HW per slot remove the dependence on
that assumption, and also let the fw_stopped == false paths reclaim the slots
firmware did quietly finish?

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260915225941.554568-1-dcostantino%40meta.com