diff mbox series

[net] net: reenable NETIF_F_IPV6_CSUM offload for BIG TCP packets

Message ID 20250101164909.1331680-1-willemdebruijn.kernel@gmail.com (mailing list archive)
State Accepted
Commit 68e068cabd2c6c533ef934c2e5151609cf6ecc6d
Delegated to: Netdev Maintainers
Headers show
Series [net] net: reenable NETIF_F_IPV6_CSUM offload for BIG TCP packets | expand

Checks

Context Check Description
netdev/series_format success Single patches do not need cover letters
netdev/tree_selection success Clearly marked for net
netdev/ynl success Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present success Fixes tag present in non-next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 1 this patch: 1
netdev/build_tools success No tools touched, skip
netdev/cc_maintainers warning 1 maintainers not CCed: horms@kernel.org
netdev/build_clang success Errors and warnings before: 3 this patch: 3
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success Fixes tag looks correct
netdev/build_allmodconfig_warn success Errors and warnings before: 6 this patch: 6
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 11 lines checked
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 79 this patch: 79
netdev/source_inline success Was 0 now: 0
netdev/contest success net-next-2025-01-02--18-00 (tests: 881)

Commit Message

Willem de Bruijn Jan. 1, 2025, 4:47 p.m. UTC 
From: Willem de Bruijn <willemb@google.com>

The blamed commit disabled hardware offoad of IPv6 packets with
extension headers on devices that advertise NETIF_F_IPV6_CSUM,
based on the definition of that feature in skbuff.h:

 *   * - %NETIF_F_IPV6_CSUM
 *     - Driver (device) is only able to checksum plain
 *       TCP or UDP packets over IPv6. These are specifically
 *       unencapsulated packets of the form IPv6|TCP or
 *       IPv6|UDP where the Next Header field in the IPv6
 *       header is either TCP or UDP. IPv6 extension headers
 *       are not supported with this feature. This feature
 *       cannot be set in features for a device with
 *       NETIF_F_HW_CSUM also set. This feature is being
 *       DEPRECATED (see below).

The change causes skb_warn_bad_offload to fire for BIG TCP
packets.

[  496.310233] WARNING: CPU: 13 PID: 23472 at net/core/dev.c:3129 skb_warn_bad_offload+0xc4/0xe0

[  496.310297]  ? skb_warn_bad_offload+0xc4/0xe0
[  496.310300]  skb_checksum_help+0x129/0x1f0
[  496.310303]  skb_csum_hwoffload_help+0x150/0x1b0
[  496.310306]  validate_xmit_skb+0x159/0x270
[  496.310309]  validate_xmit_skb_list+0x41/0x70
[  496.310312]  sch_direct_xmit+0x5c/0x250
[  496.310317]  __qdisc_run+0x388/0x620

BIG TCP introduced an IPV6_TLV_JUMBO IPv6 extension header to
communicate packet length, as this is an IPv6 jumbogram. But, the
feature is only enabled on devices that support BIG TCP TSO. The
header is only present for PF_PACKET taps like tcpdump, and not
transmitted by physical devices.

For this specific case of extension headers that are not
transmitted, return to the situation before the blamed commit
and support hardware offload.

ipv6_has_hopopt_jumbo() tests not only whether this header is present,
but also that it is the only extension header before a terminal (L4)
header.

Fixes: 04c20a9356f2 ("net: skip offload for NETIF_F_IPV6_CSUM if ipv6 header contains extension")
Reported-by: syzbot <syzkaller@googlegroups.com>
Reported-by: Eric Dumazet <edumazet@google.com>
Closes: https://lore.kernel.org/netdev/CANn89iK1hdC3Nt8KPhOtTF8vCPc1AHDCtse_BTNki1pWxAByTQ@mail.gmail.com/
Signed-off-by: Willem de Bruijn <willemb@google.com>
---
 net/core/dev.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

Comments

