Re: [PATCH net] bnxt_en: fix DMA mapping length for padded small packets

[netdev-bot+sashiko@kernel.org]

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

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

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

- [High] The fix is incomplete for short non-linear skbs, and for them it
  changes the wire frame for the worse.

--

bnxt_en: fix DMA mapping length for padded small packets

This patch replaces skb_pad() with skb_put_padto() in the normal_tx path
of bnxt_start_xmit(). After the change, skb->len and skb_headlen() include
the padding, so dma_map_single() maps the whole padded buffer. The pad
variable and the rewrite of the last BD length are removed.

Closes: https://lore.kernel.org/netdev/20261004122616.56714cbd@nargothrond/

> diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> index d7728d0c5b6e6..7ea27e81e88c5 100644
> --- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c

[ ... ]

> @@ -672,14 +672,12 @@ static netdev_tx_t bnxt_start_xmit(struct sk_buff *skb, struct net_device *dev)
>  	}
>  
>  normal_tx:
> -	if (length < BNXT_MIN_PKT_SIZE) {
> -		pad = BNXT_MIN_PKT_SIZE - length;
> -		if (skb_pad(skb, pad))
> -			/* SKB already freed. */
> -			goto tx_kick_pending;
> -		length = BNXT_MIN_PKT_SIZE;
> +	if (skb_put_padto(skb, BNXT_MIN_PKT_SIZE)) {
> +		/* SKB already freed. */
> +		goto tx_kick_pending;
>  	}
> -
> +	length = skb->len;
> +	len = skb_headlen(skb);

[Severity: High]
What happens here when the skb is non-linear and shorter than
BNXT_MIN_PKT_SIZE?

bnxt_start_xmit() reads last_frag and tx_buf->nr_frags earlier, before
any padding:

	last_frag = skb_shinfo(skb)->nr_frags;
	...
	tx_buf->nr_frags = last_frag;

For a non-linear skb, skb_put_padto()->__skb_pad() skips the tailroom
fast path and calls skb_linearize(). __pskb_pull_tail() then:

  - copies the frags into the head
  - calls skb_frag_unref() on each one
  - sets nr_frags to 0

The frags[] slots themselves are not cleared.

After that, len = skb_headlen(skb) covers all 52 padded bytes, but
last_frag still holds the old count. TX_BD_CNT(last_frag + 2) and
SET_TX_OPAQUE(bp, txr, prod, 2 + last_frag) still count the old
fragments. The frag loop also still runs over them:

	for (i = 0; i < last_frag; i++) {
		frag = &skb_shinfo(skb)->frags[i];
		...
		mapping = skb_frag_dma_map(&pdev->dev, frag, 0, len,
					   DMA_TO_DEVICE);

This emits a BD for each stale frag, and TX_BD_FLAGS_PACKET_END is set on
the last stale BD:

	txbd->tx_bd_len_flags_type |= cpu_to_le32(TX_BD_FLAGS_PACKET_END);

As an example, take a 42-byte frame with a 14-byte linear head and one
28-byte frag. AF_PACKET PACKET_TX_RING can build one through
tpacket_fill_skb(). The head BD would describe 52 bytes and the stale frag
BD another 28. Wouldn't that put an 80-byte frame on the wire, with the
payload duplicated?

The NIC would also DMA-read pages that the skb no longer holds a
reference on. If this skb held the last reference, those pages could
already be freed or recycled.

The completion path in __bnxt_tx_int() (last = tx_buf->nr_frags) and the
tx_dma_error unwind path also trust the stale count.

bnxt sets NETIF_F_SG, so the core does not linearize these skbs first.
They reach normal_tx whenever the push path is skipped, for example when
tx_push_thresh is 0, the ring is not empty, or lflags is set.

The old code mapped the same stale frags, so that part is not new. Its
total descriptor length still came to 52, though, because the pad was
added to the last BD. The oversized frame with duplicated data seems to
come from this change. It also doesn't match this part of the commit
message:

  so that dma_map_single() maps the full buffer and the descriptor
  length is consistent.

Could the padding be done before length, len, last_frag and
tx_buf->nr_frags are read, and before the free_size check? Or could
last_frag and tx_buf->nr_frags be read again after skb_put_padto()
succeeds?

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261005023812.130639-1-edumazet%40kernel.org