Re: [PATCH net-next 6/7] net: mana: support concurrent HWC requests with proper synchronization

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