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

["Rao, Nikhil" <nikhirao@amd.com>]

On 7/6/2026 11:29 AM, Rao, Nikhil wrote:
> On 7/2/2026 1:13 AM, Paolo Abeni wrote:> On 6/29/26 10:03 PM, Nikhil P. 
> Rao 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().
>  >
>  > I think this should be 2 separate patches.
> 
> Thanks for the review.
> 
> The original combined them because moving destroy_workqueue() after 
> teardown fixed both issues.
> 
> But they can be split:
> 
> - Patch 1: Set pdsc->wq = NULL before destroying it (fixes issue 1). The 
> devcmd error path checks pdsc->wq before queuing, so this prevents the 
> UAF. Also avoids needless health recovery during remove.
> 
> - Patch 2: Move destroy_workqueue() after teardown and add 
> cancel_work_sync() (fixes issue 2).
> 
> Combined, it looks like this:
> 
> struct workqueue_struct *wq = pdsc->wq;
> if (wq)
>     pdsc->wq = NULL;
> 
> /* teardown (free IRQ, cancel work, free memory) */
> 
> if (wq)
>      destroy_workqueue(wq);
> 
Setting pdsc->wq = NULL risks a NULL deref in pdsc_adminq_isr(), so the
split I suggested earlier doesn't hold.

>  >
>  >> @@ -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);
>  > AFAICS this does not look safe.
>  >
>  > A concurrent pdsc_qcq_free()/pdsc_qcq_intr_free() may free
>  > `pdsc->intr_ctrl` before setting PDS_CORE_INTR_INDEX_NOT_ASSIGNED.
>  >
>  > I think the teardown should:
>  >
>  > - disable the IRQ
>  > - cancel the work
>  > - free the structs
>  > in the above sequence.
> 
> pdsc_qcq_intr_free() doesn't free intr_ctrl - it only calls free_irq(). 
> intr_ctrl is BAR-mapped memory, freed later by pdsc_unmap_bars() after 
> teardown completes.
> 
> The sequence is already as suggested above:
> 
> pdsc_qcq_free()
> - pdsc_qcq_intr_free() /* calls pdsc_intr_free->free_irq() */
> - cancel_work_sync()
> - Frees DMA memory and structs
> 
> Note: the adminqcq ISR handles both adminq completions and notifyq 
> events, so notifyq struct is freed second.
> 
> In the current code, qcq->intx is set to NOT_ASSIGNED in 
> pdsc_qcq_intr_free(). Moving this to after cancel_work_sync() removes 
> the need for READ_ONCE/WRITE_ONCE.
>
There is no change in v3[1] beyond deleting READ_ONCE(qcq->intx) (and 
the corresponding WRITE_ONCE):
qcq->intx is now cleared after cancel_work_sync() in pdsc_qcq_free(), so 
pdsc_process_adminq() finishes before it changes.

On the Sashiko comment[2] asking whether the credit-return write could 
hit "the hardware register of a freed OS interrupt vector" and cause an
interrupt storm:

The credit write in pdsc_process_adminq() targets intr_ctrl[], device
MMIO in a PCI BAR, not the OS IRQ vector released by free_irq().
free_irq() shuts down and masks the MSI-X vector, so even if the device
is rearmed after free_irq(), the interrupt cannot be delivered - AFAIK,
is no unhandled interrupt storm.

I will send fixes for the rest of the Sashiko review comments.

Thanks,
Nikhil

[1] 
https://lore.kernel.org/netdev/20260714180223.1642792-1-nikhil.rao@amd.com/T/#t
[2] 
https://sashiko.dev/#/patchset/20260629200358.2626129-1-nikhil.rao%40amd.com