Re: [PATCH 1/6] pds_core: add support for quiet devcmd failures

[Simon Horman <horms@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
Full review at: https://sashiko.dev
---
--- Patch [1]: [PATCH 1/6] pds_core: add support for quiet devcmd failures ---
commit 2618e77f5d1aae31c9817f09b5056a69e237162a
Author: Brett Creeley <brett.creeley@amd.com>

pds_core: add support for quiet devcmd failures

This commit adds support to allow devcmd failure messages to be suppressed.
This will be used when adding support to negotiate
PDS_CORE_IDENTITY_VERSION_2.

> diff --git a/drivers/net/ethernet/amd/pds_core/dev.c b/drivers/net/ethernet/amd/pds_core/dev.c
> index bded6b33289ce..dd9989cfe6b3f 100644
> --- a/drivers/net/ethernet/amd/pds_core/dev.c
> +++ b/drivers/net/ethernet/amd/pds_core/dev.c

[ ... ]

> @@ -197,7 +199,7 @@ int pdsc_devcmd_locked(struct pdsc *pdsc, union pds_core_dev_cmd *cmd,
>  
>  	memcpy_toio(&pdsc->cmd_regs->cmd, cmd, sizeof(*cmd));
>  	pdsc_devcmd_dbell(pdsc);
> -	err = pdsc_devcmd_wait(pdsc, cmd->opcode, max_seconds);
> +	err = __pdsc_devcmd_wait(pdsc, cmd->opcode, max_seconds, do_msg);
>  
>  	if ((err == -ENXIO || err == -ETIMEDOUT) && pdsc->wq)
>  		queue_work(pdsc->wq, &pdsc->health_work);

[Severity: High]
This is a pre-existing issue, but does this error path trigger a
use-after-free during device removal?

During device removal, pdsc_remove() destroys the workqueue without clearing
the pointer before invoking pdsc_teardown():

pdsc_remove() {
	...
	if (pdsc->wq)
		destroy_workqueue(pdsc->wq);

	mutex_lock(&pdsc->config_lock);
	set_bit(PDSC_S_STOPPING_DRIVER, &pdsc->state);

	pdsc_stop(pdsc);
	pdsc_teardown(pdsc, PDSC_TEARDOWN_REMOVING);
	...
}

The teardown function issues a hardware reset command via pdsc_devcmd_reset().
If the device is unresponsive and the command times out, the error path here in
__pdsc_devcmd_locked() catches the -ETIMEDOUT error. 

Because pdsc->wq was left as a dangling pointer, the condition evaluates to true
and execution falls through to queue_work(), which will dereference the already
destroyed workqueue pointer.

Could the pdsc->wq pointer be set to NULL immediately after destruction to
prevent this, or should the teardown sequence be reordered?

>  	else
>  		memcpy_fromio(comp, &pdsc->cmd_regs->comp, sizeof(*comp));
>  
>  	if (err != -ETIMEDOUT && err != -EAGAIN)
>  		pdsc_deferred_dma_free(pdsc);
>  
>  	return err;
>  }