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

[PATCH net v2 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.

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

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/adminq.c | 15 +++++++++++----
 drivers/net/ethernet/amd/pds_core/core.c   | 21 ++++++++++++++-------
 drivers/net/ethernet/amd/pds_core/main.c   |  5 +++--
 3 files changed, 28 insertions(+), 13 deletions(-)

--
2.43.0

[PATCH net v2 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 AI Review <sashiko-bot@kernel.org>
Signed-off-by: Nikhil P. Rao <nikhil.rao@amd.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 v2 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 problem 1 by moving destroy_workqueue() after pdsc_teardown(),
ensuring the workqueue exists when health_work may be queued during
teardown.

Fix problem 2 by adding cancel_work_sync() in pdsc_qcq_free() after
free_irq(). This ensures no new ISR can queue work, and any
already-queued work is drained before freeing the qcq. Work draining
during teardown may race with intx becoming invalid, so skip returning
interrupt credits if intx is no longer assigned.

Also change pdsc_core_uninit() to free adminqcq before notifyqcq,
since adminqcq's work accesses notifyqcq via pdsc_process_notifyq().
This ensures notifyqcq remains valid while adminqcq's work drains.

Fixes: 01ba61b55b20 ("pds_core: Add adminq processing and commands")
Reported-by: Sashiko AI Review <sashiko-bot@kernel.org>
Signed-off-by: Nikhil P. Rao <nikhil.rao@amd.com>
---
 drivers/net/ethernet/amd/pds_core/adminq.c | 15 +++++++++++----
 drivers/net/ethernet/amd/pds_core/core.c   | 14 ++++++++++----
 drivers/net/ethernet/amd/pds_core/main.c   |  5 +++--
 3 files changed, 24 insertions(+), 10 deletions(-)

diff --git a/drivers/net/ethernet/amd/pds_core/adminq.c b/drivers/net/ethernet/amd/pds_core/adminq.c
index 097bb092bdb8..c0d9b7e6b8c3 100644
--- a/drivers/net/ethernet/amd/pds_core/adminq.c
+++ b/drivers/net/ethernet/amd/pds_core/adminq.c
@@ -77,6 +77,7 @@ void pdsc_process_adminq(struct pdsc_qcq *qcq)
 	unsigned long irqflags;
 	int nq_work = 0;
 	int aq_work = 0;
+	int intx;
 
 	/* Don't process AdminQ when it's not up */
 	if (!pdsc_adminq_inc_if_up(pdsc)) {
@@ -121,10 +122,16 @@ void pdsc_process_adminq(struct pdsc_qcq *qcq)
 	qcq->accum_work += aq_work;
 
 credits:
-	/* Return the interrupt credits, one for each completion */
-	pds_core_intr_credits(&pdsc->intr_ctrl[qcq->intx],
-			      nq_work + aq_work,
-			      PDS_CORE_INTR_CRED_REARM);
+	/* Return the interrupt credits, one for each completion.
+	 * Use READ_ONCE to get a single consistent copy of intx since it can
+	 * be set to PDS_CORE_INTR_INDEX_NOT_ASSIGNED concurrently during
+	 * teardown, and skip the credits if so.
+	 */
+	intx = READ_ONCE(qcq->intx);
+	if (intx != PDS_CORE_INTR_INDEX_NOT_ASSIGNED)
+		pds_core_intr_credits(&pdsc->intr_ctrl[intx],
+				      nq_work + aq_work,
+				      PDS_CORE_INTR_CRED_REARM);
 	refcount_dec(&pdsc->adminq_refcnt);
 }
 
diff --git a/drivers/net/ethernet/amd/pds_core/core.c b/drivers/net/ethernet/amd/pds_core/core.c
index 1074a022a52f..570c0cd7339e 100644
--- a/drivers/net/ethernet/amd/pds_core/core.c
+++ b/drivers/net/ethernet/amd/pds_core/core.c
@@ -110,7 +110,8 @@ 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;
+	/* Pairs with READ_ONCE in pdsc_process_adminq() */
+	WRITE_ONCE(qcq->intx, PDS_CORE_INTR_INDEX_NOT_ASSIGNED);
 }
 
 static int pdsc_qcq_intr_alloc(struct pdsc *pdsc, struct pdsc_qcq *qcq)
@@ -145,6 +146,10 @@ 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);
+
 	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 v2 1/2] pds_core: fix deadlock between reset thread and remove

[Harshitha Ramamurthy]

On Mon, Jun 29, 2026 at 1:04 PM Nikhil P. Rao <nikhil.rao@amd.com> wrote:
>
> 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 AI Review <sashiko-bot@kernel.org>
> 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
>
>

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

[Harshitha Ramamurthy]

