Re: [PATCH net-next v2 2/2] bnxt_en: Pad short SW USO segments to BNXT_MIN_PKT_SIZE

On Sat, Oct 10, 2026 at 10:55:25PM +0000, netdev-bot+sashiko@kernel.org wrote:

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

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

[...]

[Severity: Medium]
Should this have a Fixes: tag and Cc: stable, and go to net?

I mentioned this in the cover letter but Jakub specifically asked me to send
this to net-next since this issue cannot be triggered by ipv4 UDP and I have
not seen this issue in production,

If the maintainers change their mind and want me to resend to net, please let
me know.

quoted

diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_gso.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_gso.c
index ef04c9d08066d..80d5c4f963a95 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt_gso.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_gso.c

@@ -82,11 +82,19 @@ int bnxt_sw_udp_gso_xmit(struct bnxt *bp, struct bnxt_tx_ring_info *txr,
 	if (unlikely(num_segs <= 1))
 		goto drop;
 
+	/* bnxt_features_check() makes the stack segment these, but paths
+	 * that skip validate_xmit_skb() (e.g. generic XDP_TX of a GRO'd
+	 * skb) can still get here. Padding every segment would need more
+	 * BDs than bds_needed reserves below, so drop instead.
+	 */
+	if (unlikely(bnxt_sw_gso_pad_len(hdr_len, mss)))
+		goto drop;

[Severity: Low]
Could the commit message mention this new drop?

The changelog only says that bnxt_features_check() clears the GSO features
so the stack segments these skbs. That covers callers that go through
validate_xmit_skb(), but not callers that skip it, for example:

  generic_xdp_tx()
    netdev_start_xmit()
      bnxt_start_xmit()
        bnxt_sw_udp_gso_xmit()

On these paths, when hdr_len + gso_size is below BNXT_MIN_PKT_SIZE, the skb
is now freed and tx_dropped is incremented. Before this patch the skb was
sent as short segments. The comment in the code explains this, but the
changelog does not mention it.

I think mentioning it in the comment is sufficient, but up to the maintainers
if they want a respin for me to add this to commit message.

quoted

@@ -133,12 +141,14 @@ int bnxt_sw_udp_gso_xmit(struct bnxt *bp, struct bnxt_tx_ring_info *txr,

[ ... ]

quoted

 		last = (i == num_segs - 1);
+		pad = bnxt_sw_gso_pad_len(hdr_len, seg_payload);
 		offset = slot * TSO_HEADER_SIZE;
 		this_hdr = txr->tx_inline_buf + offset;
 		this_hdr_dma = txr->tx_inline_dma + offset;

[Severity: High]
This is a pre-existing issue, not one introduced by this patch. Is hdr_len
ever checked against TSO_HEADER_SIZE before tso_build_hdr() copies the
header into this slot?

Each slot is TSO_HEADER_SIZE (256) bytes. They sit in tx_inline_buf, which
bnxt_alloc_tx_inline_buf() allocates with kmalloc() as
BNXT_SW_USO_MAX_SEGS * TSO_HEADER_SIZE bytes. tso_build_hdr() then does:

net/core/tso.c:tso_build_hdr() {
	int hdr_len = skb_transport_offset(skb) + tso->tlen;
	...
	memcpy(hdr, skb->data, hdr_len);
	...
}

The driver then writes uh->check at skb_transport_offset(skb).

If hdr_len is larger than 256, can this overwrite the next slots? For the
last slot, can it write past the end of the tx_inline_buf allocation?

The validate_xmit_skb() path seems safe. bnxt advertises NETIF_F_IPV6_CSUM,
so gso_features_check() removes NETIF_F_GSO_UDP_L4 from IPv6 skbs with
extension headers.

The paths that skip validate_xmit_skb(), which the new comment above
mentions, do look reachable. One example is a remote peer sending IPv6/UDP
datagrams with large hop-by-hop or destination options headers, which
ipv6_gro_pull_exthdrs() merges. The skb then takes this path:

  __netif_receive_skb_core()
    do_xdp_generic()   /* XDP_TX */
      generic_xdp_tx()
        netdev_start_xmit()
          bnxt_start_xmit()
            bnxt_sw_udp_gso_xmit()

Neither bnxt_features_check() nor bnxt_exthdr_check() runs on this path.

A generic XDP program that prepends bytes with bpf_xdp_adjust_head() can
also make the transport offset larger. bpf_prog_run_generic_xdp() only
resets the network header.

Would it make sense to drop skbs with hdr_len > TSO_HEADER_SIZE next to the
new bnxt_sw_gso_pad_len() check?

I think this is real, but this is not specific to bnxt and it affects any
other drivers that use the tso helpers (I think).

My suggestion would be to send a followup that fixes this in the core code
instead of patching bnxt and every other driver that uses the tso helpers.

Maybe the correct fix here is to call validate_xmit_skb in the generic XDP
path and solve this for all drivers and also solve "low severity" issue
mentioned above (?)

Maintainer input would be welcome here, but IMHO, this seris is good as as-is
and a followup series could be sent to try to fix this in core.