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

[netdev-bot+sashiko@kernel.org]

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] The patch fixes real functional bugs in existing bnxt code, but
  it has no Fixes: tag and is not routed to stable.
- [Low] Clearing NETIF_F_GSO_MASK in bnxt_features_check() makes the
  stack segment a GSO skb with nr_frags > TX_MAX_FRAGS.

--

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

When MAX_SKB_FRAGS > TX_MAX_FRAGS, this patch clears the GSO features in
bnxt_features_check() for skbs that have more than TX_MAX_FRAGS frags.
The stack then segments those skbs, and bnxt_start_xmit() no longer
linearizes them. Non-GSO skbs with too many frags are still linearized in
the xmit path.

Link: https://lore.kernel.org/all/CANn89i+ps_CozfOyRnKav0_LUmSekJ9ExB5JkDbTAVf_ss_98g@mail.gmail.com/

> diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> index 33c4cacc8ffe3..f20b61180ab2d 100644
> --- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> @@ -14320,6 +14320,11 @@ static netdev_features_t bnxt_features_check(struct sk_buff *skb,
>  				skb_shinfo(skb)->gso_size))
>  		features &= ~NETIF_F_GSO_MASK;
>  
> +#if (MAX_SKB_FRAGS > TX_MAX_FRAGS)
> +	if (skb_shinfo(skb)->nr_frags > TX_MAX_FRAGS)
> +		features &= ~NETIF_F_GSO_MASK;
> +#endif

[Severity: Low]
Should this patch have a Fixes: tag?

Commit b91e82129400 ("bnxt_en: Linearize TX SKB if the fragments exceed
the max") added the unconditional linearize in bnxt_start_xmit():

bnxt_start_xmit() {
    ...
#if (MAX_SKB_FRAGS > TX_MAX_FRAGS)
	if (skb_shinfo(skb)->nr_frags > TX_MAX_FRAGS) {
		...
		if (skb_linearize(skb))
			goto tx_free;
	}
#endif
    ...
}

bnxt calls netif_set_tso_max_size(dev, GSO_MAX_SIZE), so a BIG TCP skb
can be linearized into a head larger than 64KB. That length is then
encoded here:

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

TX_BD_LEN_SHIFT is 16 and flags is a u32. The upper bits of the length
are lost, so the BD length no longer matches the DMA mapping.

bnxt also sets dev->netmem_tx = NETMEM_TX_DMA. For devmem GSO skbs,
skb_linearize() always fails, because __pskb_pull_tail() returns NULL
when !skb_frags_readable(). Those skbs are dropped at tx_free.

The commit message describes both problems. Without a Fixes: tag, stable
kernels that carry b91e82129400 won't pick up this fix.

[Severity: Low]
Can the segments that the stack produces here still have more than
TX_MAX_FRAGS unreadable frags?

Only the GSO features are cleared, and nothing limits the frag count of
each resulting segment. zerocopy_fill_skb_from_devmem() adds one frag per
iovec entry and does not merge adjacent ranges:

net/core/datagram.c:zerocopy_fill_skb_from_devmem() {
    ...
		size = min_t(size_t, size, iter_iov_len(from));
		netmem_inc_ref(net_iov_to_netmem(niov));
		skb_add_rx_frag_netmem(skb, i, net_iov_to_netmem(niov), off,
				       size, PAGE_SIZE);
    ...
}

Suppose a devmem sender uses small iovecs: frags averaging under about
47 bytes at a 1500 MTU, or under about 290 bytes at a 9K MTU. Then a
single MSS-sized segment from skb_segment() can span more than 30 frags.
__copy_skb_header() also carries over skb->unreadable to that segment.

Such a non-GSO segment then reaches skb_linearize() in bnxt_start_xmit(),
which fails for net_iov frags, and the segment is dropped at tx_free.
TCP retransmissions rebuild the same layout, so the flow may stall.

This is not worse than before the patch, when the whole GSO skb was
dropped in the same way. However, the commit message gives "always fails
for unreadable skbs" as a reason for the change, and for these flows that
failure can still happen after segmentation.

Could the commit message mention this limitation? Fully closing the gap
would need a fallback that handles unreadable frags, or a frag limit per
MSS window at sendmsg time, rather than skb_linearize().

>  	switch (vlan_get_protocol(skb)) {
>  	case htons(ETH_P_IP):
>  		if (!skb->encapsulation)

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