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

[Nicolai Buchwitz <nb@tipi-net.de>]

Hi Clashiko

On 8.10.2026 11:40, netdev-bot+sashiko@kernel.org wrote:

> [...]

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

I do not think genet is ever a DSA conduit with tail tags. At least none
of the boards I'm aware of + what appears in the tree...

Broadcom switches use a head tag, and genet already reserves
ENET_BRCM_TAG_LEN for it, so padding at the tail wont do any harm. On 
the
Broadcom SoCs that do have an integrated switch the conduit is 
SYSTEMPORT,
not genet, and that driver has the netdev_uses_dsa() checks.

Every tagger listed is a tail tagger for a switch family that is not 
paired
with genet. Reaching this would need an out of tree board wiring one 
behind
a genet SoC and raising the user port MTU past 3808 (not 3806)...

So I would keep the padding and the note in the commit message rather 
than
add a drop path for a configuration that does not exist?

The only other way (which I don't really like), would be to cap the MTU 
when
attached to dsa in _change_mtu():

	/* Padding a frame clear of the window would overwrite a DSA tail tag 
*/
	if (netdev_uses_dsa(dev) && new_mtu > ENET_MAX_PAD_FREE_MTU)
		return -EINVAL;

But if we do this, Clashiko would complain about the case where the dsa 
is
attached after the MTU is already set to something above the thresholds 
...

@Florian / Justin: Anything you are aware of in the stb universe?

Thanks,
Nicolai