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

[netdev-bot+sashiko@kernel.org]

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