[PATCH net-next v2 0/2] bnxt_en: Fix SW USO padding

[PATCH net-next v2 0/2] bnxt_en: Fix SW USO padding

[Joe Damato]

Greetings:

As a followup to Eric's fix a51d233ccd48 ("bnxt_en: fix DMA mapping length
for padded small packets") which fixes padding for normal TX, this series
fixes padding for SW USO.

There are two cases I try to address:

  1. The last segment could be shorter than BNXT_MIN_PKT_SIZE. So, add an
     extra BD if needed to pad to that minimum.
  2. The gso_size is so small that every segment would be shorter than
     BNXT_MIN_PKT_SIZE (under 10 bytes with IPv4). This seems exceedingly
     unlikely, but just in case, it is handled by clearing the GSO features
     for these skbs so the stack segments them.

Patch 1 is a small refactor to move code into a helper that patch 2 uses to
avoid duplicating code.

I ran v1 on a bnxt machine and re-ran the uso.py test and it passed, but
the existing uso.py test doesn't test this specific case, so running the
test was more of a regression test. There were no functional changes to
v2.

Note that I did not see this issue in production and it should not be
possible for QUIC to hit this case on either ipv4 or ipv6 because QUIC
packets are at least 21 bytes. AFAIU, only ipv4 segments with less than 10
bytes of UDP payload can trigger this. As such, I was asked to send this
to net-next.

Thanks,
Joe

v2:
  - Switched to net-next
  - Added a comment as discussed with Michael in patch 2
  - Added Michael's tags to patches 1 and 2

v1: https://lore.kernel.org/netdev/20261008191114.3736826-1-joe@dama.to/

Joe Damato (2):
  bnxt_en: Add helper to fill SW USO payload BDs
  bnxt_en: Pad short SW USO segments to BNXT_MIN_PKT_SIZE

 drivers/net/ethernet/broadcom/bnxt/bnxt.c     | 12 ++++
 drivers/net/ethernet/broadcom/bnxt/bnxt_gso.c | 61 +++++++++++++++----
 drivers/net/ethernet/broadcom/bnxt/bnxt_gso.h | 15 +++++
 3 files changed, 75 insertions(+), 13 deletions(-)


base-commit: d8674294aefef02266c4d47ad10131f1bffbe534
-- 
2.53.0-Meta

[PATCH net-next v2 1/2] bnxt_en: Add helper to fill SW USO payload BDs

[Joe Damato]

Move the code that fills a payload BD and its software ring entry in
bnxt_sw_udp_gso_xmit() into a helper, bnxt_sw_gso_data_bd().

No functional change. A following patch uses the helper to add a pad BD
to short SW USO segments.

Reviewed-by: Michael Chan <michael.chan@broadcom.com>
Signed-off-by: Joe Damato <joe@dama.to>
---
 drivers/net/ethernet/broadcom/bnxt/bnxt_gso.c | 33 ++++++++++++-------
 1 file changed, 22 insertions(+), 11 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_gso.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_gso.c
index 6c1060fa2ea5..ef04c9d08066 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt_gso.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_gso.c
@@ -31,6 +31,26 @@ static u32 bnxt_sw_gso_lhint(unsigned int len)
 		return TX_BD_FLAGS_LHINT_2048_AND_LARGER;
 }
 
