Re: [PATCH net 7/7] net: bcm4908_enet: drop RX frames longer than the buffer

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider.

Critical: 0 · High: 0 (1) · Medium: 2 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] The new check in bcm4908_enet_poll_rx()
  (drivers/net/ethernet/broadcom/bcm4908_enet.c:664-671) treats a…
- [Medium] The new branch in bcm4908_enet_poll_rx()
  (bcm4908_enet.c:668-670) increments netdev->stats.rx_length_errors and…

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but the RX refill
  failure path in bcm4908_enet_poll_rx() loses the old buffer and later…

--

net: bcm4908_enet: drop RX frames longer than the buffer

This patch adds a check to bcm4908_enet_poll_rx() so that RX descriptors
reporting a length larger than ENET_RX_BUF_DMA_SIZE are dropped instead
of being passed to skb_put(). Such frames are counted in rx_length_errors
and rx_dropped. The commit message says this was seen only with the RX
channel MAX_BURST raised to 32.

> diff --git a/drivers/net/ethernet/broadcom/bcm4908_enet.c b/drivers/net/ethernet/broadcom/bcm4908_enet.c
> index ea4a9eb98097c..b2debdfa0902e 100644
> --- a/drivers/net/ethernet/broadcom/bcm4908_enet.c
> +++ b/drivers/net/ethernet/broadcom/bcm4908_enet.c
> @@ -660,10 +660,13 @@ static int bcm4908_enet_poll_rx(struct napi_struct *napi, int weight)
>  
>  		len = (ctl & DMA_CTL_LEN_DESC_BUFLENGTH) >> DMA_CTL_LEN_DESC_BUFLENGTH_SHIFT;
>  
> -		if (len < ETH_ZLEN ||
> +		/* A length past the buffer would make skb_put() panic the kernel */
> +		if (len < ETH_ZLEN || len > ENET_RX_BUF_DMA_SIZE ||

[Severity: Medium]
On a mainline kernel, can the device ever report a length like this?

bcm4908_enet_dma_alloc_rx_buf() maps exactly ENET_RX_BUF_DMA_SIZE (1532)
bytes and puts that size in the descriptor:

    tmp = ENET_RX_BUF_DMA_SIZE << DMA_CTL_LEN_DESC_BUFLENGTH_SHIFT;

bcm4908_enet_set_mtu() programs mtu + ENET_MAX_ETH_OVERHEAD, which is
also 1532 at ENET_MTU_MAX:

    enet_umac_write(enet, UMAC_MAX_FRAME_LEN, mtu + ENET_MAX_ETH_OVERHEAD);

bcm4908_enet_dma_ring_init() only ever writes ENET_DMA_MAX_BURST_LEN (8)
to ENET_DMA_CH_CFG_MAX_BURST.

The commit message says:

    It was seen on a BCM4908 with the RX channel's MAX_BURST raised to
    32, where the DMA reported a length of 3388 bytes, more than twice
    the size of the buffer; with the default burst length it has not
    been seen.

Given that, does the Fixes: tag (and the stable backport it implies) fit
here? The triggering configuration is not one mainline uses.

There is also the question of what the hardware actually did. If the
DMA wrote the 3388 bytes it reported into a 1532-byte mapping, then the
skb_shared_info area of the frag, and part of the next napi_alloc_frag()
allocation, were overwritten before the CPU saw the descriptor.

Before this patch, skb_over_panic() stopped the system at that point.
With this patch the frame is dropped silently. There is no WARN or log
message, the channel is not reset, and bcm4908_enet_dma_rx_ring_enable()
re-enables the ring with the same configuration.

Did the hardware really overrun the buffer, or did it only misreport the
length? If an overrun is possible, would a WARN_ONCE() or a ratelimited
netdev_err() be better than a silent drop?

>  		    (ctl & (DMA_CTL_STATUS_SOP | DMA_CTL_STATUS_EOP)) != (DMA_CTL_STATUS_SOP | DMA_CTL_STATUS_EOP)) {
>  			dma_unmap_single(dev, slot.dma_addr, ENET_RX_BUF_DMA_SIZE, DMA_FROM_DEVICE);
>  			skb_free_frag(slot.buf);

[Severity: High]
This isn't a bug introduced by this patch, but slot.buf can be NULL
when this path runs, after an earlier RX refill failure.

bcm4908_enet_poll_rx() copies the ring slot by value and then refills
the same index:

    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;

bcm4908_enet_dma_alloc_rx_buf() writes straight into the ring slot
before it knows whether the allocation worked:

    slot->buf = napi_alloc_frag(ENET_RX_SKB_BUF_ALLOC_SIZE);
    if (!slot->buf)
        return -ENOMEM;

If napi_alloc_frag() fails, the ring slot now holds buf == NULL. The
only reference to the old, still-mapped frag was in the local copy,
which is discarded on the break.

On a dma_mapping_error(), slot->dma_addr is overwritten with the error
cookie and slot->buf is set to NULL, so the old streaming mapping is
leaked as well.

read_idx is not advanced, and the descriptor still has OWN clear. The
next poll therefore handles the same index with slot.buf == NULL. Once a
refill succeeds:

    bcm4908_enet_poll_rx()
      build_skb(NULL, ENET_RX_SKB_BUF_ALLOC_SIZE)
        __build_skb_around()
          __finalize_skb_around()
            memset(shinfo, ...)   <- NULL + size

Does this oops in NAPI softirq context? On the bad-frame path, including
the new len > ENET_RX_BUF_DMA_SIZE branch, the code calls
skb_free_frag(NULL) instead, and after a mapping error it calls
dma_unmap_single() on the error cookie.

bcm4908_enet_dma_uninit() skips slots with buf == NULL, so the leaked
frag and its mapping are never released at teardown either.

Could bcm4908_enet_dma_alloc_rx_buf() allocate and map into local
variables, and update the slot only on success?

> +			if (len > ENET_RX_BUF_DMA_SIZE)
> +				enet->netdev->stats.rx_length_errors++;
>  			enet->netdev->stats.rx_dropped++;

[Severity: Medium]
Should rx_errors be incremented here as well?

The struct rtnl_link_stats64 kernel-doc in include/uapi/linux/if_link.h
says:

    @rx_errors: Total number of bad packets received on this network
    device. This counter must include events counted by
    @rx_length_errors, ...

This driver never touches rx_errors, and bcm4908_enet_netdev_ops has no
ndo_get_stats64. Userspace therefore sees the raw netdev->stats values.
After one oversized frame, ip -s link would show a non-zero frame count
with zero errors.

The same frame is also counted in rx_dropped, which the uAPI describes
as packets received but not processed, for example because of a lack of
resources.

For comparison, bcmsysport.c and bgmac.c increment rx_length_errors
together with rx_errors, and do not also count the frame in rx_dropped.

>  			break;
>  		}

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