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

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

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);

 >
 >> @@ -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.

Please let me know if the proposed split works, I will include it in v3 
with the change above

Nikhil