Re: [PATCH net] net/mlx5e: strip runt Ethernet padding in HW-GRO (SHAMPO)

[Tariq Toukan <tariqt@nvidia.com>]

On 31/07/2026 21:54, Glenn Judd wrote:
> When hardware GRO (SHAMPO) coalesces a small IPv4/TCP segment
> that was padded up to the 60-byte minimum Ethernet frame, the trailing
> padding is folded into the merged payload.
> 
> The selftest tools/testing/selftests/drivers/net/hw/gro.py subtest
> hw_ipv4_data_lrg_1byte sends {100, 1} expecting to receive {101}.
> In current code, it receives {106} (100 + 1 payload + 5 pad) instead.
> 
> Fix is to compute the real (unpadded) frame length from the segment's
> IPv4 total (padded) length and drop the padding before it is merged.
> 
> Fixes: 92552d3abd32 ("net/mlx5e: HW_GRO cqe handler implementation")
> Cc: stable@vger.kernel.org
> Assisted-by: Claude:claude-opus-4-8
> Assisted-by: Codex:gpt-5.6
> Assisted-by: Meta:internal-AI-tooling
> Signed-off-by: Glenn Judd <gmj@meta.com>
> ---

Thanks for your patch.

>   .../net/ethernet/mellanox/mlx5/core/en_rx.c   | 72 ++++++++++++++++---
>   1 file changed, 64 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_rx.c b/drivers/net/ethernet/mellanox/mlx5/core/en_rx.c
> index 6fbc0441c4b8..0b40db3d50aa 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/en_rx.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_rx.c
> @@ -1506,12 +1506,41 @@ static inline bool mlx5e_build_rx_skb(struct mlx5_cqe64 *cqe,
>   	return false;
>   }
>   
> +/* Real (unpadded) L2 frame length, or 0 when it can't be determined. */
> +static u32 mlx5e_shampo_frame_len(struct mlx5e_rq *rq, struct mlx5_cqe64 *cqe,
> +				  struct sk_buff *skb)
> +{
> +	u16 head_size = cqe->shampo.header_size;
> +	struct iphdr *iph;
> +	int thoff, nhoff;
> +
> +	if (rq->hw_gro_data->fk.basic.n_proto != htons(ETH_P_IP) ||
> +	    rq->hw_gro_data->fk.control.flags & FLOW_DIS_ENCAPSULATION)
> +		return 0;
> +
> +	thoff = rq->hw_gro_data->fk.control.thoff;
> +	if (head_size < ETH_HLEN + thoff)
> +		return 0;
> +
> +	nhoff = ETH_HLEN + thoff - sizeof(struct iphdr);
> +	if (skb) {
> +		iph = (struct iphdr *)(skb->data + thoff - sizeof(struct iphdr));
> +	} else {
> +		void *hdr = mlx5e_shampo_get_hdr(rq, cqe, ETH_HLEN + thoff);
> +
> +		iph = (struct iphdr *)(hdr + nhoff);
> +	}
> +
> +	return nhoff + ntohs(iph->tot_len);
> +}
> +

This function is costly as it inspects the header. I'd limit the header 
lookup only to cases where the issue might be relevant. i.e. call it 
only for short_frames().

