[PATCH net v3 0/2] pds_core: fix use-after-free on workqueue during remove

[PATCH net v3 0/2] pds_core: fix use-after-free on workqueue during remove

[Nikhil P. Rao]

This series fixes a use-after-free on the workqueue during driver remove.

Patch 1 fixes a pre-existing deadlock between the PCI reset worker and
pdsc_remove() that was identified during review of v1.

Patch 2 is the reworked UAF fix that moves destroy_workqueue() after
pdsc_teardown() and adds proper work synchronization.

v3:
- Drop READ_ONCE(qcq->intx) in pdsc_process_adminq(); clear qcq->intx
  after cancel_work_sync() in pdsc_qcq_free() so the work path observes
  a stable value without the barrier (addresses Paolo's v2 comment).
  This removes the adminq.c change entirely.

v2:
- Fix deadlock between pci_reset_thread and remove (new patch 1/2)
  found by sashiko AI review of v1
- Rework UAF fix: move destroy_workqueue() after pdsc_teardown()
  instead of setting wq to NULL (addresses NULL deref found by sashiko)
- Add cancel_work_sync() after free_irq() to drain ISR-queued work
- Reorder adminqcq/notifyqcq freeing to avoid accessing freed notifyqcq

v2: https://lore.kernel.org/netdev/20260629200358.2626129-3-nikhil.rao@amd.com/
v1: https://lore.kernel.org/netdev/20260610025952.196470-1-nikhil.rao@amd.com/

Nikhil P. Rao (2):
  pds_core: fix deadlock between reset thread and remove
  pds_core: fix use-after-free on workqueue during remove

 drivers/net/ethernet/amd/pds_core/core.c | 21 ++++++++++++++-------
 drivers/net/ethernet/amd/pds_core/main.c |  5 +++--
 2 files changed, 17 insertions(+), 9 deletions(-)

--
2.43.0

[PATCH net v3 1/2] pds_core: fix deadlock between reset thread and remove

[Nikhil P. Rao]

pci_reset_function() acquires device_lock before performing the reset.
pdsc_remove() is called by the PCI core with device_lock already held.
If pdsc_pci_reset_thread() is running when pdsc_remove() is called,
destroy_workqueue() will block waiting for the work to complete, while
the work is blocked waiting for device_lock - deadlock.

Use pci_try_reset_function() which uses pci_dev_trylock() internally.
This acquires both the device lock and the PCI config access lock
without blocking - if either lock is contended, it returns -EAGAIN
immediately. This avoids the deadlock while also ensuring proper
config space access serialization during the reset.

The pci_dev_get/put calls are also removed as they were unnecessary -
the driver-owned workqueue is destroyed in pdsc_remove(), guaranteeing
the work completes before remove returns. The PCI core holds its
reference to pci_dev throughout the entire unbind sequence.

Fixes: 81665adf25d2 ("pds_core: Fix pdsc_check_pci_health function to use work thread")
Reported-by: sashiko-bot <sashiko-bot@kernel.org>
Closes: https://patchwork.kernel.org/comment/27002369/
Signed-off-by: Nikhil P. Rao <nikhil.rao@amd.com>
Reviewed-by: Harshitha Ramamurthy <hramamurthy@google.com>
---
 drivers/net/ethernet/amd/pds_core/core.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/amd/pds_core/core.c b/drivers/net/ethernet/amd/pds_core/core.c
index 38a2446571af..1074a022a52f 100644
--- a/drivers/net/ethernet/amd/pds_core/core.c
+++ b/drivers/net/ethernet/amd/pds_core/core.c
@@ -606,9 +606,10 @@ void pdsc_pci_reset_thread(struct work_struct *work)
 	struct pdsc *pdsc = container_of(work, struct pdsc, pci_reset_work);
 	struct pci_dev *pdev = pdsc->pdev;
 
-	pci_dev_get(pdev);
-	pci_reset_function(pdev);
-	pci_dev_put(pdev);
+	/* Use try variant to avoid deadlock with pdsc_remove().
+	 * If lock is contended, the watchdog timer will retry.
+	 */
+	pci_try_reset_function(pdev);
 }
 
 static void pdsc_check_pci_health(struct pdsc *pdsc)
-- 
2.43.0

[PATCH net v3 2/2] pds_core: fix use-after-free on workqueue during remove

[Nikhil P. Rao]

In pdsc_remove(), the workqueue is destroyed before pdsc_teardown()
is called. This ordering allows two paths to queue work on the
destroyed workqueue:

1. If pdsc_teardown() -> pdsc_devcmd_reset() times out, the error
   path in pdsc_devcmd_locked() queues health_work.

2. A NotifyQ event can trigger the ISR and queue work before free_irq()
   is called in pdsc_teardown().

Fix by moving destroy_workqueue() after pdsc_teardown() so the
workqueue outlives every queuer; destroy_workqueue() then flushes any
work still pending.