+static struct tx_bd *bnxt_sw_gso_data_bd(struct bnxt *bp,
+					 struct bnxt_tx_ring_info *txr,
+					 u16 prod, dma_addr_t addr,
+					 unsigned int len)
+{
+	struct tx_bd *txbd = &txr->tx_desc_ring[TX_RING(bp, prod)][TX_IDX(prod)];
+	struct bnxt_sw_tx_bd *tx_buf = &txr->tx_buf_ring[RING_TX(bp, prod)];
+
+	txbd->tx_bd_haddr = cpu_to_le64(addr);
+	txbd->tx_bd_len_flags_type = cpu_to_le32(len << TX_BD_LEN_SHIFT);
+	txbd->tx_bd_opaque = 0;
+
+	dma_unmap_addr_set(tx_buf, mapping, addr);
+	dma_unmap_len_set(tx_buf, len, 0);
+	tx_buf->skb = NULL;
+	tx_buf->is_sw_gso = 0;
+
+	return txbd;
+}
+
 /* Transmit an skb requiring software UDP segmentation.
  *
  * Returns 1 if the skb was queued and new BDs were produced, 0 if the skb
@@ -181,15 +201,10 @@ int bnxt_sw_udp_gso_xmit(struct bnxt *bp, struct bnxt_tx_ring_info *txr,
 		while (tso_dma_map_next(&map, &dma_addr, &chunk_len,
 					&mapping_len, seg_payload)) {
 			prod = NEXT_TX(prod);
-			txbd = &txr->tx_desc_ring[TX_RING(bp, prod)][TX_IDX(prod)];
+			txbd = bnxt_sw_gso_data_bd(bp, txr, prod, dma_addr,
+						   chunk_len);
 			tx_buf = &txr->tx_buf_ring[RING_TX(bp, prod)];
 
-			txbd->tx_bd_haddr = cpu_to_le64(dma_addr);
-			dma_unmap_addr_set(tx_buf, mapping, dma_addr);
-			dma_unmap_len_set(tx_buf, len, 0);
-			tx_buf->skb = NULL;
-			tx_buf->is_sw_gso = 0;
-
 			if (mapping_len) {
 				if (last_unmap_buf) {
 					dma_unmap_addr_set(last_unmap_buf,
@@ -204,10 +219,6 @@ int bnxt_sw_udp_gso_xmit(struct bnxt *bp, struct bnxt_tx_ring_info *txr,
 			}
 			last_unmap_buf = tx_buf;
 
-			flags = chunk_len << TX_BD_LEN_SHIFT;
-			txbd->tx_bd_len_flags_type = cpu_to_le32(flags);
-			txbd->tx_bd_opaque = 0;
-
 			seg_payload -= chunk_len;
 		}
 
-- 
2.53.0-Meta

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

[Joe Damato]

Each SW USO segment is hdr_len + seg_payload bytes. With IPv4, hdr_len
is 42 bytes, so a segment carrying less than 10 bytes of UDP payload is
sent shorter than BNXT_MIN_PKT_SIZE. This happens with a short last
segment (e.g. gso_size 1400 and 1405 bytes of payload) or with a
gso_size below 10 bytes.

Fix the short last segment in bnxt_sw_udp_gso_xmit(): zero the pad
bytes after the header in the segment's inline header slot and add a
pad BD pointing at them after the payload BDs. Only the last segment
can need padding, and its pad BD fits within the existing bound in
bds_needed and BNXT_SW_USO_MAX_DESCS, since payload BDs are at most
num_segs + nr_frags.

If gso_size itself is too small, every segment would need a pad BD,
exceeding the descriptor bound. This case is unlikely, so
bnxt_features_check() clears GSO features for these skbs and the stack
segments them instead; bnxt_start_xmit() then pads each segment.

Reviewed-by: Michael Chan <michael.chan@broadcom.com>
Signed-off-by: Joe Damato <joe@dama.to>
---
 drivers/net/ethernet/broadcom/bnxt/bnxt.c     | 12 ++++++++
 drivers/net/ethernet/broadcom/bnxt/bnxt_gso.c | 28 +++++++++++++++++--
 drivers/net/ethernet/broadcom/bnxt/bnxt_gso.h | 15 ++++++++++
 3 files changed, 53 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
index c1d3fcc7ae84..0682060b048c 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
@@ -14268,6 +14268,18 @@ static netdev_features_t bnxt_features_check(struct sk_buff *skb,
 	u8 *l4_proto;
 
 	features = vlan_features_check(skb, features);
+
+	/* SW USO pads at most the last segment; let the stack segment skbs
+	 * where every segment would be shorter than BNXT_MIN_PKT_SIZE.
+	 */
+	if (skb_is_gso(skb) &&
+	    (skb_shinfo(skb)->gso_type & SKB_GSO_UDP_L4) &&
+	    !(bp->flags & BNXT_FLAG_UDP_GSO_CAP) &&
+	    bnxt_sw_gso_pad_len(skb_transport_offset(skb) +
+				sizeof(struct udphdr),
+				skb_shinfo(skb)->gso_size))
+		features &= ~NETIF_F_GSO_MASK;
+
 	switch (vlan_get_protocol(skb)) {
 	case htons(ETH_P_IP):
 		if (!skb->encapsulation)
diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_gso.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_gso.c
index ef04c9d08066..80d5c4f963a9 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;
+
 	/* Upper bound on the number of descriptors needed.
 	 *
 	 * Each segment uses 1 long BD + 1 ext BD + payload BDs, which is
 	 * at most num_segs + nr_frags (each frag boundary crossing adds at
-	 * most 1 extra BD).
+	 * most 1 extra BD). The last segment may need 1 pad BD.
 	 */
 	bds_needed = 3 * num_segs + skb_shinfo(skb)->nr_frags + 1;
 
@@ -133,12 +141,14 @@ int bnxt_sw_udp_gso_xmit(struct bnxt *bp, struct bnxt_tx_ring_info *txr,
 		dma_addr_t dma_addr;
 		struct tx_bd *txbd;
 		struct udphdr *uh;
+		unsigned int pad;
 		void *this_hdr;
 		int bd_count;
 		bool last;
 		u32 flags;
 
 		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;
@@ -156,10 +166,18 @@ int bnxt_sw_udp_gso_xmit(struct bnxt *bp, struct bnxt_tx_ring_info *txr,
 			iph->check = 0;
 		}
 
+		/* Pad bytes follow the header in the inline slot. Zero them
+		 * so stale header bytes from an earlier packet are not sent.
+		 */
+		if (pad)
+			memset(this_hdr + hdr_len, 0, pad);
+
 		dma_sync_single_for_device(&pdev->dev, this_hdr_dma,
-					   hdr_len, DMA_TO_DEVICE);
+					   hdr_len + pad, DMA_TO_DEVICE);
 
 		bd_count = tso_dma_map_count(&map, seg_payload);
+		if (pad)
+			bd_count++;
 
 		tx_buf = &txr->tx_buf_ring[RING_TX(bp, prod)];
 		txbd = &txr->tx_desc_ring[TX_RING(bp, prod)][TX_IDX(prod)];
@@ -222,6 +240,12 @@ int bnxt_sw_udp_gso_xmit(struct bnxt *bp, struct bnxt_tx_ring_info *txr,
 			seg_payload -= chunk_len;
 		}
 
+		if (pad) {
+			prod = NEXT_TX(prod);
+			txbd = bnxt_sw_gso_data_bd(bp, txr, prod,
+						   this_hdr_dma + hdr_len, pad);
+		}
+
 		txbd->tx_bd_len_flags_type |=
 			cpu_to_le32(TX_BD_FLAGS_PACKET_END);
 
diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_gso.h b/drivers/net/ethernet/broadcom/bnxt/bnxt_gso.h
index 77d9af97cc22..5916ee08c0e6 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt_gso.h
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_gso.h
@@ -19,10 +19,25 @@
  * Each segment: 1 long BD + 1 ext BD + payload BDs.
  * Total payload BDs across all segs <= num_segs + nr_frags (each frag
  * boundary crossing adds at most 1 extra BD).
+ * The last segment may need 1 pad BD (see bnxt_sw_gso_pad_len()).
  * So: 3 * max_segs + MAX_SKB_FRAGS + 1 = 3 * 64 + 17 + 1 = 210.
  */
 #define BNXT_SW_USO_MAX_DESCS	(3 * BNXT_SW_USO_MAX_SEGS + MAX_SKB_FRAGS + 1)
 
+/* Bytes of padding needed to bring a segment carrying @seg_payload bytes
+ * up to BNXT_MIN_PKT_SIZE, or 0 if no padding is needed.
+ */
+static inline unsigned int bnxt_sw_gso_pad_len(unsigned int hdr_len,
+					       unsigned int seg_payload)
+{
+	unsigned int len = hdr_len + seg_payload;
+
+	if (len < BNXT_MIN_PKT_SIZE)
+		return BNXT_MIN_PKT_SIZE - len;
+	else
+		return 0;
+}
+
 static inline u16 bnxt_inline_avail(struct bnxt_tx_ring_info *txr)
 {
 	return BNXT_SW_USO_MAX_SEGS -
-- 
2.53.0-Meta

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

[netdev-bot+sashiko]

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

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

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] This patch fixes a bug in released kernels but has no `Fixes:`
  tag and no `Cc: stable`.
- [Low] The patch adds a drop that the commit message never mentions.

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but
  `bnxt_sw_udp_gso_xmit()` never checks hdr_len against TSO_HEADER_SIZE…

--

bnxt_en: Pad short SW USO segments to BNXT_MIN_PKT_SIZE

This patch pads a short last SW USO segment to BNXT_MIN_PKT_SIZE. The pad
bytes are zeroed in the inline header slot and sent through an extra pad BD.
When gso_size is too small for any segment to reach the minimum,
bnxt_features_check() clears the GSO features. If such skbs still reach
bnxt_sw_udp_gso_xmit(), they are dropped there.

> Reviewed-by: Michael Chan <michael.chan@broadcom.com>
> Signed-off-by: Joe Damato <joe@dama.to>

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

bnxt_sw_udp_gso_xmit() has not padded segments since it was added in:

  cc5d90667db8 ("net: bnxt: Implement software USO")

That commit is already in released kernels.

When BNXT_FLAG_UDP_GSO_CAP is clear, bnxt_start_xmit() passes every
SKB_GSO_UDP_L4 skb to bnxt_sw_udp_gso_xmit(). It returns before the
skb_put_padto(skb, BNXT_MIN_PKT_SIZE) call, so stable trees with SW USO
still send frames shorter than 52 bytes.

The companion fix a51d233ccd48 ("bnxt_en: fix DMA mapping length for
padded small packets") was sent with Fixes: and Cc: stable. Its changelog
says:

    Note that SW USO segments are still not padded: with IPv4, a segment
    carrying less than 10 bytes of UDP payload (small gso_size, or a short
    last segment) is sent as a frame shorter than BNXT_MIN_PKT_SIZE.
    This is a separate issue, left for a followup patch.

This patch looks like that followup. However, it is based on net-next, after
the prep patch "bnxt_en: Add helper to fill SW USO payload BDs".

Would something like this be appropriate?

  Fixes: cc5d90667db8 ("net: bnxt: Implement software USO")

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

[ ... ]

> @@ -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?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261009215426.2377946-1-joe%40dama.to