Re: [PATCH net-next v4 1/5] bnxt_en: let the stack segment GSO skbs with too many frags

[netdev-bot+sashiko]

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] This commit fixes two real bnxt TX bugs but has no `Fixes:` tag
  and no `Cc: stable`.

--

bnxt_en: let the stack segment GSO skbs with too many frags

This commit clears the GSO features in bnxt_features_check() for skbs
with more than TX_MAX_FRAGS frags when MAX_SKB_FRAGS > TX_MAX_FRAGS. The
stack then segments those skbs instead of bnxt_start_xmit() linearizing
them. Non-GSO skbs are still linearized in the xmit path.

Link: https://lore.kernel.org/all/CANn89i+ps_CozfOyRnKav0_LUmSekJ9ExB5JkDbTAVf_ss_98g@xxxxxxxxxxxxxx/

> With MAX_SKB_FRAGS > TX_MAX_FRAGS, the xmit path linearizes skbs with
> more frags than a packet can have BDs. For GSO skbs, linearizing needs a
> large allocation, can produce a head which doesn't fit in a BD with BIG
> TCP, and always fails for unreadable skbs. Clear the GSO features of
> such skbs in ndo_features_check instead, so that the stack segments
> them. Non-GSO skbs are still linearized.
>
> Suggested-by: Eric Dumazet <edumazet@xxxxxxxxxx>
> Link: https://lore.kernel.org/all/CANn89i+ps_CozfOyRnKav0_LUmSekJ9ExB5JkDbTAVf_ss_98g@xxxxxxxxxxxxxx/
> Signed-off-by: Stanislav Fomichev <sdf@xxxxxxxxxxx>

[ ... ]

[Severity: Low]
Should this patch carry a Fixes: tag and Cc: stable?

Both failures described above come from the linearize fallback that
commit b91e82129400 ("bnxt_en: Linearize TX SKB if the fragments exceed
the max") added to bnxt_start_xmit():

drivers/net/ethernet/broadcom/bnxt/bnxt.c:bnxt_start_xmit() {
    ...
#if (MAX_SKB_FRAGS > TX_MAX_FRAGS)
	if (skb_shinfo(skb)->nr_frags > TX_MAX_FRAGS) {
		netdev_warn_once(dev, ...);
		if (skb_linearize(skb))
			goto tx_free;
	}
#endif
    ...
}

The driver calls netif_set_tso_max_size(dev, GSO_MAX_SIZE), so with BIG
TCP the linearized head can be larger than 65535 bytes. The BD length is
then built in bnxt_start_xmit() as:

	flags = (len << TX_BD_LEN_SHIFT) | TX_BD_TYPE_LONG_TX_BD |
		TX_BD_CNT(last_frag + 2);

TX_BD_LEN_SHIFT is 16, so the high bits of len are shifted out of the
u32. Wouldn't the descriptor then get a truncated length of len & 0xffff?

For devmem TX, the driver sets dev->netmem_tx, so unreadable skbs can
reach this path. skb_linearize() can't pull unreadable frags, so every
such GSO skb with more than TX_MAX_FRAGS (30) frags goes to tx_free and
is dropped.

A later patch in this series, "net: devmem: size TX net_iovs from the
dmabuf sg table", also calls the bnxt BD length limit a bug that needs
a fix. An earlier fix to the same block, commit 00eeab0c644a ("bnxt_en:
Write doorbell when linearizing skb fails"), carried
Fixes: b91e82129400 and Cc: stable@xxxxxxxxxxxxxxx.

Without the tag, stable kernels built with CONFIG_MAX_SKB_FRAGS > 30
would not get this fix automatically. Would something like this work?

Fixes: b91e82129400 ("bnxt_en: Linearize TX SKB if the fragments exceed the max")
Cc: stable@xxxxxxxxxxxxxxx

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261111224740.2744522-1-sdf%40fomichev.me