Re: [PATCH net] net/mlx5e: SHAMPO, Fix IP length overflow on large HW GRO sessions

[Dragos Tatulea]

Hi Tianyu,

Thanks for your patch. It is a good find.

On 29.07.26 22:47, Tianyu Zuo wrote:
> mlx5e_hw_gro_skb_has_enough_space() bounds a HW GRO session by the
> payload held in the skb fragments only. The L3/L4 headers that
> header-data split placed in the linear area are not accounted for, and
> the limit is inclusive of GRO_LEGACY_MAX_SIZE.
> 
> On a 4K page system a session can therefore grow to 16 full page
> fragments (65536 bytes) plus the 40 bytes of IPv4/TCP headers in the
> linear part, giving skb->len = 65576.
> 
> mlx5e_shampo_update_hdr() writes the IP length itself:
> 
> 	__be16 newlen = htons(skb->len - nhoff);
> 	csum_replace2(&ipv4->check, ipv4->tot_len, newlen);
> 	ipv4->tot_len = newlen;
> 
Commit message is clear until here.

> With nhoff == 0 this stores tot_len = 40 and updates the header checksum
> to match, so the corruption is self-consistent. The GRO stack does not
> repair it: the header is written before napi_gro_receive(), and
> inet_gro_complete() only runs for skbs that the GRO engine holds on its
> gro_list. HW GRO sessions are typically flushed on TCP_FLAG_PSH, which
> makes tcp_gro_receive() set NAPI_GRO_CB(skb)->flush, so dev_gro_receive()
> hands the skb over via GRO_NORMAL and the gro_complete() callbacks are
> never invoked. The length check in inet_gro_receive() cannot catch it
> either, since tot_len and skb_gro_len() are compared modulo 64K.
> 
> ip_rcv_core() then trims the 64KB skb down to the wrapped tot_len,
> silently dropping the payload. The IPv6 path wraps identically in
> ipv6hdr->payload_len.
> 
> Triggering this requires the payload of the aggregated session to reach
> GRO_LEGACY_MAX_SIZE with every fragment fully populated, since
> page_size * nr_frags otherwise overestimates the data actually present
> and the session is flushed earlier. In practice this needs an MSS that
> is a multiple of the page size (for example 8192 on a 4K page host with
> jumbo frames) together with a page aligned start of the session.
> 
You can drop this bit.

> Account for skb_headlen() and make both checks strictly less than
> GRO_LEGACY_MAX_SIZE so that skb->len can never exceed 65535.
> 
This is useful.

> The check is strictly more conservative than before, so the implicit
> bound on the fragment count is preserved: page_size * nr_frags +
> data_bcnt <= 65535 gives nr_frags + data_bcnt / page_size <=
> 65536 / page_size - 1, and a single CQE adds at most
> data_bcnt / page_size + 1 fragments, for a total of at most
> 65536 / page_size (16 on 4K pages), well below MAX_SKB_FRAGS.
> 
You can drop this last paragraph. Or summarize it in the previous
one.

> Fixes: 92552d3abd32 ("net/mlx5e: HW_GRO cqe handler implementation")
> Signed-off-by: Tianyu Zuo <cosmosocket@xxxxxxxxx>
> ---
>  drivers/net/ethernet/mellanox/mlx5/core/en_rx.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 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..2e9676305439 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/en_rx.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_rx.c
> @@ -2222,9 +2222,10 @@ static bool mlx5e_hw_gro_skb_has_enough_space(struct sk_buff *skb,
>  	int nr_frags = skb_shinfo(skb)->nr_frags;
>  
>  	if (page_size >= GRO_LEGACY_MAX_SIZE)
> -		return skb->len + data_bcnt <= GRO_LEGACY_MAX_SIZE;
> +		return skb->len + data_bcnt < GRO_LEGACY_MAX_SIZE;
>  	else
> -		return page_size * nr_frags + data_bcnt <= GRO_LEGACY_MAX_SIZE;
> +		return skb_headlen(skb) + page_size * nr_frags + data_bcnt <
> +			GRO_LEGACY_MAX_SIZE;
>  }
>  
>  static void mlx5e_handle_rx_cqe_mpwrq_shampo(struct mlx5e_rq *rq, struct mlx5_cqe64 *cqe)
> 
> base-commit: 51b093a7ba27476e1f639455f005e8d2e75390e4

The code looks good to me. Wait for Tariq's comments before re-spinning.

Thanks,
Dragos