[PATCH net 2/3] net/mlx5e: SHAMPO, Fix skb size check for 64K pages

Tariq Toukan posted 3 patches 3 months, 1 week ago
There is a newer version of this series
[PATCH net 2/3] net/mlx5e: SHAMPO, Fix skb size check for 64K pages
Posted by Tariq Toukan 3 months, 1 week ago
From: Dragos Tatulea <dtatulea@nvidia.com>

mlx5e_hw_gro_skb_has_enough_space() uses a formula to check if there is
enough space in the skb frags to store more data. This formula is
incorrect for 64K page sizes and it triggers early GRO session
termination because the first fragment will blow up beyond
GRO_LEGACY_MAX_SIZE.

This patch adds a special case for page sizes >= GRO_LEGACY_MAX_SIZE
(64K) which will uses the skb->data_len instead. Within this context,
this check will be safe from fragment overflow.

It is expected that the if statement will be optimized out as the
check is done with constants.

Fixes: 92552d3abd32 ("net/mlx5e: HW_GRO cqe handler implementation")
Signed-off-by: Dragos Tatulea <dtatulea@nvidia.com>
Signed-off-by: Tariq Toukan <tariqt@nvidia.com>
---
 drivers/net/ethernet/mellanox/mlx5/core/en_rx.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_rx.c b/drivers/net/ethernet/mellanox/mlx5/core/en_rx.c
index 77f7a1ca091d..ea4e7f486c8b 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_rx.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_rx.c
@@ -2350,7 +2350,10 @@ mlx5e_hw_gro_skb_has_enough_space(struct sk_buff *skb, u16 data_bcnt)
 {
 	int nr_frags = skb_shinfo(skb)->nr_frags;
 
-	return PAGE_SIZE * nr_frags + data_bcnt <= GRO_LEGACY_MAX_SIZE;
+	if (PAGE_SIZE >= 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;
 }
 
 static void mlx5e_handle_rx_cqe_mpwrq_shampo(struct mlx5e_rq *rq, struct mlx5_cqe64 *cqe)
-- 
2.31.1
Re: [PATCH net 2/3] net/mlx5e: SHAMPO, Fix skb size check for 64K pages
Posted by Simon Horman 3 months, 1 week ago
On Tue, Oct 28, 2025 at 08:47:18AM +0200, Tariq Toukan wrote:
> From: Dragos Tatulea <dtatulea@nvidia.com>
> 
> mlx5e_hw_gro_skb_has_enough_space() uses a formula to check if there is
> enough space in the skb frags to store more data. This formula is
> incorrect for 64K page sizes and it triggers early GRO session
> termination because the first fragment will blow up beyond
> GRO_LEGACY_MAX_SIZE.
> 
> This patch adds a special case for page sizes >= GRO_LEGACY_MAX_SIZE
> (64K) which will uses the skb->data_len instead. Within this context,
> this check will be safe from fragment overflow.

The above mentions skb->data_len, but the code uses skb->len.

Also, I think it would be worth describing why this is safe
in this context.

> 
> It is expected that the if statement will be optimized out as the
> check is done with constants.
> 
> Fixes: 92552d3abd32 ("net/mlx5e: HW_GRO cqe handler implementation")
> Signed-off-by: Dragos Tatulea <dtatulea@nvidia.com>
> Signed-off-by: Tariq Toukan <tariqt@nvidia.com>

...
Re: [PATCH net 2/3] net/mlx5e: SHAMPO, Fix skb size check for 64K pages
Posted by Dragos Tatulea 3 months, 1 week ago
On Wed, Oct 29, 2025 at 03:50:41PM +0000, Simon Horman wrote:
> On Tue, Oct 28, 2025 at 08:47:18AM +0200, Tariq Toukan wrote:
> > From: Dragos Tatulea <dtatulea@nvidia.com>
> > 
> > mlx5e_hw_gro_skb_has_enough_space() uses a formula to check if there is
> > enough space in the skb frags to store more data. This formula is
> > incorrect for 64K page sizes and it triggers early GRO session
> > termination because the first fragment will blow up beyond
> > GRO_LEGACY_MAX_SIZE.
> > 
> > This patch adds a special case for page sizes >= GRO_LEGACY_MAX_SIZE
> > (64K) which will uses the skb->data_len instead. Within this context,
> > this check will be safe from fragment overflow.
> 
> The above mentions skb->data_len, but the code uses skb->len.
>
Yep. Will fix on respin.

> Also, I think it would be worth describing why this is safe
> in this context.
>
Makes sense.

Thanks for the review Simon!

Thanks,
Dragos