netdev - Re: [PATCH net] net: check dev->gso_max_size in gso_features

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date: Mon, 10 Jun 2024 15:53:43 +0200
From: Daniel Borkmann <daniel@...earbox.net>
To: Eric Dumazet <edumazet@...gle.com>, Paolo Abeni <pabeni@...hat.com>
Cc: "David S . Miller" <davem@...emloft.net>, Jakub Kicinski
 <kuba@...nel.org>, Willem de Bruijn <willemb@...gle.com>,
 netdev@...r.kernel.org, eric.dumazet@...il.com, razor@...ckwall.org
Subject: Re: [PATCH net] net: check dev->gso_max_size in gso_features_check()

Hey all,

On 12/19/23 4:02 PM, Eric Dumazet wrote:
> On Tue, Dec 19, 2023 at 3:42 PM Paolo Abeni <pabeni@...hat.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@...earbox.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@...ckwall.org>
Signed-off-by: Daniel Borkmann <daniel@...earbox.net>
Cc: Eric Dumazet <edumazet@...gle.com>
Cc: Paolo Abeni <pabeni@...hat.com>
Cc: Nikolay Aleksandrov <razor@...ckwall.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