* [PATCH v2 net] net: skip offload for NETIF_F_IPV6_CSUM if ipv6 header contains extension
@ 2024-10-24 14:01 Benoît Monin
2024-10-25 14:55 ` Willem de Bruijn
2024-12-23 10:40 ` Eric Dumazet
0 siblings, 2 replies; 6+ messages in thread
From: Benoît Monin @ 2024-10-24 14:01 UTC (permalink / raw)
To: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Jiri Pirko, Sebastian Andrzej Siewior, Lorenzo Bianconi
Cc: netdev, linux-kernel, Benoît Monin
As documented in skbuff.h, devices with NETIF_F_IPV6_CSUM capability
can only checksum TCP and UDP over IPv6 if the IP header does not
contains extension.
This is enforced for UDP packets emitted from user-space to an IPv6
address as they go through ip6_make_skb(), which calls
__ip6_append_data() where a check is done on the header size before
setting CHECKSUM_PARTIAL.
But the introduction of UDP encapsulation with fou6 added a code-path
where it is possible to get an skb with a partial UDP checksum and an
IPv6 header with extension:
* fou6 adds a UDP header with a partial checksum if the inner packet
does not contains a valid checksum.
* ip6_tunnel adds an IPv6 header with a destination option extension
header if encap_limit is non-zero (the default value is 4).
The thread linked below describes in more details how to reproduce the
problem with GRE-in-UDP tunnel.
Add a check on the network header size in skb_csum_hwoffload_help() to
make sure no IPv6 packet with extension header is handed to a network
device with NETIF_F_IPV6_CSUM capability.
Link: https://lore.kernel.org/netdev/26548921.1r3eYUQgxm@benoit.monin/T/#u
Fixes: aa3463d65e7b ("fou: Add encap ops for IPv6 tunnels")
Signed-off-by: Benoît Monin <benoit.monin@gmx.fr>
---
changelog
* v2:
- patch against net instead of net-next
- clarify documentation of NETIF_F_IPV6_CSUM
- add link to thread describing the problem
- add fixes tag
- use vlan_get_protocol to check for IPv6
* v1:
- https://lore.kernel.org/netdev/0dc0c2af98e96b1df20bd36aeaed4eb4e27d507e.1728056028.git.benoit.monin@gmx.fr/T/#u
---
net/core/dev.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/net/core/dev.c b/net/core/dev.c
index ea5fbcd133ae..8453e14d301b 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -3639,6 +3639,9 @@ int skb_csum_hwoffload_help(struct sk_buff *skb,
return 0;
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))
+ goto sw_checksum;
switch (skb->csum_offset) {
case offsetof(struct tcphdr, check):
case offsetof(struct udphdr, check):
@@ -3646,6 +3649,7 @@ int skb_csum_hwoffload_help(struct sk_buff *skb,
}
}
+sw_checksum:
return skb_checksum_help(skb);
}
EXPORT_SYMBOL(skb_csum_hwoffload_help);
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH v2 net] net: skip offload for NETIF_F_IPV6_CSUM if ipv6 header contains extension
2024-10-24 14:01 [PATCH v2 net] net: skip offload for NETIF_F_IPV6_CSUM if ipv6 header contains extension Benoît Monin
@ 2024-10-25 14:55 ` Willem de Bruijn
2024-12-23 10:40 ` Eric Dumazet
1 sibling, 0 replies; 6+ messages in thread
From: Willem de Bruijn @ 2024-10-25 14:55 UTC (permalink / raw)
To: Benoît Monin, David S. Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, Jiri Pirko, Sebastian Andrzej Siewior,
Lorenzo Bianconi
Cc: netdev, linux-kernel, Benoît Monin
Benoît Monin wrote:
> As documented in skbuff.h, devices with NETIF_F_IPV6_CSUM capability
> can only checksum TCP and UDP over IPv6 if the IP header does not
> contains extension.
>
> This is enforced for UDP packets emitted from user-space to an IPv6
> address as they go through ip6_make_skb(), which calls
> __ip6_append_data() where a check is done on the header size before
> setting CHECKSUM_PARTIAL.
>
> But the introduction of UDP encapsulation with fou6 added a code-path
> where it is possible to get an skb with a partial UDP checksum and an
> IPv6 header with extension:
> * fou6 adds a UDP header with a partial checksum if the inner packet
> does not contains a valid checksum.
> * ip6_tunnel adds an IPv6 header with a destination option extension
> header if encap_limit is non-zero (the default value is 4).
>
> The thread linked below describes in more details how to reproduce the
> problem with GRE-in-UDP tunnel.
>
> Add a check on the network header size in skb_csum_hwoffload_help() to
> make sure no IPv6 packet with extension header is handed to a network
> device with NETIF_F_IPV6_CSUM capability.
>
> Link: https://lore.kernel.org/netdev/26548921.1r3eYUQgxm@benoit.monin/T/#u
> Fixes: aa3463d65e7b ("fou: Add encap ops for IPv6 tunnels")
> Signed-off-by: Benoît Monin <benoit.monin@gmx.fr>
Reviewed-by: Willem de Bruijn <willemb@google.com>
> ---
> changelog
> * v2:
> - patch against net instead of net-next
> - clarify documentation of NETIF_F_IPV6_CSUM
> - add link to thread describing the problem
> - add fixes tag
> - use vlan_get_protocol to check for IPv6
> * v1:
> - https://lore.kernel.org/netdev/0dc0c2af98e96b1df20bd36aeaed4eb4e27d507e.1728056028.git.benoit.monin@gmx.fr/T/#u
> ---
> net/core/dev.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/net/core/dev.c b/net/core/dev.c
> index ea5fbcd133ae..8453e14d301b 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -3639,6 +3639,9 @@ int skb_csum_hwoffload_help(struct sk_buff *skb,
> return 0;
>
> 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))
> + goto sw_checksum;
skb_network_header_len requires skb->transport_header to be set.
This is not true for all egress packets. See for instance commit
d2aa125d6290 ("net: Don't set transport offset to invalid value").
But it should be true for all CHECKSUM_PARTIAL packets. See for
instance skb_partial_csum_set. So LGTM.
Just calling this out as it is not obvious and in case someone
does know a counter example of CHECKSUM_PARTIAL and
!skb_transport_header_was_set.
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH v2 net] net: skip offload for NETIF_F_IPV6_CSUM if ipv6 header contains extension
2024-10-24 14:01 [PATCH v2 net] net: skip offload for NETIF_F_IPV6_CSUM if ipv6 header contains extension Benoît Monin
2024-10-25 14:55 ` Willem de Bruijn
@ 2024-12-23 10:40 ` Eric Dumazet
2024-12-31 10:56 ` Willem de Bruijn
2024-12-31 15:24 ` Benoît Monin
1 sibling, 2 replies; 6+ messages in thread
From: Eric Dumazet @ 2024-12-23 10:40 UTC (permalink / raw)
To: Benoît Monin
Cc: David S. Miller, Jakub Kicinski, Paolo Abeni, Jiri Pirko,
Sebastian Andrzej Siewior, Lorenzo Bianconi, netdev, linux-kernel
On Thu, Oct 24, 2024 at 4:01 PM Benoît Monin <benoit.monin@gmx.fr> wrote:
>
> As documented in skbuff.h, devices with NETIF_F_IPV6_CSUM capability
> can only checksum TCP and UDP over IPv6 if the IP header does not
> contains extension.
>
> This is enforced for UDP packets emitted from user-space to an IPv6
> address as they go through ip6_make_skb(), which calls
> __ip6_append_data() where a check is done on the header size before
> setting CHECKSUM_PARTIAL.
>
> But the introduction of UDP encapsulation with fou6 added a code-path
> where it is possible to get an skb with a partial UDP checksum and an
> IPv6 header with extension:
> * fou6 adds a UDP header with a partial checksum if the inner packet
> does not contains a valid checksum.
> * ip6_tunnel adds an IPv6 header with a destination option extension
> header if encap_limit is non-zero (the default value is 4).
>
> The thread linked below describes in more details how to reproduce the
> problem with GRE-in-UDP tunnel.
>
> Add a check on the network header size in skb_csum_hwoffload_help() to
> make sure no IPv6 packet with extension header is handed to a network
> device with NETIF_F_IPV6_CSUM capability.
>
> Link: https://lore.kernel.org/netdev/26548921.1r3eYUQgxm@benoit.monin/T/#u
> Fixes: aa3463d65e7b ("fou: Add encap ops for IPv6 tunnels")
> Signed-off-by: Benoît Monin <benoit.monin@gmx.fr>
> ---
> changelog
> * v2:
> - patch against net instead of net-next
> - clarify documentation of NETIF_F_IPV6_CSUM
> - add link to thread describing the problem
> - add fixes tag
> - use vlan_get_protocol to check for IPv6
> * v1:
> - https://lore.kernel.org/netdev/0dc0c2af98e96b1df20bd36aeaed4eb4e27d507e.1728056028.git.benoit.monin@gmx.fr/T/#u
> ---
> net/core/dev.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/net/core/dev.c b/net/core/dev.c
> index ea5fbcd133ae..8453e14d301b 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -3639,6 +3639,9 @@ int skb_csum_hwoffload_help(struct sk_buff *skb,
> return 0;
>
> 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))
> + goto sw_checksum;
> switch (skb->csum_offset) {
> case offsetof(struct tcphdr, check):
> case offsetof(struct udphdr, check):
> @@ -3646,6 +3649,7 @@ int skb_csum_hwoffload_help(struct sk_buff *skb,
> }
> }
>
> +sw_checksum:
> return skb_checksum_help(skb);
> }
> EXPORT_SYMBOL(skb_csum_hwoffload_help);
FYI, this patch broke BIG TCP over IPv6.
[ 239.698598] Oops skb_network_header_len()=48 skb->len=67210
[ 239.704122] skb len=67210 headroom=162 headlen=94 tailroom=0
mac=(162,14) mac_len=0 net=(176,48) trans=224
shinfo(txflags=0 nr_frags=3 gso(size=1428 type=16 segs=47))
csum(0x1000e0 start=224 offset=16 ip_summed=3
complete_sw=0 valid=0 level=0)
hash(0xadf29e31 sw=0 l4=1) proto=0x86dd pkttype=0 iif=0
priority=0x18020 mark=0x0 alloc_cpu=46 vlan_all=0x0
encapsulation=0 inner(proto=0x0000, mac=0, net=0,
trans=0)\x00, net=0, trans=0)
[ 239.704153] dev name=eth2 feat=0x0000030000114ab3
[ 239.704155] sk family=10 type=1 proto=6
[ 239.704156] skb linear: 00000000: 02 32 00 00 00 00 94 eb 2c 18
9c d8 86 dd 60 2d
[ 239.704157] skb linear: 00000010: 31 9e 00 00 00 7f 20 02 0a 0d
87 01 00 00 00 00
[ 239.704158] skb linear: 00000020: 00 00 00 00 00 00 20 02 0a 05
68 30 1f 86 00 00
[ 239.704159] skb linear: 00000030: 00 00 00 00 00 00 06 00 c2 04
00 01 06 54 ac 4c
[ 239.704160] skb linear: 00000040: 81 9b 82 a6 d6 74 ca 75 8d 24
80 18 00 42 69 21
[ 239.704161] skb linear: 00000050: 00 00 01 01 08 0a 1b fe e2 2a ca 8f 78 6e
[ 239.704162] skb frag: 00000000: 6e 65 74 70 65 72 66 00 6e 65
74 70 65 72 66 00
[ 239.704163] skb frag: 00000010: 6e 65 74 70 65 72 66 00 6e 65
74 70 65 72 66 00
[ 239.704163] skb frag: 00000020: 6e 65 74 70 65 72 66 00 6e 65
74 70 65 72 66 00
[ 239.704164] skb frag: 00000030: 6e 65 74 70 65 72 66 00 6e 65
74 70 65 72 66 00
[ 239.704165] skb frag: 00000040: 6e 65 74 70 65 72 66 00 6e 65
74 70 65 72 66 00
[ 239.704166] skb frag: 00000050: 6e 65 74 70 65 72 66 00 6e 65
74 70 65 72 66 00
[ 239.704166] skb frag: 00000060: 6e 65 74 70 65 72 66 00 6e 65
74 70 65 72 66 00
[ 239.704167] skb frag: 00000070: 6e 65
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH v2 net] net: skip offload for NETIF_F_IPV6_CSUM if ipv6 header contains extension
2024-12-23 10:40 ` Eric Dumazet
@ 2024-12-31 10:56 ` Willem de Bruijn
2024-12-31 15:24 ` Benoît Monin
1 sibling, 0 replies; 6+ messages in thread
From: Willem de Bruijn @ 2024-12-31 10:56 UTC (permalink / raw)
To: Eric Dumazet, Benoît Monin
Cc: David S. Miller, Jakub Kicinski, Paolo Abeni, Jiri Pirko,
Sebastian Andrzej Siewior, Lorenzo Bianconi, netdev, linux-kernel
Eric Dumazet wrote:
> On Thu, Oct 24, 2024 at 4:01 PM Benoît Monin <benoit.monin@gmx.fr> wrote:
> >
> > As documented in skbuff.h, devices with NETIF_F_IPV6_CSUM capability
> > can only checksum TCP and UDP over IPv6 if the IP header does not
> > contains extension.
> >
> > This is enforced for UDP packets emitted from user-space to an IPv6
> > address as they go through ip6_make_skb(), which calls
> > __ip6_append_data() where a check is done on the header size before
> > setting CHECKSUM_PARTIAL.
> >
> > But the introduction of UDP encapsulation with fou6 added a code-path
> > where it is possible to get an skb with a partial UDP checksum and an
> > IPv6 header with extension:
> > * fou6 adds a UDP header with a partial checksum if the inner packet
> > does not contains a valid checksum.
> > * ip6_tunnel adds an IPv6 header with a destination option extension
> > header if encap_limit is non-zero (the default value is 4).
> >
> > The thread linked below describes in more details how to reproduce the
> > problem with GRE-in-UDP tunnel.
> >
> > Add a check on the network header size in skb_csum_hwoffload_help() to
> > make sure no IPv6 packet with extension header is handed to a network
> > device with NETIF_F_IPV6_CSUM capability.
> >
> > Link: https://lore.kernel.org/netdev/26548921.1r3eYUQgxm@benoit.monin/T/#u
> > Fixes: aa3463d65e7b ("fou: Add encap ops for IPv6 tunnels")
> > Signed-off-by: Benoît Monin <benoit.monin@gmx.fr>
> > ---
> > changelog
> > * v2:
> > - patch against net instead of net-next
> > - clarify documentation of NETIF_F_IPV6_CSUM
> > - add link to thread describing the problem
> > - add fixes tag
> > - use vlan_get_protocol to check for IPv6
> > * v1:
> > - https://lore.kernel.org/netdev/0dc0c2af98e96b1df20bd36aeaed4eb4e27d507e.1728056028.git.benoit.monin@gmx.fr/T/#u
> > ---
> > net/core/dev.c | 4 ++++
> > 1 file changed, 4 insertions(+)
> >
> > diff --git a/net/core/dev.c b/net/core/dev.c
> > index ea5fbcd133ae..8453e14d301b 100644
> > --- a/net/core/dev.c
> > +++ b/net/core/dev.c
> > @@ -3639,6 +3639,9 @@ int skb_csum_hwoffload_help(struct sk_buff *skb,
> > return 0;
> >
> > 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))
> > + goto sw_checksum;
> > switch (skb->csum_offset) {
> > case offsetof(struct tcphdr, check):
> > case offsetof(struct udphdr, check):
> > @@ -3646,6 +3649,7 @@ int skb_csum_hwoffload_help(struct sk_buff *skb,
> > }
> > }
> >
> > +sw_checksum:
> > return skb_checksum_help(skb);
> > }
> > EXPORT_SYMBOL(skb_csum_hwoffload_help);
>
>
> FYI, this patch broke BIG TCP over IPv6.
>
> [ 239.698598] Oops skb_network_header_len()=48 skb->len=67210
> [ 239.704122] skb len=67210 headroom=162 headlen=94 tailroom=0
> mac=(162,14) mac_len=0 net=(176,48) trans=224
> shinfo(txflags=0 nr_frags=3 gso(size=1428 type=16 segs=47))
> csum(0x1000e0 start=224 offset=16 ip_summed=3
> complete_sw=0 valid=0 level=0)
> hash(0xadf29e31 sw=0 l4=1) proto=0x86dd pkttype=0 iif=0
> priority=0x18020 mark=0x0 alloc_cpu=46 vlan_all=0x0
> encapsulation=0 inner(proto=0x0000, mac=0, net=0,
I'm looking into the following fix
+++ b/net/core/dev.c
@@ -3642,7 +3642,8 @@ 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;
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH v2 net] net: skip offload for NETIF_F_IPV6_CSUM if ipv6 header contains extension
2024-12-23 10:40 ` Eric Dumazet
2024-12-31 10:56 ` Willem de Bruijn
@ 2024-12-31 15:24 ` Benoît Monin
2024-12-31 16:00 ` Eric Dumazet
1 sibling, 1 reply; 6+ messages in thread
From: Benoît Monin @ 2024-12-31 15:24 UTC (permalink / raw)
To: Eric Dumazet
Cc: David S. Miller, Jakub Kicinski, Paolo Abeni, Jiri Pirko,
Sebastian Andrzej Siewior, Lorenzo Bianconi, netdev, linux-kernel
Hi,
23/12/2024 Eric Dumazet wrote:
[...]
>
> FYI, this patch broke BIG TCP over IPv6.
>
> [ 239.698598] Oops skb_network_header_len()=48 skb->len=67210
> [ 239.704122] skb len=67210 headroom=162 headlen=94 tailroom=0
> mac=(162,14) mac_len=0 net=(176,48) trans=224
> shinfo(txflags=0 nr_frags=3 gso(size=1428 type=16 segs=47))
> csum(0x1000e0 start=224 offset=16 ip_summed=3
> complete_sw=0 valid=0 level=0)
> hash(0xadf29e31 sw=0 l4=1) proto=0x86dd pkttype=0 iif=0
> priority=0x18020 mark=0x0 alloc_cpu=46 vlan_all=0x0
> encapsulation=0 inner(proto=0x0000, mac=0, net=0,
> trans=0)\x00, net=0, trans=0)
> [ 239.704153] dev name=eth2 feat=0x0000030000114ab3
> [ 239.704155] sk family=10 type=1 proto=6
[...]
What is the driver of eth2?
Since it was working before the patch, it means that the hardware is able to
deal with variable-sized IP header. So shouldn't its features contains
NETIF_F_HW_CSUM instead of NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM?
--
Benoît
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2 net] net: skip offload for NETIF_F_IPV6_CSUM if ipv6 header contains extension
2024-12-31 15:24 ` Benoît Monin
@ 2024-12-31 16:00 ` Eric Dumazet
0 siblings, 0 replies; 6+ messages in thread
From: Eric Dumazet @ 2024-12-31 16:00 UTC (permalink / raw)
To: Benoît Monin
Cc: David S. Miller, Jakub Kicinski, Paolo Abeni, Jiri Pirko,
Sebastian Andrzej Siewior, Lorenzo Bianconi, netdev, linux-kernel
On Tue, Dec 31, 2024 at 4:24 PM Benoît Monin <benoit.monin@gmx.fr> wrote:
>
> Hi,
>
> 23/12/2024 Eric Dumazet wrote:
> [...]
> >
> > FYI, this patch broke BIG TCP over IPv6.
> >
> > [ 239.698598] Oops skb_network_header_len()=48 skb->len=67210
> > [ 239.704122] skb len=67210 headroom=162 headlen=94 tailroom=0
> > mac=(162,14) mac_len=0 net=(176,48) trans=224
> > shinfo(txflags=0 nr_frags=3 gso(size=1428 type=16 segs=47))
> > csum(0x1000e0 start=224 offset=16 ip_summed=3
> > complete_sw=0 valid=0 level=0)
> > hash(0xadf29e31 sw=0 l4=1) proto=0x86dd pkttype=0 iif=0
> > priority=0x18020 mark=0x0 alloc_cpu=46 vlan_all=0x0
> > encapsulation=0 inner(proto=0x0000, mac=0, net=0,
> > trans=0)\x00, net=0, trans=0)
> > [ 239.704153] dev name=eth2 feat=0x0000030000114ab3
> > [ 239.704155] sk family=10 type=1 proto=6
> [...]
> What is the driver of eth2?
> Since it was working before the patch, it means that the hardware is able to
> deal with variable-sized IP header. So shouldn't its features contains
> NETIF_F_HW_CSUM instead of NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM?
>
Drivers supporting BIG TCP ipv6 remove the hop by hop header before
the packet hits the NIC
commit 1169a64265c4ea7100091228c98d4267f041b0e7
Author: Eric Dumazet <edumazet@google.com>
Date: Fri May 13 11:34:07 2022 -0700
mlx4: support BIG TCP packets
Rationale is explained in https://netdevconf.info/0x15/slides/35/BIG%20TCP.pdf
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2024-12-31 16:01 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-10-24 14:01 [PATCH v2 net] net: skip offload for NETIF_F_IPV6_CSUM if ipv6 header contains extension Benoît Monin
2024-10-25 14:55 ` Willem de Bruijn
2024-12-23 10:40 ` Eric Dumazet
2024-12-31 10:56 ` Willem de Bruijn
2024-12-31 15:24 ` Benoît Monin
2024-12-31 16:00 ` Eric Dumazet
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).