Re: [PATCH net] net/mlx5: retain command mailboxes after timeout

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 2 · Medium: 1 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] Retained mailboxes are freed after cmd_ent_put() has already
  released the slot.
- [High] A reset flush can now consume the retained FW reference while a
  software EQ-recovery pass is processing the late real completion for…
- [Medium] Mailboxes now owned by the entry are never released on normal
  command-interface teardown, so dma_pool_destroy() finds the pool busy…

--

net/mlx5: retain command mailboxes after timeout

After a command timeout, this patch gives the command entry ownership of
its input and output mailboxes, and cmd_free_ent() frees them on the final
reference. A new helper, mlx5_cmd_claim_completion(), takes the
PENDING_COMP claim and hands off the firmware reference under alloc_lock.
Triggered (reset) completions now drop a reference that an earlier timeout
kept.

> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/cmd.c b/drivers/net/ethernet/mellanox/mlx5/core/cmd.c
> index 84583dc5eb1c0..22508b26972de 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/cmd.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/cmd.c
> @@ -142,8 +142,19 @@ cmd_alloc_ent(struct mlx5_cmd *cmd, struct mlx5_cmd_msg *in,
>  	return ent;
>  }
>  
> +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_free_ent(struct mlx5_cmd_work_ent *ent)
>  {
> +	if (test_bit(MLX5_CMD_ENT_STATE_RETAIN_MSGS, &ent->state)) {
> +		struct mlx5_core_dev *dev = container_of(ent->cmd,
> +							  struct mlx5_core_dev, cmd);
> +
> +		mlx5_free_cmd_msg(dev, ent->out);
> +		free_msg(dev, ent->in);
> +	}
>  	kfree(ent);
>  }

[Severity: High]
Can these retained mailboxes be freed after the command DMA pool has
already been destroyed?

cmd_ent_put() releases the slot and the semaphore under alloc_lock. It
then drops the lock and only after that calls cmd_free_ent():

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

	cmd_free_ent(ent);
}

With this patch, cmd_free_ent() calls dma_pool_free() on cmd->pool
through mlx5_free_cmd_msg(). free_msg() can also list_add the input msg
back onto msg->parent->head.

For an async command, cb_timeout_handler() runs on system_wq and holds its
own reference. Its forced mlx5_cmd_comp_handler() call sets RETAIN_MSGS and
TIMEDOUT and runs the user callback, so mlx5_async_ctx teardown stops
waiting. Only after that does it call cmd_ent_put().

The firmware reference can be dropped first, either by a late real
completion or by mlx5_cmd_trigger_completions(), which drops both the
firmware reference and its own temporary one. In that case the put in
cb_timeout_handler() is the final one.

Once that put calls up(), the down_trylock() loop in mlx5_cmd_flush() can
succeed. Error recovery or unload can then continue into
mlx5_cmd_disable():

	flush_workqueue(cmd->wq);
	clean_debug_files(dev);
	destroy_msg_cache(dev);
	free_cmd_page(dev, cmd);
	dma_pool_destroy(cmd->pool);

This flushes only cmd->wq. It does not flush system_wq or the
cb_timeout_work items.

Suppose the timeout worker is preempted after it drops alloc_lock and
resumes after this point. Would cmd_free_ent() then call dma_pool_free() on
a freed struct dma_pool, or add the input msg to a destroyed cache list?

Before this patch, this window only covered kfree(ent), which never
touched the pool.

[ ... ]

> @@ -1732,6 +1744,66 @@ static void free_msg(struct mlx5_core_dev *dev, struct mlx5_cmd_msg *msg)
>  	}
>  }
>  
> +/*

[ ... ]

> + * A blocking timeout retains the mailboxes even when a real completion has
> + * claimed PENDING_COMP. That handler keeps its firmware-event reference until
> + * it finishes using the mailboxes. Keeping the reference transitions under
> + * alloc_lock prevents timeout, firmware, and reset from consuming the same
> + * reference.
> + */
> +static bool mlx5_cmd_claim_completion(struct mlx5_core_dev *dev,
> +				      struct mlx5_cmd_work_ent *ent, u64 vec,
> +				      bool forced, bool *drop_fw_ref)
> +{

[ ... ]

> +	} else if (!forced) {
> +		clear_bit(MLX5_CMD_ENT_STATE_TIMEDOUT, &ent->state);
> +		*drop_fw_ref = true;
> +	} else if (vec & MLX5_TRIGGERED_CMD_COMP) {
> +		/* Reset cannot receive a late firmware completion. */
> +		*drop_fw_ref = test_and_clear_bit(MLX5_CMD_ENT_STATE_TIMEDOUT,
> +						  &ent->state);
> +	}

[Severity: High]
Can a reset flush and a late real completion both drop the same retained
firmware reference here?

