[PATCH net] net: bgmac: keep the maximum RX frame within one RX buffer

[PATCH net] net: bgmac: keep the maximum RX frame within one RX buffer

[Rosen Penev]

Each RX descriptor holds at most BGMAC_RX_MAX_FRAME_SIZE (1536) bytes of
frame data. The MAC, however, is programmed to accept up to
32 + ETHER_MAX_LEN = 1550 bytes in bgmac_chip_init(), and up to
32 + mtu = 1564 bytes after bgmac_change_mtu(). max_mtu is set to
BGMAC_RX_MAX_FRAME_SIZE - ETH_FCS_LEN, which leaves out the Ethernet
header and any VLAN tag, so a frame at max_mtu does not fit in one buffer
either.

The DMA engine is the same dma64 engine used by b43 and brcmsmac, which
continue a frame that does not fit into the next descriptor's buffer.
bgmac_dma_rx_read() does not handle that: it consumes only the first
descriptor and parses the continuation buffer as a new frame, taking its
RX header from frame data the sender controls.

Program UMAC_MAX_FRAME_LEN from the MTU plus a VLAN tagged Ethernet header
and the FCS, as bcmgenet does for the same UniMAC register, and limit
max_mtu so that the resulting frame fits in one RX buffer. At the default
MTU of 1500 this gives 1522 bytes. A DSA conduit carrying a 4 byte
Broadcom tag (MTU 1504) gives 1526 bytes. max_mtu drops from 1532 to 1514.

bgmac_chip_init() runs on every ndo_open and used to reset the limit to
the fixed default. Use the current MTU instead.

bgmac_change_mtu() also never updated net_dev->mtu. When a driver
provides ndo_change_mtu, the core leaves that to the driver, so the MTU
seen by the stack never changed. Set it.

Only compile tested. Not tested on hardware. Found with a review on a
different commit by Sashiko.

Fixes: dd4544f05469 ("bgmac: driver for GBit MAC core on BCMA bus")
Fixes: 752e5fcc2e77 ("bgmac: reduce max frame size to support just MTU 1500")
Signed-off-by: Rosen Penev <rosenp@gmail.com>
---
 drivers/net/ethernet/broadcom/bgmac.c | 15 +++++++++++----
 drivers/net/ethernet/broadcom/bgmac.h |  6 ++++--
 2 files changed, 15 insertions(+), 6 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bgmac.c b/drivers/net/ethernet/broadcom/bgmac.c
index 669eb7ce7d40..f29568a4190a 100644
--- a/drivers/net/ethernet/broadcom/bgmac.c
+++ b/drivers/net/ethernet/broadcom/bgmac.c
@@ -1110,6 +1110,13 @@ static void bgmac_enable(struct bgmac *bgmac)
 	}
 }
 
+/* The MAC counts the FCS in the maximum frame length. */
+static void bgmac_set_max_frame_len(struct bgmac *bgmac, int mtu)
+{
+	bgmac_umac_write(bgmac, UMAC_MAX_FRAME_LEN,
+			 mtu + VLAN_ETH_HLEN + ETH_FCS_LEN);
+}
+
 /* http://bcm-v4.sipsolutions.net/mac-gbit/gmac/chipinit */
 static void bgmac_chip_init(struct bgmac *bgmac)
 {
@@ -1131,7 +1138,7 @@ static void bgmac_chip_init(struct bgmac *bgmac)
 	else
 		bgmac_umac_cmd_maskset(bgmac, ~CMD_LCL_LOOP_EN, 0, false);
 
-	bgmac_umac_write(bgmac, UMAC_MAX_FRAME_LEN, 32 + ETHER_MAX_LEN);
+	bgmac_set_max_frame_len(bgmac, bgmac->net_dev->mtu);
 
 	bgmac_chip_intrs_on(bgmac);
 
@@ -1267,7 +1274,8 @@ static int bgmac_change_mtu(struct net_device *net_dev, int mtu)
 {
 	struct bgmac *bgmac = netdev_priv(net_dev);
 
-	bgmac_umac_write(bgmac, UMAC_MAX_FRAME_LEN, 32 + mtu);
+	bgmac_set_max_frame_len(bgmac, mtu);
+	WRITE_ONCE(net_dev->mtu, mtu);
 	return 0;
 }
 
@@ -1555,8 +1563,7 @@ int bgmac_enet_probe(struct bgmac *bgmac)
 	net_dev->hw_features = net_dev->features;
 	net_dev->vlan_features = net_dev->features;
 
-	/* Omit FCS from max MTU size */
-	net_dev->max_mtu = BGMAC_RX_MAX_FRAME_SIZE - ETH_FCS_LEN;
+	net_dev->max_mtu = BGMAC_RX_MAX_MTU;
 
 	bgmac->in_init = false;
 
diff --git a/drivers/net/ethernet/broadcom/bgmac.h b/drivers/net/ethernet/broadcom/bgmac.h
index 6fee9a41839c..ace65789289f 100644
--- a/drivers/net/ethernet/broadcom/bgmac.h
+++ b/drivers/net/ethernet/broadcom/bgmac.h
@@ -2,6 +2,7 @@
 #ifndef _BGMAC_H
 #define _BGMAC_H
 
+#include <linux/if_vlan.h>
 #include <linux/netdevice.h>
 
 #include "unimac.h"
@@ -330,6 +331,9 @@
 						 BGMAC_RX_FRAME_OFFSET)
 #define BGMAC_RX_MAX_FRAME_SIZE			1536
 #define BGMAC_RX_BUF_SIZE			(BGMAC_RX_FRAME_OFFSET + BGMAC_RX_MAX_FRAME_SIZE)
