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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] mlx5: race condition causes uninitialized memory use on command timeout
--

commit 9bc189299b20224d8c729888220a0891ddf01904
Author: Manjunath Patil <manjunath.b.patil@oracle.com>

net/mlx5: retain command mailboxes after timeout

This commit fixes a DMA pool reallocation issue by giving the command entry
ownership of the mailboxes on timeout and freeing them when the final
reference is dropped. It also serializes completion claims and firmware
reference handoffs under alloc_lock.

> 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
[ ... ]
> @@ -1163,6 +1170,10 @@ static void wait_func_handle_exec_timeout(struct mlx5_core_dev *dev,
>  		       mlx5_command_str(ent->op), ent->op);
>  
>  	ent->ret = -ETIMEDOUT;
> +	/* The real handler may have claimed the completion but still be using
> +	 * the mailboxes. Keep them with the entry until its last reference.
> +	 */
> +	set_bit(MLX5_CMD_ENT_STATE_RETAIN_MSGS, &ent->state);
>  	mlx5_cmd_comp_handler(dev, 1ULL << ent->idx, true);
>  }

[Severity: High]
Does setting the timeout error and retain flag without locks here race with
the real completion handler? 

If a blocking command times out just as the hardware completes it, this can
set ent->ret = -ETIMEDOUT while the EQ handler in mlx5_cmd_comp_handler()
is simultaneously checking or updating it.

[ ... ]
> @@ -1744,38 +1816,33 @@ static void mlx5_cmd_comp_handler(struct mlx5_core_dev *dev, u64 vec, bool force
>  	struct mlx5_cmd_stats *stats;
>  	unsigned long flags;
>  	unsigned long vector;
> +	bool pending;
> +	bool drop_fw_ref;
>  
>  	/* there can be at most 32 command queues */
>  	vector = vec & 0xffffffff;
>  	for (i = 0; i < (1 << cmd->vars.log_sz); i++) {
>  		if (test_bit(i, &vector)) {
>  			ent = cmd->ent_arr[i];
> -
> -			if (forced && ent->ret == -ETIMEDOUT)
> -				set_bit(MLX5_CMD_ENT_STATE_TIMEDOUT,
> -					&ent->state);
> -			else if (!forced) /* real FW completion */
> -				clear_bit(MLX5_CMD_ENT_STATE_TIMEDOUT,
> -					  &ent->state);
> +			pending = mlx5_cmd_claim_completion(dev, ent, vec, forced,
> +							    &drop_fw_ref);

[Severity: High]
Can this overwrite the concurrently set -ETIMEDOUT with 0 on success?

If the timeout from wait_func_handle_exec_timeout() occurs concurrently with
this completion handler, the following code later in mlx5_cmd_comp_handler()
can overwrite the error:

    if (!ent->ret) {
        if (!cmd->checksum_disabled)
            ent->ret = verify_signature(ent);

If the timeout occurs between the if (!ent->ret) check and the
ent->ret = verify_signature(ent) assignment, it appears this would hide the
timeout from the waiting thread.

[ ... ]
> @@ -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;
>  
>  	if (err > 0) /* Failed in FW, command didn't execute */
>  		err = deliv_status_to_err(err);

[Severity: High]
Does this cause uninitialized memory to be used if the race described above
occurs?

If retain_msgs is true but err was overwritten to 0 by the completion handler,
this skips copying the output buffer and returns success (0). 

Would this leave the caller with uninitialized memory in the output buffer,
potentially leading to data corruption or crashes when the caller assumes it
contains valid firmware responses?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261002205649.2029588-1-manjunath.b.patil@oracle.com?part=1