[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