Re: [PATCH net-next v3 6/8] net: bcmgenet: pad transmit frames out of the packet ready window

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 0 · Medium: 1 · 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).

- [Medium] In bcmgenet_xmit()
  (drivers/net/ethernet/broadcom/genet/bcmgenet.c), any frame whose…
- [Low] The commit message describes the transmit stall (Tx DMA will not
  halt, bcmgenet_init_dma() fails, the interface cannot be reopened) as…

--

net: bcmgenet: pad transmit frames out of the packet ready window

This patch picks a lower transmit packet ready threshold for MTUs where
the longest frame would end just past it. It pads frames that end within
64 bytes past the threshold so they clear the window. Frames longer than
the threshold that ask for checksum offload get their checksum computed
in software.

> A frame that ends a few bytes past the transmit packet ready threshold
> stops the transmitter as soon as a shorter frame follows. Tx DMA then
> refuses to halt, so every later bcmgenet_init_dma() fails and the interface
> cannot be opened again.

[Severity: Low]

Can this stall happen at this commit? genet does not set max_mtu or
provide ndo_change_mtu yet, so the MTU stays capped at the ether_setup()
default:

net/ethernet/eth.c:ether_setup() {
    ...
	dev->max_mtu		= ETH_DATA_LEN;
    ...
}

With an MTU of 1500, ENET_MAX_FRAME_LEN(mtu) - ETH_FCS_LEN stays well
below 3840. bcmgenet_tx_pkt_rdy_thld() therefore returns ENET_THLD_MAX
and tx_thld_len is 3840. Neither new branch in bcmgenet_xmit() can then
be reached.

The message does say "Nothing below an MTU of 3809 is affected". The new
code only becomes reachable with the later "net: bcmgenet: allow the MTU
to be changed" patch.

Could the message say that nothing reaches this code until the MTU limit
is raised later in the series?

> diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> index 4a4b0c46f8986..17c8318c20388 100644
> --- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> +++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c

[ ... ]

> @@ -2176,6 +2181,29 @@ static netdev_tx_t bcmgenet_xmit(struct sk_buff *skb, struct net_device *dev)
>  		goto out;
>  	}
>  

[ ... ]

> +	/* Keep the frame out of the window just past the threshold */
> +	if (unlikely(skb->len > priv->tx_thld_len &&
> +		     skb->len < priv->tx_thld_len + ENET_TX_SAFE_MARGIN)) {
> +		if (skb_put_padto(skb, priv->tx_thld_len + ENET_TX_SAFE_MARGIN)) {
> +			BCMGENET_STATS64_INC((&ring->stats64), dropped);
> +			goto drop;
> +		}
> +	}

[Severity: Medium]

What happens here when the GENET is the conduit for a DSA switch that
uses a tail tag? By the time the skb reaches bcmgenet_xmit(), the tagger
has already written the tag into the last bytes of the frame. This
skb_put_padto() then appends zeros after the tag.

The DSA core pads before tagging so that the conduit never has to add
bytes after a tail tag:

net/dsa/user.c:dsa_user_xmit() {
    ...
	if (dev->needed_tailroom)
		eth_skb_pad(skb);
    ...
}

For KSZ9477-class switches the tag holds the egress port mask, the
priority, and the override bit for link-local frames:

net/dsa/tag_ksz.c:ksz9477_xmit() {
    ...
	if (is_link_local_ether_addr(hdr->h_dest))
		val |= KSZ9477_TAIL_TAG_OVERRIDE;

	*tag = cpu_to_be16(val);
    ...
}

lan937x_xmit() also always sets LAN937X_TAIL_TAG_VALID.

After padding, would the switch read an all-zero tag in place of these
fields, with the real tag left inside the payload? HSR/PRP RCT trailers
look like they would be corrupted the same way.

The commit message says these protocols "see the zeros instead". The
switch, though, would read the zeros as forwarding instructions: no port
mask, no override bit and no valid bit. Depending on the switch, the
frame could be dropped, forwarded by address lookup or flooded, whatever
egress port the host chose. That could break standalone port isolation,
and link-local frames would lose their forced egress port.

This path opens up once later patches in the series raise max_mtu. If a
user port MTU is raised above roughly 3806, dsa_user_change_mtu() calls
dev_set_mtu(conduit, new_conduit_mtu), which moves the conduit into the
MTU range that has the window. After that, whoever sends the traffic
decides whether a frame lands in the window, for example through the UDP
datagram size, IP fragment tails, or forwarded traffic.

Instead of padding after the trailer, would it be safer to check
netdev_uses_dsa() here and drop the frame? Another option would be to
refuse conduit MTUs that reach the window.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261007-nb-genet-mtu-nn-v2-v3-0-74a796c019ce%40tipi-net.de