Re: [PATCH net 5/6] net: bcmasp: account for offload header in TX short packet padding

[netdev-bot+sashiko]

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 only works for linear skbs.

--

net: bcmasp: account for offload header in TX short packet padding

This patch makes the TX padding threshold in bcmasp_xmit() include
sizeof(struct bcmasp_pkt_offload) when hardware checksum offload is in
use. bcmasp_csum_offload() pushes that 20-byte header onto the skb, and
the hardware strips it before transmission. Without the adjustment,
short frames could go out below ETH_ZLEN + ETH_FCS_LEN.

> diff --git a/drivers/net/ethernet/broadcom/asp2/bcmasp_intf.c b/drivers/net/ethernet/broadcom/asp2/bcmasp_intf.c
> index 7a63a592f158d..3370cbf1b395a 100644
> --- a/drivers/net/ethernet/broadcom/asp2/bcmasp_intf.c
> +++ b/drivers/net/ethernet/broadcom/asp2/bcmasp_intf.c

[ ... ]

> @@ -267,8 +267,12 @@ static netdev_tx_t bcmasp_xmit(struct sk_buff *skb, struct net_device *dev)
>  	for (i = 0; i <= nr_frags; i++) {
>  		if (!i) {
>  			size = skb_headlen(skb);
> -			if (!nr_frags && size < (ETH_ZLEN + ETH_FCS_LEN)) {
> -				if (skb_put_padto(skb, ETH_ZLEN + ETH_FCS_LEN))
> +			min_size = ETH_ZLEN + ETH_FCS_LEN;
> +			if (csum_hw)
> +				min_size += sizeof(struct bcmasp_pkt_offload);
> +
> +			if (!nr_frags && size < min_size) {
> +				if (skb_put_padto(skb, min_size))
>  					return NETDEV_TX_OK;
>  				size = skb->len;
>  			}

[Severity: High]
Does this still let short checksum-offloaded frames go out undersized
when the skb is non-linear?

The new min_size is only applied when nr_frags is zero, and it is
compared against skb_headlen() rather than skb->len.

bcmasp_interface_create() advertises scatter-gather together with L4
checksum offload:

	ndev->features |= NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM | NETIF_F_SG |
			  NETIF_F_RXCSUM;

tcp_sendmsg_locked() copies payload into page frags through
skb_copy_to_page_nocache(). This means small TCP segments arrive here
as CHECKSUM_PARTIAL skbs with headers in the linear area and
nr_frags >= 1.

As an example, take an IPv4 TCP segment without timestamps carrying 1
to 5 bytes of payload. That is 14 + 20 + 20 + N = 55 to 59 bytes.
bcmasp_csum_offload() pushes the 20-byte header and sets csum_hw, so
skb->len becomes 75 to 79. That is below the new min_size of 84.

Because nr_frags == 1, skb_put_padto() is never called. The hardware
strips the offload header and a 55 to 59 byte frame goes out on the
wire. This looks like the same failure the commit message describes:

  "causing short frames to skip padding and be sent on the wire smaller
  than the minimum Ethernet frame size (ETH_ZLEN + ETH_FCS_LEN)"

For TCP, retransmits of that segment would have the same non-linear
shape, so they could be dropped by the receiver every time.

Would it be better to make the padding decision on skb->len before the
descriptor loop? One way is skb_put_padto() or eth_skb_pad(), which
linearize through __skb_pad(), followed by re-reading nr_frags.

The pad can't safely happen inside the loop. nr_frags is cached before
bcmasp_csum_offload():

	nr_frags = skb_shinfo(skb)->nr_frags;

If the skb were linearized in the middle of the loop, the loop would go
on indexing frags that no longer exist.

As far as I can tell, no later patch in the series changes this code in
bcmasp_intf.c.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260922221630.3864427-1-florian.fainelli%40broadcom.com