Start with entry A, which has already timed out (PENDING_COMP clear,
TIMEDOUT set). Only its firmware reference keeps it alive.

Thread R: another command's timeout calls mlx5_cmd_eq_recover(). That runs
mlx5_eq_async_int(ASYNC_EQ_RECOVER) in process context. cmd_comp_notifier()
passes its INTERNAL_ERROR check, and mlx5_cmd_comp_handler(dev, vec, false)
reads ent = cmd->ent_arr[i] without taking a reference.

Thread F: enter_error_state() sets INTERNAL_ERROR and calls
mlx5_cmd_flush()->mlx5_cmd_trigger_completions().
mlx5_eq_synchronize_cmd_irq() does not wait for R, because R is not running
in IRQ context.

The sequence would be:

F: cmd_ent_get(cmd->ent_arr[i])          /* temporary ref */
F: mlx5_cmd_claim_completion()
     vec & MLX5_TRIGGERED_CMD_COMP
     test_and_clear_bit(TIMEDOUT) -> true
   cmd_ent_put(ent)                      /* drops fw ref */
R: mlx5_cmd_claim_completion()
     !forced
     *drop_fw_ref = true                 /* unconditional */
   cmd_ent_put(ent)                      /* drops F's temp ref */
     -> refcount 0, ent_arr[i] = NULL, cmd_free_ent()
F: cmd_ent_put(cmd->ent_arr[i])          /* NULL or a new entry */

Would the trailing put in mlx5_cmd_trigger_completions() then dereference
NULL? Or would it drop a reference on an unrelated entry that has reused
slot i?

If F finishes completely first, R's claim would touch the freed entry
instead.

The comment above says:

    Keeping the reference transitions under alloc_lock prevents timeout,
    firmware, and reset from consuming the same reference.

The !forced branch does not check whether TIMEDOUT is still set, so does
the helper actually enforce this?

In the baseline, the triggered path never dropped the reference of an
entry with TIMEDOUT set, so this sequence stayed balanced.

[ ... ]

> @@ -2008,9 +2082,12 @@ static int cmd_exec(struct mlx5_core_dev *dev, void *in, int in_size, void *out,
>  	}
>  
>  	err = mlx5_cmd_invoke(dev, inb, outb, out, out_size, callback, context,
> -			      pages_queue, token, force_polling);
> +			      pages_queue, token, force_polling, &retain_msgs);
>  	if (callback && !err)
>  		return 0;
> +	/* The entry releases retained DMA mailboxes with its final reference. */
> +	if (retain_msgs)
> +		goto out_up;

[Severity: Medium]
What releases these retained mailboxes on a normal command interface
teardown?

Take a command that times out while the interface is up and its opcode is
allowed. mlx5_cmd_claim_completion() sets RETAIN_MSGS and TIMEDOUT and
keeps the firmware reference. cmd_exec() now skips freeing inb and outb.

Only two things drop that reference: a later real completion, or a
MLX5_TRIGGERED_CMD_COMP flush. The flush only runs from
enter_error_state().

mlx5_function_disable() and the err_cmd_cleanup path in
mlx5_function_enable() call mlx5_cmd_disable() without either:

err_cmd_cleanup:
	mlx5_cmd_set_state(dev, MLX5_CMDIF_STATE_DOWN);
	mlx5_cmd_disable(dev);

mlx5_cmd_disable() then runs destroy_msg_cache() and
dma_pool_destroy(cmd->pool) while the retained blocks are still allocated.
dma_pool_destroy() sees nr_active != 0, prints "dma_pool_destroy mlx5_cmd
busy", and skips dma_free_coherent() for every page.

Does this leak all of the pool's coherent pages, including the ones backing
the whole message cache?

A retained cached input msg is also not on ch->head, so destroy_msg_cache()
never frees it.

Polling mode is active during mlx5_function_enable(), and it seems to reach
this easily:

cmd_work_handler() {
    ...
	if (cmd_mode == CMD_MODE_POLLING || poll_cmd) {
		poll_timeout(ent);
		/* make sure we read the descriptor after ownership is SW */
		rmb();
		mlx5_cmd_comp_handler(dev, 1ULL << ent->idx, !!ent->ret);
	}
}

There is no command EQ yet, so no late completion can arrive. If
MANAGE_PAGES, SET_ISSI or QUERY_ISSI times out and mlx5_function_enable()
then fails, the pool leaks.

Before this patch, cmd_exec() freed the mailboxes at timeout, so only the
ent struct and its slot leaked.

A less certain follow-on: if a triggered flush drops the retained
reference after mlx5_cmd_disable() has run, would cmd_free_ent() call
dma_pool_free() on the destroyed pool? cmd->pool is not cleared.

>  
>  	if (err > 0) /* Failed in FW, command didn't execute */
>  		err = deliv_status_to_err(err);

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261002205649.2029588-1-manjunath.b.patil%40oracle.com