Re: [PATCH net 6/7] net: bcm4908_enet: publish the buffer address before OWN

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] The comment this patch adds in bcm4908_enet_dma_alloc_rx_buf()
  (drivers/net/ethernet/broadcom/bcm4908_enet.c, lines 292-295) says the…

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but it is in the
  function the patch edits and is still present at the end of the series.

--

net: bcm4908_enet: publish the buffer address before OWN

This patch changes the RX refill in bcm4908_enet_dma_alloc_rx_buf() so the
buffer address is written before the control word that carries
DMA_CTL_STATUS_OWN. It adds a dma_wmb() between the two stores on both the
RX refill path and the TX path. It also adds a dma_rmb() in
bcm4908_enet_poll_rx() after DMA_CTL_STATUS_OWN is seen clear.

> diff --git a/drivers/net/ethernet/broadcom/bcm4908_enet.c b/drivers/net/ethernet/broadcom/bcm4908_enet.c
> index c0aca68010c3b..ea4a9eb98097c 100644
> --- a/drivers/net/ethernet/broadcom/bcm4908_enet.c
> +++ b/drivers/net/ethernet/broadcom/bcm4908_enet.c
> @@ -289,8 +289,13 @@ static int bcm4908_enet_dma_alloc_rx_buf(struct bcm4908_enet *enet, unsigned int
>  	tmp |= DMA_CTL_STATUS_OWN;
>  	if (idx == enet->rx_ring.length - 1)
>  		tmp |= DMA_CTL_STATUS_WRAP;
> -	buf_desc->ctl = cpu_to_le32(tmp);
> +	/* The DMA must not see OWN before the new buffer address: with a full
> +	 * ring it waits on this very descriptor and would fill the old buffer,
> +	 * which is on its way up the stack as an skb.
> +	 */

[Severity: Low]
This isn't a bug, but does this comment match what the rest of the driver
assumes about a full ring? The commit message makes the same claim:

  "which is where the DMA waits once the ring is full"

The end of bcm4908_enet_poll_rx() assumes the opposite for the same case:

	/* Hardware could disable ring if it run out of descriptors */
	bcm4908_enet_dma_rx_ring_enable(enet, &enet->rx_ring);

If the channel is disabled when it runs out of descriptors, it only
restarts through the writel() in bcm4908_enet_dma_rx_ring_enable(). That
write comes after the descriptor stores, so the window described here
would not open.

The new addr -> dma_wmb() -> ctl ordering looks correct under either
model. Could the new comment and the commit message be brought in line
with the existing comment, or hedged, so a reader can tell which hardware
behaviour applies?

>  	buf_desc->addr = cpu_to_le32(slot->dma_addr);
> +	dma_wmb();
> +	buf_desc->ctl = cpu_to_le32(tmp);
>  
>  	return 0;
>  }

[ ... ]

> @@ -638,6 +645,8 @@ static int bcm4908_enet_poll_rx(struct napi_struct *napi, int weight)
>  		ctl = le32_to_cpu(buf_desc->ctl);
>  		if (ctl & DMA_CTL_STATUS_OWN)
>  			break;
> +		/* Nothing the DMA wrote may be read before it gave the descriptor back */
> +		dma_rmb();
>  
>  		slot = enet->rx_ring.slots[enet->rx_ring.read_idx];
>  

[Severity: High]
This is a pre-existing issue, not one introduced by this patch, but it is
in the refill path this patch touches. What happens to the old RX buffer
when bcm4908_enet_dma_alloc_rx_buf() fails?

The refill writes into the live slot before it knows the refill will
succeed:

bcm4908_enet_dma_alloc_rx_buf() {
	slot->buf = napi_alloc_frag(ENET_RX_SKB_BUF_ALLOC_SIZE);
	if (!slot->buf)
		return -ENOMEM;
	...
	if (err) {
		...
		skb_free_frag(slot->buf);
		slot->buf = NULL;
		return err;
	}
	...
}

bcm4908_enet_poll_rx() keeps the old buffer only in its local copy:

	slot = enet->rx_ring.slots[enet->rx_ring.read_idx];

	/* Provide new buffer before unpinning the old one */
	err = bcm4908_enet_dma_alloc_rx_buf(enet, enet->rx_ring.read_idx);
	if (err)
		break;

After that break, slots[read_idx].buf is NULL. Its dma_addr holds either
the old mapping or the mapping error cookie. The filled, still-mapped old
buffer is leaked, and read_idx does not move. The descriptor still has
DMA_CTL_STATUS_OWN clear, and ctl still holds the old length and
SOP/EOP bits.

The next poll stops on the same index, now with slot.buf == NULL. The
refill succeeds this time, and the length and SOP/EOP check passes on the
stale ctl, so we get:

bcm4908_enet_poll_rx()
  dma_unmap_single(dev, slot.dma_addr, ...)   <- stale or error dma_addr
  build_skb(NULL, ENET_RX_SKB_BUF_ALLOC_SIZE)
    __build_skb_around()
      __finalize_skb_around()                 <- writes shinfo at NULL + size

The bad-frame path calls skb_free_frag(slot.buf) on NULL instead. Could
this oops in NAPI softirq context when GFP_ATOMIC allocations fail or DMA
mappings run out?

bcm4908_enet_dma_uninit() also skips slots with buf == NULL:

	if (!slot->buf)
		continue;

so the leaked frag and its mapping are never freed. The code looks the
same at the end of the series.

Would it work to allocate and map into local variables, and only write
them to the slot and the descriptor once both steps succeed? On failure,
poll_rx() could give the old buffer back to the hardware, or drop the
frame and keep the slot intact.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261109235700.2586821-1-kudzo51%40gmail.com