+/* Largest MTU whose VLAN tagged frames, FCS included, fit in one RX buffer */
+#define BGMAC_RX_MAX_MTU			(BGMAC_RX_MAX_FRAME_SIZE - VLAN_ETH_HLEN - \
+						 ETH_FCS_LEN)
 #define BGMAC_RX_ALLOC_SIZE			(SKB_DATA_ALIGN(BGMAC_RX_BUF_SIZE + BGMAC_RX_BUF_OFFSET) + \
 						 SKB_DATA_ALIGN(sizeof(struct skb_shared_info)))
 
@@ -363,8 +367,6 @@
 #define BGMAC_CHIPCTL_7_IF_TYPE_MII		0x00000040
 #define BGMAC_CHIPCTL_7_IF_TYPE_RGMII		0x00000080
 
-#define ETHER_MAX_LEN	(ETH_FRAME_LEN + ETH_FCS_LEN)
-
 /* Feature Flags */
 #define BGMAC_FEAT_TX_MASK_SETUP	BIT(0)
 #define BGMAC_FEAT_RX_MASK_SETUP	BIT(1)
-- 
2.56.0

Re: [PATCH net] net: bgmac: keep the maximum RX frame within one RX buffer

[netdev-bot+sashiko]

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

Critical: 0 · High: 1 · Medium: 0 · Low: 1

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).

- [High] Incomplete fix: the patch tries to close the case where
  bgmac_dma_rx_read() reads a dma64 continuation buffer as a new frame…
- [Low] The commit message says UMAC_MAX_FRAME_LEN is programmed 'from
  the MTU plus a VLAN tagged Ethernet header and the FCS, as bcmgenet…

--

net: bgmac: keep the maximum RX frame within one RX buffer

This patch sets UMAC_MAX_FRAME_LEN to the MTU plus a VLAN tagged
Ethernet header and the FCS. It also lowers max_mtu to BGMAC_RX_MAX_MTU
(1514), so that the largest frame the MAC accepts fits in one 1536 byte
RX buffer. In addition, bgmac_chip_init() now uses the current MTU, and
bgmac_change_mtu() now updates net_dev->mtu.

> Program UMAC_MAX_FRAME_LEN from the MTU plus a VLAN tagged Ethernet header
> and the FCS, as bcmgenet does for the same UniMAC register, and limit
> max_mtu so that the resulting frame fits in one RX buffer.

[Severity: Low]
Is the comparison to bcmgenet accurate? init_umac() in bcmgenet.c writes
a fixed value that does not depend on the MTU:

    bcmgenet_umac_writel(priv, ENET_MAX_MTU_SIZE, UMAC_MAX_FRAME_LEN);

bcmgenet.h defines that value as:

#define ENET_MAX_MTU_SIZE	(ETH_DATA_LEN + ETH_HLEN + VLAN_HLEN + \
				 ENET_BRCM_TAG_LEN + ETH_FCS_LEN + ENET_PAD)

That adds up to 1536. It includes room for a 6 byte Broadcom tag and 8
bytes of padding.

bcm4908_enet_set_mtu() does compute the limit from the MTU, but it uses
mtu + ENET_MAX_ETH_OVERHEAD (32), which also leaves room for tags.

Could the commit message stop citing bcmgenet as the precedent for the
mtu + VLAN_ETH_HLEN + ETH_FCS_LEN formula?

