[PATCH net] net: check dev->gso_max_size in gso_features_check()

[PATCH net] net: check dev->gso_max_size in gso_features_check()

[Eric Dumazet]

Some drivers might misbehave if TSO packets get too big.

GVE for instance uses a 16bit field in its TX descriptor,
and will do bad things if a packet is bigger than 2^16 bytes.

Linux TCP stack honors dev->gso_max_size, but there are
other ways for too big packets to reach an ndo_start_xmit()
handler : virtio_net, af_packet, GRO...

Add a generic check in gso_features_check() and fallback
to GSO when needed.

gso_max_size was added in the blamed commit.

Fixes: 82cc1a7a5687 ("[NET]: Add per-connection option to set max TSO frame size")
Signed-off-by: Eric Dumazet <edumazet@google.com>
---
 net/core/dev.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/net/core/dev.c b/net/core/dev.c
index 0432b04cf9b000628497345d9ec0e8a141a617a3..b55d539dca153f921260346a4f23bcce0e888227 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -3471,6 +3471,9 @@ static netdev_features_t gso_features_check(const struct sk_buff *skb,
 	if (gso_segs > READ_ONCE(dev->gso_max_segs))
 		return features & ~NETIF_F_GSO_MASK;
 
+	if (unlikely(skb->len >= READ_ONCE(dev->gso_max_size)))
+		return features & ~NETIF_F_GSO_MASK;
+
 	if (!skb_shinfo(skb)->gso_type) {
 		skb_warn_bad_offload(skb);
 		return features & ~NETIF_F_GSO_MASK;
-- 
2.43.0.472.g3155946c3a-goog

Re: [PATCH net] net: check dev->gso_max_size in gso_features_check()

[Paolo Abeni]

On Tue, 2023-12-19 at 12:53 +0000, Eric Dumazet wrote:
> diff --git a/net/core/dev.c b/net/core/dev.c
> index 0432b04cf9b000628497345d9ec0e8a141a617a3..b55d539dca153f921260346a4f23bcce0e888227 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -3471,6 +3471,9 @@ static netdev_features_t gso_features_check(const struct sk_buff *skb,
>  	if (gso_segs > READ_ONCE(dev->gso_max_segs))
>  		return features & ~NETIF_F_GSO_MASK;
>  
> +	if (unlikely(skb->len >= READ_ONCE(dev->gso_max_size)))

Since we are checking vs the limit supported by the NIC, should the
above be 'tso_max_size'?

My understanding is that 'gso{,_ipv4}_max_size' is the max aggregate
size the device asks for, and 'tso_max_size' is the actual limit
supported by the NIC.

Thanks!

Paolo

Re: [PATCH net] net: check dev->gso_max_size in gso_features_check()

[Eric Dumazet]

On Tue, Dec 19, 2023 at 3:42 PM Paolo Abeni <pabeni@redhat.com> wrote:
>
> On Tue, 2023-12-19 at 12:53 +0000, Eric Dumazet wrote:
> > diff --git a/net/core/dev.c b/net/core/dev.c
> > index 0432b04cf9b000628497345d9ec0e8a141a617a3..b55d539dca153f921260346a4f23bcce0e888227 100644
> > --- a/net/core/dev.c
> > +++ b/net/core/dev.c
> > @@ -3471,6 +3471,9 @@ static netdev_features_t gso_features_check(const struct sk_buff *skb,
> >       if (gso_segs > READ_ONCE(dev->gso_max_segs))
> >               return features & ~NETIF_F_GSO_MASK;
> >
> > +     if (unlikely(skb->len >= READ_ONCE(dev->gso_max_size)))
>
> Since we are checking vs the limit supported by the NIC, should the
> above be 'tso_max_size'?
>
> My understanding is that 'gso{,_ipv4}_max_size' is the max aggregate
> size the device asks for, and 'tso_max_size' is the actual limit
> supported by the NIC.
>

Problem is tso_max_size has been added very recently, depending on
this would make backports tricky.

I think the fix using gso_max_size is more portable to stable
versions, and allows the user to tweak the value,
and build tests.

As a bonus, dev->gso_max_size is in the net_device_read_tx cacheline,
while tso_max_size is currently far away.

Re: [PATCH net] net: check dev->gso_max_size in gso_features_check()

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net.git (main)
by Paolo Abeni <pabeni@redhat.com>:

On Tue, 19 Dec 2023 12:53:31 +0000 you wrote:
> Some drivers might misbehave if TSO packets get too big.
> 
> GVE for instance uses a 16bit field in its TX descriptor,
> and will do bad things if a packet is bigger than 2^16 bytes.
> 
> Linux TCP stack honors dev->gso_max_size, but there are
> other ways for too big packets to reach an ndo_start_xmit()
> handler : virtio_net, af_packet, GRO...
> 
> [...]

Here is the summary with links:
  - [net] net: check dev->gso_max_size in gso_features_check()
    https://git.kernel.org/netdev/net/c/24ab059d2ebd

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

Re: [PATCH net] net: check dev->gso_max_size in gso_features_check()

[Daniel Borkmann]

Hey all,

On 12/19/23 4:02 PM, Eric Dumazet wrote:
> On Tue, Dec 19, 2023 at 3:42 PM Paolo Abeni <pabeni@redhat.com> wrote:
>> On Tue, 2023-12-19 at 12:53 +0000, Eric Dumazet wrote:
>>> diff --git a/net/core/dev.c b/net/core/dev.c
>>> index 0432b04cf9b000628497345d9ec0e8a141a617a3..b55d539dca153f921260346a4f23bcce0e888227 100644
>>> --- a/net/core/dev.c
>>> +++ b/net/core/dev.c
>>> @@ -3471,6 +3471,9 @@ static netdev_features_t gso_features_check(const struct sk_buff *skb,
>>>        if (gso_segs > READ_ONCE(dev->gso_max_segs))
>>>                return features & ~NETIF_F_GSO_MASK;
>>>
>>> +     if (unlikely(skb->len >= READ_ONCE(dev->gso_max_size)))
>>
>> Since we are checking vs the limit supported by the NIC, should the
>> above be 'tso_max_size'?
>>
>> My understanding is that 'gso{,_ipv4}_max_size' is the max aggregate
>> size the device asks for, and 'tso_max_size' is the actual limit
>> supported by the NIC.
> 
> Problem is tso_max_size has been added very recently, depending on
> this would make backports tricky.
> 
> I think the fix using gso_max_size is more portable to stable
> versions, and allows the user to tweak the value,
> and build tests.
> 
> As a bonus, dev->gso_max_size is in the net_device_read_tx cacheline,
> while tso_max_size is currently far away.

We noticed in Cilium which supports both BIG TCP IPv4 as well as BIG TCP
IPv6 that when a user has configured the former but not the latter, we get
a performance regression. Meaning, kernel creates larger super packets for
IPv4, but later hits the lower IPv6 dev->gso_max_size limit. :/

Given tso_max_size is far away, would sth like the below fix be acceptable?

Thanks,
Daniel

 From 65260578ffda2969acfa5109eeef0484b7dd9193 Mon Sep 17 00:00:00 2001
From: Daniel Borkmann <daniel@iogearbox.net>
Date: Mon, 10 Jun 2024 12:52:22 +0000
Subject: [PATCH net] net, gso: Fix regression in BIG TCP v4 when BIG TCP v6 is not set

[ commit sg tbd ]

Fxies: 24ab059d2ebd ("net: check dev->gso_max_size in gso_features_check()")
Bisected-by: Nikolay Aleksandrov <razor@blackwall.org>
Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
Cc: Eric Dumazet <edumazet@google.com>
Cc: Paolo Abeni <pabeni@redhat.com>
Cc: Nikolay Aleksandrov <razor@blackwall.org>
---
  net/core/dev.c | 9 ++++++++-
  1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/net/core/dev.c b/net/core/dev.c
index 4d4de9008f6f..495457891191 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -3502,7 +3502,14 @@ static netdev_features_t gso_features_check(const struct sk_buff *skb,
  	if (gso_segs > READ_ONCE(dev->gso_max_segs))
  		return features & ~NETIF_F_GSO_MASK;

-	if (unlikely(skb->len >= READ_ONCE(dev->gso_max_size)))
+	/* Both GSO max sizes need to be checked e.g. for the case
+	 * when BIG TCP is enabled for IPv4 but not for IPv6. This
+	 * is checking the limits supported by the NIC (tso_max_size).
+	 * However, the latter is not hot in net_device_read_tx.
+	 */
+	if (unlikely(skb->len >=
+		     max(READ_ONCE(dev->gso_max_size),
+			 READ_ONCE(dev->gso_ipv4_max_size))))
  		return features & ~NETIF_F_GSO_MASK;

  	if (!skb_shinfo(skb)->gso_type) {
-- 
2.34.1