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

[Joe Damato <joe@dama.to>]

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.

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

> > @@ -133,12 +141,14 @@ int bnxt_sw_udp_gso_xmit(struct bnxt *bp, struct bnxt_tx_ring_info *txr,
> 
> [ ... ]
> 
> >  		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.