> diff --git a/drivers/net/ethernet/broadcom/bgmac.c b/drivers/net/ethernet/broadcom/bgmac.c
> index 669eb7ce7d40b..f29568a4190a8 100644
> --- a/drivers/net/ethernet/broadcom/bgmac.c
> +++ b/drivers/net/ethernet/broadcom/bgmac.c
> @@ -1110,6 +1110,13 @@ static void bgmac_enable(struct bgmac *bgmac)
>  	}
>  }
>  
> +/* The MAC counts the FCS in the maximum frame length. */
> +static void bgmac_set_max_frame_len(struct bgmac *bgmac, int mtu)
> +{
> +	bgmac_umac_write(bgmac, UMAC_MAX_FRAME_LEN,
> +			 mtu + VLAN_ETH_HLEN + ETH_FCS_LEN);
> +}

[Severity: High]
Does this register write fully close the continuation buffer problem
described in the commit message? The RX side that parses the header is
unchanged.

bgmac_dma_rx_enable() still sets the overflow continue bit. Any frame
longer than BGMAC_RX_MAX_FRAME_SIZE will still spill into the next
descriptor:

    ctl |= BGMAC_DMA_RX_OVERFLOW_CONT;

bgmac_chip_reset() puts CMD_RX_ERR_DISC in the clear mask and sets
CMD_NO_LEN_CHK. So the MAC is not set up to discard errored or
over-length frames before DMA.

bcmgenet_desc_rx() uses the same UniMAC. It receives over-length frames
with DMA_RX_LG set and drops them in software. That suggests the MAC
flags oversize frames instead of always discarding them.

At max_mtu the new limit is 1514 + 18 + 4 = 1536, exactly
BGMAC_RX_MAX_FRAME_SIZE. That leaves no margin for a stacked VLAN tag or
for any extra length the MAC allows.

If a frame longer than 1536 bytes does reach DMA, bgmac_dma_rx_read()
reads the next slot's rx->len and rx->flags from bytes the sender
controls. These are the checks it then relies on:

    if (len > BGMAC_RX_ALLOC_SIZE) {
    ...
    /* Omit CRC. */
    len -= ETH_FCS_LEN;
    ...
    skb_put(skb, BGMAC_RX_FRAME_OFFSET +
            BGMAC_RX_BUF_OFFSET + len);

BGMAC_RX_ALLOC_SIZE includes the alignment padding and skb_shared_info.
That is roughly 1.9 to 2 KB, well above the 1536 bytes the DMA can write
per slot. rx->flags is never checked.

There is also no lower bound. With u16 len, an rx->len of 0..3 wraps to
about 65532 and hits skb_over_panic(). With 64 byte cachelines:

  - a len above about 1602 overruns skb->end, which is also a BUG()
  - a len of 1541..1602 passes stale netdev_alloc_frag bytes up the stack

Other drivers check this in software. b44_rx() bounds the length by the
real buffer size and rejects error flags:

    if ((len > (RX_PKT_BUF_SZ - RX_PKT_OFFSET)) ||
        (rh->flags & cpu_to_le16(RX_FLAG_ERRORS))) {

brcmsmac dma_rx() drains continuation buffers.

The patch is noted as compile tested only. Would it be safer to also
validate in bgmac_dma_rx_read()? For example:

  - drop frames with len > BGMAC_RX_MAX_FRAME_SIZE or
    len < ETH_HLEN + ETH_FCS_LEN
  - check the error and oversize flags
  - skip continuation descriptors, or stop setting
    BGMAC_DMA_RX_OVERFLOW_CONT

[ ... ]

> diff --git a/drivers/net/ethernet/broadcom/bgmac.h b/drivers/net/ethernet/broadcom/bgmac.h
> index 6fee9a41839c0..ace65789289fc 100644
> --- a/drivers/net/ethernet/broadcom/bgmac.h
> +++ b/drivers/net/ethernet/broadcom/bgmac.h

[ ... ]

> @@ -330,6 +331,9 @@
>  						 BGMAC_RX_FRAME_OFFSET)
>  #define BGMAC_RX_MAX_FRAME_SIZE			1536
>  #define BGMAC_RX_BUF_SIZE			(BGMAC_RX_FRAME_OFFSET + BGMAC_RX_MAX_FRAME_SIZE)
> +/* Largest MTU whose VLAN tagged frames, FCS included, fit in one RX buffer */
> +#define BGMAC_RX_MAX_MTU			(BGMAC_RX_MAX_FRAME_SIZE - VLAN_ETH_HLEN - \
> +						 ETH_FCS_LEN)
>  #define BGMAC_RX_ALLOC_SIZE			(SKB_DATA_ALIGN(BGMAC_RX_BUF_SIZE + BGMAC_RX_BUF_OFFSET) + \
>  						 SKB_DATA_ALIGN(sizeof(struct skb_shared_info)))

[ ... ]

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