Re: [PATCH net v3 3/6] net: mana: free HWC comp_buf after destroying the EQ

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
net: mana: free HWC comp_buf after destroying the EQ

This moves the kfree(hwc_cq->comp_buf) in mana_hwc_destroy_cq() to after
the CQ and EQ have been destroyed, so a late EQ handler calling
mana_hwc_comp_event() cannot dereference freed memory.  A comment is
added describing the fencing that makes the new order safe.

> diff --git a/drivers/net/ethernet/microsoft/mana/hw_channel.c b/drivers/net/ethernet/microsoft/mana/hw_channel.c
> index cbb56c7647876..d701c427fe47b 100644
> --- a/drivers/net/ethernet/microsoft/mana/hw_channel.c
> +++ b/drivers/net/ethernet/microsoft/mana/hw_channel.c
> @@ -384,14 +384,20 @@ static void mana_hwc_comp_event(void *ctx, struct gdma_queue *q_self)
>  
>  static void mana_hwc_destroy_cq(struct gdma_context *gc, struct hwc_cq *hwc_cq)
>  {
> -	kfree(hwc_cq->comp_buf);
> -
>  	if (hwc_cq->gdma_cq)
>  		mana_gd_destroy_queue(gc, hwc_cq->gdma_cq);
>  
> +	/* comp_buf is reached only by mana_hwc_comp_event(), which the
> +	 * EQ handler invokes via cq_table[id].  The CQ destroy above
> +	 * already cleared that slot and ran synchronize_rcu(), so no
> +	 * handler can reach comp_buf once it returns.  Destroying the EQ
> +	 * here additionally tears down the IRQ (defense in depth) before
> +	 * comp_buf and hwc_cq are freed below.
> +	 */

[Severity: Low]
Does the EQ destroy really tear down the IRQ here?

mana_gd_destroy_queue() for GDMA_EQ calls mana_gd_destroy_eq(), whose whole
body is:

	if (flush_evenets) {
		err = mana_gd_test_eq(gc, queue);
		...
	}

	mana_gd_deregister_irq(queue);

	if (queue->eq.disable_needed)
		mana_gd_disable_queue(queue);

and mana_gd_deregister_irq() only unlinks the EQ from the shared IRQ's
list:

	spin_lock_irqsave(&gic->lock, flags);
	list_for_each_entry_rcu(eq, &gic->eq_list, entry) {
		if (queue == eq) {
			list_del_rcu(&eq->entry);
			break;
		}
	}
	spin_unlock_irqrestore(&gic->lock, flags);

	synchronize_rcu();

There is no free_irq() and no mana_gd_put_gic() on this path; the only
free_irq() in gdma_main.c lives in mana_gd_put_gic(), which the HWC path
reaches from mana_gd_remove_irqs() instead.  The MSI-X line stays
registered and keeps firing for the other EQs sharing it.

So the actual fence appears to be RCU in both steps (the cq_table slot
clear plus synchronize_rcu() in mana_gd_destroy_cq(), and the list_del_rcu()
plus synchronize_rcu() above), not IRQ teardown.

The same wording appears in the commit message:

    Destroy the CQ and EQ first -- the EQ teardown deregisters the IRQ and
    fences in-flight handlers -- then free comp_buf and hwc_cq.

Could the comment and the changelog be reworded to say the EQ is
deregistered from the shared IRQ's RCU list, rather than that the IRQ is
torn down?  The comment and the changelog also disagree on which step is
load bearing: the comment treats the EQ destroy as "defense in depth"
while the changelog presents it as the mechanism that fences handlers.

> +
>  	if (hwc_cq->gdma_eq)
>  		mana_gd_destroy_queue(gc, hwc_cq->gdma_eq);
>  
> +	kfree(hwc_cq->comp_buf);
>  	kfree(hwc_cq);
>  }
>  

[Severity: Medium]
This isn't a bug introduced by this patch, but the same window the patch
closes for comp_buf still exists for hwc->rxq and hwc->txq, so the fix
looks asymmetric.  In mana_hwc_destroy_channel() the work queues are
freed before the CQ/EQ teardown that does the cq_table clear and the
synchronize_rcu() calls:

	if (hwc->txq)
		mana_hwc_destroy_wq(hwc, hwc->txq);

	if (hwc->rxq)
		mana_hwc_destroy_wq(hwc, hwc->rxq);

	if (hwc->cq)
		mana_hwc_destroy_cq(hwc->gdma_dev->gdma_context, hwc->cq);

mana_hwc_destroy_wq() releases the DMA buffer and the hwc_wq itself with
no fencing, and the GDMA_RQ/GDMA_SQ cases of mana_gd_destroy_queue() are
empty:

	mana_hwc_dealloc_dma_buf(hwc, hwc_wq->msg_buf);

	if (hwc_wq->gdma_wq)
		mana_gd_destroy_queue(hwc->gdma_dev->gdma_context,
				      hwc_wq->gdma_wq);

	kfree(hwc_wq);

In that window mana_hwc_comp_event() is still published in cq_table[id]
and reaches the event handlers with ctx == hwc:

		if (completions[i].is_sq)
			hwc_cq->tx_event_handler(hwc_cq->tx_event_ctx, ...);
		else
			hwc_cq->rx_event_handler(hwc_cq->rx_event_ctx, ...);

and mana_hwc_rx_event_handler() then dereferences the freed rxq:

	if (WARN_ON_ONCE(hwc_rxq->gdma_wq->id != gdma_rxq_id))
		return;

	rq = hwc_rxq->gdma_wq;
	...
	rq_base_addr = hwc_rxq->msg_buf->mem_info.dma_handle;

Can a completion delivered in that window read freed hwc_wq/hwc_dma_buf
memory, and repost into the already dma-unmapped RQ ring via
mana_hwc_post_rx_wqe()?  hwc->rxq and hwc->txq are also left set after
being freed.

For the record, this ordering does get corrected later in the same series
by "net: mana: fix HWC teardown safety with setup_active flag and destroy
ordering", where mana_hwc_destroy_channel() destroys the CQ before the
work queues.  Would it make sense to fold that reordering in here, or note
the dependency, so this patch does not land with only half of the window
closed?