Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [High] Use-After-Free and waitqueue data corruption involving `gc->hwc_drain_waitq` due to flawed teardown logic and a race condition. --
--- Patch [6]: [PATCH net-next 6/7] net: mana: support concurrent HWC requests with proper synchronization --- commit c7a2831143a0268564f0b676313bc28bfee068e7 Author: Long Li <[email protected]> net: mana: support concurrent HWC requests with proper synchronization This patch replaces the depth-1 semaphore in the HWC with a waitqueue and bitmap scheme to allow multiple in-flight requests. It introduces per-slot synchronization, queue locks, and active sender tracking to serialize channel teardown against concurrent requests. > diff --git a/drivers/net/ethernet/microsoft/mana/gdma_main.c > b/drivers/net/ethernet/microsoft/mana/gdma_main.c > --- a/drivers/net/ethernet/microsoft/mana/gdma_main.c > +++ b/drivers/net/ethernet/microsoft/mana/gdma_main.c [ ... ] > @@ -367,9 +371,25 @@ static int mana_gd_detect_devices(struct pci_dev *pdev) > int mana_gd_send_request(struct gdma_context *gc, u32 req_len, const void > *req, > u32 resp_len, void *resp) > { > - struct hw_channel_context *hwc = gc->hwc.driver_data; > + struct hw_channel_context *hwc; > + unsigned long flags; > + int err; > + > + spin_lock_irqsave(&gc->hwc_lock, flags); > + hwc = gc->hwc.driver_data; > + if (!hwc) { > + spin_unlock_irqrestore(&gc->hwc_lock, flags); > + return -ENODEV; > + } > + atomic_inc(&hwc->active_senders); > + spin_unlock_irqrestore(&gc->hwc_lock, flags); > + > + err = mana_hwc_send_request(hwc, req_len, req, resp_len, resp); > > - return mana_hwc_send_request(hwc, req_len, req, resp_len, resp); > + if (atomic_dec_and_test(&hwc->active_senders)) > + wake_up(&gc->hwc_drain_waitq); [Severity: High] Could this cause a use-after-free of the gdma_context (gc) structure? If atomic_dec_and_test() drops the last reference and returns true, it immediately unblocks the wait_event() in mana_hwc_destroy_channel(). The teardown thread can then proceed and the caller (mana_gd_remove) might free gc before this sender thread executes the wake_up() call on gc->hwc_drain_waitq. > + return err; > } > diff --git a/drivers/net/ethernet/microsoft/mana/hw_channel.c > b/drivers/net/ethernet/microsoft/mana/hw_channel.c > --- a/drivers/net/ethernet/microsoft/mana/hw_channel.c > +++ b/drivers/net/ethernet/microsoft/mana/hw_channel.c [ ... ] > @@ -950,11 +1060,49 @@ void mana_hwc_destroy_channel(struct gdma_context *gc) > hwc->setup_active = false; > } > > + /* After SMC teardown, no more hardware events should arrive. > + * Force-complete any remaining in-flight senders so they can > + * exit and drop their refs. > + */ > + if (hwc->caller_ctx) { [ ... ] > + } > + > + /* Wait for all sender threads to finish and drop their refs. > + * After this, only slots held by timed-out senders whose > + * handle_resp() never ran remain in the bitmap. > + */ > + wait_event(gc->hwc_drain_waitq, > + atomic_read(&hwc->active_senders) == 0); [Severity: High] What happens if the function level reset (FLR) fails during teardown? Looking at mana_hwc_destroy_channel(), if pcie_flr() fails, it prints a warning and returns early from the function. Because this early return bypasses the newly added wait_event() logic above, any active sender threads are abandoned. When mana_gd_remove() subsequently frees gc, wouldn't these stranded senders cause a use-after-free when they eventually time out and access the freed gc->hwc_drain_waitq? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=6