On Mon, Jun 29, 2026 at 1:04 PM Nikhil P. Rao <nikhil.rao@amd.com> wrote:
>
> 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 problem 1 by moving destroy_workqueue() after pdsc_teardown(),
> ensuring the workqueue exists when health_work may be queued during
> teardown.
>
> Fix problem 2 by adding cancel_work_sync() in pdsc_qcq_free() after
> free_irq(). This ensures no new ISR can queue work, and any
> already-queued work is drained before freeing the qcq. Work draining
> during teardown may race with intx becoming invalid, so skip returning
> interrupt credits if intx is no longer assigned.
>
> Also change pdsc_core_uninit() to free adminqcq before notifyqcq,
> since adminqcq's work accesses notifyqcq via pdsc_process_notifyq().
> This ensures notifyqcq remains valid while adminqcq's work drains.

Code LGTM but wonder if this change could be split into a separate patch.

>
> Fixes: 01ba61b55b20 ("pds_core: Add adminq processing and commands")
> Reported-by: Sashiko AI Review <sashiko-bot@kernel.org>
> Signed-off-by: Nikhil P. Rao <nikhil.rao@amd.com>
> ---
>  drivers/net/ethernet/amd/pds_core/adminq.c | 15 +++++++++++----
>  drivers/net/ethernet/amd/pds_core/core.c   | 14 ++++++++++----
>  drivers/net/ethernet/amd/pds_core/main.c   |  5 +++--
>  3 files changed, 24 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/net/ethernet/amd/pds_core/adminq.c b/drivers/net/ethernet/amd/pds_core/adminq.c
> index 097bb092bdb8..c0d9b7e6b8c3 100644
> --- a/drivers/net/ethernet/amd/pds_core/adminq.c
> +++ b/drivers/net/ethernet/amd/pds_core/adminq.c
> @@ -77,6 +77,7 @@ void pdsc_process_adminq(struct pdsc_qcq *qcq)
>         unsigned long irqflags;
>         int nq_work = 0;
>         int aq_work = 0;
> +       int intx;
>
>         /* Don't process AdminQ when it's not up */
>         if (!pdsc_adminq_inc_if_up(pdsc)) {
> @@ -121,10 +122,16 @@ void pdsc_process_adminq(struct pdsc_qcq *qcq)
>         qcq->accum_work += aq_work;
>
>  credits:
> -       /* Return the interrupt credits, one for each completion */
> -       pds_core_intr_credits(&pdsc->intr_ctrl[qcq->intx],
> -                             nq_work + aq_work,
> -                             PDS_CORE_INTR_CRED_REARM);
> +       /* Return the interrupt credits, one for each completion.
> +        * Use READ_ONCE to get a single consistent copy of intx since it can
> +        * be set to PDS_CORE_INTR_INDEX_NOT_ASSIGNED concurrently during
> +        * teardown, and skip the credits if so.
> +        */
> +       intx = READ_ONCE(qcq->intx);
> +       if (intx != PDS_CORE_INTR_INDEX_NOT_ASSIGNED)
> +               pds_core_intr_credits(&pdsc->intr_ctrl[intx],
> +                                     nq_work + aq_work,
> +                                     PDS_CORE_INTR_CRED_REARM);
>         refcount_dec(&pdsc->adminq_refcnt);
>  }
>
> diff --git a/drivers/net/ethernet/amd/pds_core/core.c b/drivers/net/ethernet/amd/pds_core/core.c
> index 1074a022a52f..570c0cd7339e 100644
> --- a/drivers/net/ethernet/amd/pds_core/core.c
> +++ b/drivers/net/ethernet/amd/pds_core/core.c
> @@ -110,7 +110,8 @@ 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;
> +       /* Pairs with READ_ONCE in pdsc_process_adminq() */
> +       WRITE_ONCE(qcq->intx, PDS_CORE_INTR_INDEX_NOT_ASSIGNED);
>  }
>
>  static int pdsc_qcq_intr_alloc(struct pdsc *pdsc, struct pdsc_qcq *qcq)
> @@ -145,6 +146,10 @@ 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);
> +
>         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 v2 2/2] pds_core: fix use-after-free on workqueue during remove

[Rao, Nikhil]

Hi,

On 6/29/2026 2:32 PM, Harshitha Ramamurthy wrote:
>
> On Mon, Jun 29, 2026 at 1:04 PM Nikhil P. Rao <nikhil.rao@amd.com> wrote:
>>
[..]
>> Also change pdsc_core_uninit() to free adminqcq before notifyqcq,
>> since adminqcq's work accesses notifyqcq via pdsc_process_notifyq().
>> This ensures notifyqcq remains valid while adminqcq's work drains.
> 
> Code LGTM but wonder if this change could be split into a separate patch.

Thanks for the review.

The previous code had the bug this patch fixes: destroy_workqueue() was 
called before pdsc_teardown(), so work could be queued to a destroyed 
workqueue.

This patch fixes it by moving destroy_workqueue() after pdsc_teardown(), 
now adminqcq's work accesses notifyqcq, hence the change in order of 
freeing is tied to the fix.

Nikhil