Draining the queued work also requires ordering the teardown so the
resources that work touches are freed last:

  - In pdsc_qcq_free(), after freeing the interrupt, cancel_work_sync()
    the queue's work and only then clear qcq->intx, so
    pdsc_process_adminq()'s read of qcq->intx for interrupt-credit
    return cannot race with the clear.

  - Free adminqcq before notifyqcq: the shared adminq ISR is released
    when adminqcq is freed, and the adminq work accesses notifyqcq, so
    both must be stopped before notifyqcq is freed.

Fixes: 01ba61b55b20 ("pds_core: Add adminq processing and commands")
Reported-by: sashiko-bot <sashiko-bot@kernel.org>
Closes: https://patchwork.kernel.org/comment/27002369/
Signed-off-by: Nikhil P. Rao <nikhil.rao@amd.com>
---
 drivers/net/ethernet/amd/pds_core/core.c | 14 ++++++++++----
 drivers/net/ethernet/amd/pds_core/main.c |  5 +++--
 2 files changed, 13 insertions(+), 6 deletions(-)

diff --git a/drivers/net/ethernet/amd/pds_core/core.c b/drivers/net/ethernet/amd/pds_core/core.c
index 1074a022a52f..e39b2c9beb20 100644
--- a/drivers/net/ethernet/amd/pds_core/core.c
+++ b/drivers/net/ethernet/amd/pds_core/core.c
@@ -110,7 +110,6 @@ static void pdsc_qcq_intr_free(struct pdsc *pdsc, struct pdsc_qcq *qcq)
 		return;
 
 	pdsc_intr_free(pdsc, qcq->intx);
-	qcq->intx = PDS_CORE_INTR_INDEX_NOT_ASSIGNED;
 }
 
 static int pdsc_qcq_intr_alloc(struct pdsc *pdsc, struct pdsc_qcq *qcq)
@@ -145,6 +144,12 @@ void pdsc_qcq_free(struct pdsc *pdsc, struct pdsc_qcq *qcq)
 
 	pdsc_qcq_intr_free(pdsc, qcq);
 
+	/* Drain any work queued by ISR before it was freed above */
+	if (qcq->work.func)
+		cancel_work_sync(&qcq->work);
+
+	qcq->intx = PDS_CORE_INTR_INDEX_NOT_ASSIGNED;
+
 	if (qcq->q_base)
 		dma_free_coherent(dev, qcq->q_size,
 				  qcq->q_base, qcq->q_base_pa);