Eric Dumazet Jan. 2, 2025, 11:51 a.m. UTC | #1 
On Wed, Jan 1, 2025 at 5:49 PM Willem de Bruijn
<willemdebruijn.kernel@gmail.com> wrote:
>
> From: Willem de Bruijn <willemb@google.com>
>
> The blamed commit disabled hardware offoad of IPv6 packets with
> extension headers on devices that advertise NETIF_F_IPV6_CSUM,
> based on the definition of that feature in skbuff.h:
>
>  *   * - %NETIF_F_IPV6_CSUM
>  *     - Driver (device) is only able to checksum plain
>  *       TCP or UDP packets over IPv6. These are specifically
>  *       unencapsulated packets of the form IPv6|TCP or
>  *       IPv6|UDP where the Next Header field in the IPv6
>  *       header is either TCP or UDP. IPv6 extension headers
>  *       are not supported with this feature. This feature
>  *       cannot be set in features for a device with
>  *       NETIF_F_HW_CSUM also set. This feature is being
>  *       DEPRECATED (see below).
>
> The change causes skb_warn_bad_offload to fire for BIG TCP
> packets.
>
> [  496.310233] WARNING: CPU: 13 PID: 23472 at net/core/dev.c:3129 skb_warn_bad_offload+0xc4/0xe0
>
> [  496.310297]  ? skb_warn_bad_offload+0xc4/0xe0
> [  496.310300]  skb_checksum_help+0x129/0x1f0
> [  496.310303]  skb_csum_hwoffload_help+0x150/0x1b0
> [  496.310306]  validate_xmit_skb+0x159/0x270
> [  496.310309]  validate_xmit_skb_list+0x41/0x70
> [  496.310312]  sch_direct_xmit+0x5c/0x250
> [  496.310317]  __qdisc_run+0x388/0x620
>
> BIG TCP introduced an IPV6_TLV_JUMBO IPv6 extension header to
> communicate packet length, as this is an IPv6 jumbogram. But, the
> feature is only enabled on devices that support BIG TCP TSO. The
> header is only present for PF_PACKET taps like tcpdump, and not
> transmitted by physical devices.
>
> For this specific case of extension headers that are not
> transmitted, return to the situation before the blamed commit
> and support hardware offload.
>
> ipv6_has_hopopt_jumbo() tests not only whether this header is present,
> but also that it is the only extension header before a terminal (L4)
> header.
>
> Fixes: 04c20a9356f2 ("net: skip offload for NETIF_F_IPV6_CSUM if ipv6 header contains extension")
> Reported-by: syzbot <syzkaller@googlegroups.com>
> Reported-by: Eric Dumazet <edumazet@google.com>
> Closes: https://lore.kernel.org/netdev/CANn89iK1hdC3Nt8KPhOtTF8vCPc1AHDCtse_BTNki1pWxAByTQ@mail.gmail.com/
> Signed-off-by: Willem de Bruijn <willemb@google.com>

Reviewed-by: Eric Dumazet <edumazet@google.com>

Thanks Willem !
patchwork-bot+netdevbpf@kernel.org Jan. 3, 2025, 2:20 a.m. UTC | #2 
Hello:

This patch was applied to netdev/net.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Wed,  1 Jan 2025 11:47:40 -0500 you wrote:
> From: Willem de Bruijn <willemb@google.com>
> 
> The blamed commit disabled hardware offoad of IPv6 packets with
> extension headers on devices that advertise NETIF_F_IPV6_CSUM,
> based on the definition of that feature in skbuff.h:
> 
>  *   * - %NETIF_F_IPV6_CSUM
>  *     - Driver (device) is only able to checksum plain
>  *       TCP or UDP packets over IPv6. These are specifically
>  *       unencapsulated packets of the form IPv6|TCP or
>  *       IPv6|UDP where the Next Header field in the IPv6
>  *       header is either TCP or UDP. IPv6 extension headers
>  *       are not supported with this feature. This feature
>  *       cannot be set in features for a device with
>  *       NETIF_F_HW_CSUM also set. This feature is being
>  *       DEPRECATED (see below).
> 
> [...]

Here is the summary with links:
  - [net] net: reenable NETIF_F_IPV6_CSUM offload for BIG TCP packets
    https://git.kernel.org/netdev/net/c/68e068cabd2c

You are awesome, thank you!
diff mbox series

Patch

diff --git a/net/core/dev.c b/net/core/dev.c
index 45a8c3dd4a64..faa23042df38 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -3642,8 +3642,10 @@  int skb_csum_hwoffload_help(struct sk_buff *skb,
 
 	if (features & (NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM)) {
 		if (vlan_get_protocol(skb) == htons(ETH_P_IPV6) &&
-		    skb_network_header_len(skb) != sizeof(struct ipv6hdr))
+		    skb_network_header_len(skb) != sizeof(struct ipv6hdr) &&
+		    !ipv6_has_hopopt_jumbo(skb))
 			goto sw_checksum;
+
 		switch (skb->csum_offset) {
 		case offsetof(struct tcphdr, check):
 		case offsetof(struct udphdr, check):