>   static bool mlx5e_shampo_complete_rx_cqe(struct mlx5e_rq *rq,
>   					 struct mlx5_cqe64 *cqe,
>   					 u32 cqe_bcnt,
>   					 struct sk_buff *skb)
>   {
>   	struct mlx5e_rq_stats *stats = rq->stats;
> +	u32 frame_len;
>   
>   	stats->packets++;
>   	stats->bytes += cqe_bcnt;
> @@ -1525,6 +1554,13 @@ static bool mlx5e_shampo_complete_rx_cqe(struct mlx5e_rq *rq,
>   	if (!skb_flow_dissect_flow_keys(skb, &rq->hw_gro_data->fk, 0)) {
>   		napi_gro_receive(rq->cq.napi, skb);
>   		rq->hw_gro_data->skb = NULL;
> +	} else {
> +		frame_len = mlx5e_shampo_frame_len(rq, cqe, skb);
> +		if (frame_len && frame_len >= cqe->shampo.header_size &&
> +		    frame_len < cqe_bcnt) {
> +			pskb_trim_rcsum(skb, skb->len - (cqe_bcnt - frame_len));
> +			skb_shinfo(skb)->gso_size = frame_len - cqe->shampo.header_size;
> +		}
>   	}
>   	return false;
>   }
> @@ -2244,6 +2280,7 @@ static void mlx5e_handle_rx_cqe_mpwrq_shampo(struct mlx5e_rq *rq, struct mlx5_cq
>   	struct mlx5e_rx_wqe_ll *wqe;
>   	struct mlx5e_mpw_info *wi;
>   	struct mlx5_wq_ll *wq;
> +	u32 frame_len = 0;
>   	u32 data_offset;
>   	u32 page_idx;
>   
> @@ -2263,11 +2300,22 @@ static void mlx5e_handle_rx_cqe_mpwrq_shampo(struct mlx5e_rq *rq, struct mlx5_cq
>   
>   	data_offset = wqe_offset & (page_size - 1);
>   	page_idx = wqe_offset >> rq->mpwqe.page_shift;
> -	if (*skb &&
> -	    !(match && mlx5e_hw_gro_skb_has_enough_space(*skb, data_bcnt,
> -							 page_size))) {
> -		match = false;
> -		mlx5e_shampo_flush_skb(rq, cqe, match);
> +
> +	if (*skb) {
> +		u32 gro_bcnt = data_bcnt;
> +
> +		if (match) {
> +			frame_len = mlx5e_shampo_frame_len(rq, cqe, NULL);
> +			if (frame_len && frame_len >= head_size &&
> +			    frame_len - head_size < gro_bcnt)
> +				gro_bcnt = frame_len - head_size;
> +		}
> +
> +		if (!(match && mlx5e_hw_gro_skb_has_enough_space(*skb, gro_bcnt,
> +								 page_size))) {
> +			match = false;
> +			mlx5e_shampo_flush_skb(rq, cqe, match);
> +		}
>   	}
>   
>   	if (!*skb) {
> @@ -2311,10 +2359,18 @@ static void mlx5e_handle_rx_cqe_mpwrq_shampo(struct mlx5e_rq *rq, struct mlx5_cq
>   
>   	if (likely(head_size)) {
>   		if (data_bcnt) {
> -			struct mlx5e_frag_page *frag_page;
> +			if (NAPI_GRO_CB(*skb)->count > 1 &&
> +			    frame_len >= head_size &&
> +			    frame_len - head_size < data_bcnt)
> +				data_bcnt = frame_len - head_size;
>   
> -			frag_page = &wi->alloc_units.frag_pages[page_idx];
> -			mlx5e_shampo_fill_skb_data(*skb, rq, frag_page, data_bcnt, data_offset);
> +			if (data_bcnt) {
> +				struct mlx5e_frag_page *frag_page =
> +					&wi->alloc_units.frag_pages[page_idx];
> +
> +				mlx5e_shampo_fill_skb_data(*skb, rq, frag_page,
> +							   data_bcnt, data_offset);
> +			}
>   		} else {
>   			stats->hds_nodata_packets++;
>   			stats->hds_nodata_bytes += head_size;
> 

I agree that getting the Eth padding into TCP payload needs fixing.
But proposed change is really invasive..

IMO it's much simpler to avoid SW aggregation for all small packets 
(without any inspection) rather than running costly and complicated 
logic just to coalesce a small packet.

Besides the selftest expectation, did you check what does SW GRO do? 
Does it strip the padding and coalesce, or refuses to coalesce padded 
frames?

> base-commit: 8ba098e6b6ff0db8edf28528d1552be261af30d4