@@ -304,8 +309,11 @@ int pdsc_qcq_alloc(struct pdsc *pdsc, unsigned int type, unsigned int index,
 
 static void pdsc_core_uninit(struct pdsc *pdsc)
 {
-	pdsc_qcq_free(pdsc, &pdsc->notifyqcq);
+	/* Free adminqcq first: its work accesses notifyqcq, so we must
+	 * disable its IRQ and drain its work before freeing notifyqcq.
+	 */
 	pdsc_qcq_free(pdsc, &pdsc->adminqcq);
+	pdsc_qcq_free(pdsc, &pdsc->notifyqcq);
 
 	if (pdsc->kern_dbpage) {
 		iounmap(pdsc->kern_dbpage);
@@ -479,8 +487,6 @@ void pdsc_teardown(struct pdsc *pdsc, bool removing)
 {
 	if (!pdsc->pdev->is_virtfn)
 		pdsc_devcmd_reset(pdsc);
-	if (pdsc->adminqcq.work.func)
-		cancel_work_sync(&pdsc->adminqcq.work);
 
 	pci_clear_master(pdsc->pdev);
 
diff --git a/drivers/net/ethernet/amd/pds_core/main.c b/drivers/net/ethernet/amd/pds_core/main.c
index 22db78343eb0..638b9c7a509d 100644
--- a/drivers/net/ethernet/amd/pds_core/main.c
+++ b/drivers/net/ethernet/amd/pds_core/main.c
@@ -435,8 +435,6 @@ static void pdsc_remove(struct pci_dev *pdev)
 		pdsc_auxbus_dev_del(pdsc, pdsc, &pdsc->padev);
 
 		timer_shutdown_sync(&pdsc->wdtimer);
-		if (pdsc->wq)
-			destroy_workqueue(pdsc->wq);
 
 		mutex_lock(&pdsc->config_lock);
 		set_bit(PDSC_S_STOPPING_DRIVER, &pdsc->state);
@@ -444,6 +442,9 @@ static void pdsc_remove(struct pci_dev *pdev)
 		pdsc_stop(pdsc);
 		pdsc_teardown(pdsc, PDSC_TEARDOWN_REMOVING);
 		mutex_unlock(&pdsc->config_lock);
+
+		if (pdsc->wq)
+			destroy_workqueue(pdsc->wq);
 		mutex_destroy(&pdsc->config_lock);
 		mutex_destroy(&pdsc->devcmd_lock);
 
-- 
2.43.0

Re: [PATCH net v3 1/2] pds_core: fix deadlock between reset thread and remove

[Simon Horman]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
https://sashiko.dev/#/patchset/20260714180223.1642792-2-nikhil.rao@amd.com?part=1
---
--- Patch [1]: [PATCH net v3 1/2] pds_core: fix deadlock between reset thread and remove ---
commit e973ecbe07b7edd4ed406cf37388217ed5a167be
Author: Nikhil P. Rao <nikhil.rao@amd.com>

pds_core: fix deadlock between reset thread and remove

This commit switches the PCI reset function in pdsc_pci_reset_thread() to
use pci_try_reset_function(). This avoids a deadlock that occurs when
pdsc_remove() holds the device_lock while waiting for destroy_workqueue()
to complete.

Link: https://patchwork.kernel.org/comment/27002369/

> pci_reset_function() acquires device_lock before performing the reset.
> pdsc_remove() is called by the PCI core with device_lock already held.
> If pdsc_pci_reset_thread() is running when pdsc_remove() is called,
> destroy_workqueue() will block waiting for the work to complete, while
> the work is blocked waiting for device_lock - deadlock.
> 
> Use pci_try_reset_function() which uses pci_dev_trylock() internally.
> This acquires both the device lock and the PCI config access lock
> without blocking - if either lock is contended, it returns -EAGAIN
> immediately. This avoids the deadlock while also ensuring proper
> config space access serialization during the reset.
> 
> The pci_dev_get/put calls are also removed as they were unnecessary -
> the driver-owned workqueue is destroyed in pdsc_remove(), guaranteeing
> the work completes before remove returns. The PCI core holds its
> reference to pci_dev throughout the entire unbind sequence.

[Severity: High]
This is a pre-existing issue, but since we rely on destroy_workqueue() 
during pdsc_remove(), does this leave a dangling pointer that can be 
accessed during teardown?

In pdsc_remove(), destroy_workqueue(pdsc->wq) is called but pdsc->wq is 
not set to NULL. Immediately after, pdsc_remove() calls pdsc_teardown() 
which calls pdsc_devcmd_reset(). If the firmware is unresponsive, 
pdsc_devcmd_wait() times out and pdsc_devcmd_locked() will unconditionally 
evaluate if (pdsc->wq), which remains true, and attempt to queue health work.

Additionally, hardware interrupts are not fully masked until pdsc_stop() 
is called after destroy_workqueue(). Could an interrupt arriving during this 
narrow window cause pdsc_adminq_isr() to race and call 
queue_work(pdsc->wq, &qcq->work) on the destroyed workqueue?

> diff --git a/drivers/net/ethernet/amd/pds_core/core.c b/drivers/net/ethernet/amd/pds_core/core.c
> index 38a2446571afa..1074a022a52fb 100644
> --- a/drivers/net/ethernet/amd/pds_core/core.c
> +++ b/drivers/net/ethernet/amd/pds_core/core.c
> @@ -606,9 +606,10 @@ void pdsc_pci_reset_thread(struct work_struct *work)
>  	struct pdsc *pdsc = container_of(work, struct pdsc, pci_reset_work);
>  	struct pci_dev *pdev = pdsc->pdev;
>  
> -	pci_dev_get(pdev);
> -	pci_reset_function(pdev);
> -	pci_dev_put(pdev);
> +	/* Use try variant to avoid deadlock with pdsc_remove().
> +	 * If lock is contended, the watchdog timer will retry.
> +	 */
> +	pci_try_reset_function(pdev);

[Severity: High]
This is a pre-existing issue, but could this uncancelled work item perform 
an uncoordinated PCI reset if it executes after an AER recovery?

When a PCI error is detected, pdsc_health_thread() queues pci_reset_work. 
If the PCI core independently initiates AER recovery before this work executes, 
it calls pdsc_reset_prepare(), which then calls pdsc_stop_health_thread().

While pdsc_stop_health_thread() cancels health_work, it does not appear to 
cancel this already-queued pci_reset_work. 

Because the AER thread holds the device lock during recovery, if this work 
is delayed and executes after AER recovery completes and releases the lock, 
will pci_try_reset_function() successfully acquire the lock and perform a 
hardware reset while the driver thinks the device is actively running?

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

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Tue, 14 Jul 2026 18:02:21 +0000 you wrote:
> This series fixes a use-after-free on the workqueue during driver remove.
> 
> Patch 1 fixes a pre-existing deadlock between the PCI reset worker and
> pdsc_remove() that was identified during review of v1.
> 
> Patch 2 is the reworked UAF fix that moves destroy_workqueue() after
> pdsc_teardown() and adds proper work synchronization.
> 
> [...]

Here is the summary with links:
  - [net,v3,1/2] pds_core: fix deadlock between reset thread and remove
    https://git.kernel.org/netdev/net/c/ab0eec0ff0a4
  - [net,v3,2/2] pds_core: fix use-after-free on workqueue during remove
    https://git.kernel.org/netdev/net/c/0ad134881508

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html