Re: [PATCH net] pds_core: fix use-after-free on workqueue during remove

[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
---
pds_core: fix use-after-free on workqueue during remove

This commit fixes a use-after-free on the workqueue during remove by
destroying the workqueue earlier and setting pdsc->wq to NULL.

> diff --git a/drivers/net/ethernet/amd/pds_core/main.c b/drivers/net/ethernet/amd/pds_core/main.c
> index 22db78343eb07..cb3ac26266aa7 100644
> --- a/drivers/net/ethernet/amd/pds_core/main.c
> +++ b/drivers/net/ethernet/amd/pds_core/main.c
> @@ -435,8 +435,10 @@ static void pdsc_remove(struct pci_dev *pdev)
>  		pdsc_auxbus_dev_del(pdsc, pdsc, &pdsc->padev);
>  
>  		timer_shutdown_sync(&pdsc->wdtimer);
> -		if (pdsc->wq)
> +		if (pdsc->wq) {
>  			destroy_workqueue(pdsc->wq);

[Severity: High]
This is a pre-existing issue, but can this destroy_workqueue() call deadlock
with pdsc_pci_reset_thread()?

The PCI subsystem holds device_lock() when calling pdsc_remove().
If pdsc_pci_reset_thread() is currently executing on this workqueue,
destroy_workqueue() will block waiting for it to finish.

However, pdsc_pci_reset_thread() calls pci_reset_function(), which attempts
to acquire the same device_lock(), resulting in a deadlock.

> +			pdsc->wq = NULL;

[Severity: High]
Can setting pdsc->wq to NULL here introduce a regression where the interrupt
handler dereferences a NULL pointer?

This assignment is executed before set_bit(PDSC_S_STOPPING_DRIVER,
&pdsc->state) and before pdsc_stop() masks hardware interrupts.

If an interrupt fires (e.g., a NotifyQ event) in this window,
pdsc_adminq_isr() will run. Because the PDSC_S_STOPPING_DRIVER state is not
yet set, the ISR check succeeds and it will unconditionally call queue_work()
with the NULL pdsc->wq:

pdsc_adminq_isr()
    ...
    queue_work(pdsc->wq, &qcq->work);

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