Re: [PATCH net v3] net: bgmac: fix TX DMA unmapping and slot cleanup

[Simon Horman <horms@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
Full review at: https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261022215235.3692083-1-rosenp@gmail.com?part=1
---
net: bgmac: fix TX DMA unmapping and slot cleanup

This patch fixes the bgmac_dma_tx_add() error path so that the head and
each mapped fragment are unmapped from their own slots. It also clears
dma_addr on every slot the failed packet touched. In
bgmac_dma_tx_ring_free() it picks the unmap function from the SOF flag
of the descriptor, unmaps before freeing, and clears slot->skb and
slot->dma_addr.

> This was found through code inspection by an LLM while hitting other
> bugs on an Asus RT-N16. It did not hit these ones specifically.
>
> Fixes: 9cde94506eac ("bgmac: implement scatter/gather support")
> Signed-off-by: Rosen Penev <rosenp@gmail.com>

[Severity: Low]
This isn't a bug, but the commit message says an LLM found these
problems. Should the trailers include an Assisted-by: tag?

Documentation/process/submitting-patches.rst says:

  If you used any sort of advanced coding tool in the creation of your
  patch, you need to acknowledge that use by adding an Assisted-by tag.

Documentation/process/coding-assistants.rst gives the format as
"Assisted-by: LLM [TOOL1] [TOOL2]". Step 6 of its procedure for finding
and fixing bugs also asks for an Assisted-by tag.

> diff --git a/drivers/net/ethernet/broadcom/bgmac.c b/drivers/net/ethernet/broadcom/bgmac.c
> index 3e9c57196a397..d44ee8ed32459 100644
> --- a/drivers/net/ethernet/broadcom/bgmac.c
> +++ b/drivers/net/ethernet/broadcom/bgmac.c

[ ... ]

> @@ -535,17 +540,19 @@ static void bgmac_dma_tx_ring_free(struct bgmac *bgmac,
>  		unsigned int len = ctl1 & BGMAC_DESC_CTL1_LEN;
>  
>  		slot = &ring->slots[i];
> -		dev_kfree_skb(slot->skb);
>  
> -		if (!slot->dma_addr)
> -			continue;
> +		if (slot->dma_addr) {
> +			if (le32_to_cpu(dma_desc[i].ctl0) & BGMAC_DESC_CTL0_SOF)
> +				dma_unmap_single(dma_dev, slot->dma_addr,
> +						 len, DMA_TO_DEVICE);
> +			else
> +				dma_unmap_page(dma_dev, slot->dma_addr,
> +					       len, DMA_TO_DEVICE);
> +			slot->dma_addr = 0;
> +		}
>  
> -		if (slot->skb)
> -			dma_unmap_single(dma_dev, slot->dma_addr,
> -					 len, DMA_TO_DEVICE);
> -		else
> -			dma_unmap_page(dma_dev, slot->dma_addr,
> -				       len, DMA_TO_DEVICE);
> +		dev_kfree_skb(slot->skb);
> +		slot->skb = NULL;

[Severity: Low]
If a pending packet wraps around the end of the ring, is the skb still
freed before all of its slots are unmapped?

The commit message says bgmac_dma_tx_ring_free() will "unmap before
freeing". That ordering only holds within a single slot, because the
loop walks slots in physical order from 0 to BGMAC_TX_RING_SLOTS - 1.

bgmac_dma_tx_add() puts fragments in consecutive slots modulo the ring
size, and sets slot->skb only on the last one:

drivers/net/ethernet/broadcom/bgmac.c:bgmac_dma_tx_add() {
    ...
		index = (index + 1) % BGMAC_TX_RING_SLOTS;
		slot = &ring->slots[index];
    ...
	slot->skb = skb;
    ...
}

Take a scatter/gather packet whose head (SOF) is in the last slot of the
ring and whose last fragment, which owns the skb, is in slot 0. The loop
reaches i == 0 first and calls dev_kfree_skb(), which releases skb->head
and the frag pages. Only later, at i == BGMAC_TX_RING_SLOTS - 1, does it
call dma_unmap_single() on the head buffer, which is already freed.

bgmac_stop() and the suspend path call bgmac_chip_reset() before
bgmac_dma_cleanup(), and the mappings are DMA_TO_DEVICE. So this may
have no visible effect, but it still breaks the DMA API rule that a
buffer is unmapped before it is freed.

bgmac_dma_tx_free() avoids this by walking from ring->start to
ring->end. Could the same be done here? Another option is to unmap every
slot in a first pass and free the skbs in a second pass.

>  	}
>  }

[Severity: High]
This is a pre-existing issue and was not introduced by this patch. When
request_irq() fails in bgmac_open(), are the RX buffers freed while the
hardware is still running?

drivers/net/ethernet/broadcom/bgmac.c:bgmac_open() {
	bgmac_chip_reset(bgmac);

	err = bgmac_dma_init(bgmac);
	...
	bgmac_chip_init(bgmac);

	err = request_irq(bgmac->irq, bgmac_interrupt, IRQF_SHARED,
			  net_dev->name, net_dev);
	if (err < 0) {
		dev_err(bgmac->dev, "IRQ request error: %d!\n", err);
		bgmac_dma_cleanup(bgmac);
		return err;
	}
    ...
}

By the time request_irq() fails, bgmac_dma_init() has enabled the RX DMA
engine, attached a mapped page to every RX descriptor and written
RX_INDEX. bgmac_chip_init() has also unmasked device interrupts with
bgmac_chip_intrs_on() and enabled the MAC with bgmac_enable().

bgmac_dma_cleanup() then unmaps and frees every RX buffer through
bgmac_dma_rx_ring_free(), and nothing stops the engine first.

Could a frame received before the next bgmac_chip_reset() be DMA'd into
pages that are already freed? The device interrupt also stays unmasked
on an IRQF_SHARED line with no bgmac handler registered.

bgmac_stop() quiesces the hardware before cleanup:

drivers/net/ethernet/broadcom/bgmac.c:bgmac_stop() {
    ...
	bgmac_chip_intrs_off(bgmac);
	free_irq(bgmac->irq, net_dev);

	bgmac_chip_reset(bgmac);
	bgmac_dma_cleanup(bgmac);
    ...
}

Should the bgmac_open() error path also call bgmac_chip_intrs_off() and
bgmac_chip_reset() before bgmac_dma_cleanup()?