* [PATCH] net: Reduce skb_warn_bad_offload() noise.
@ 2017-08-04 13:11 Tonghao Zhang
2017-08-04 13:29 ` Eric Dumazet
0 siblings, 1 reply; 9+ messages in thread
From: Tonghao Zhang @ 2017-08-04 13:11 UTC (permalink / raw)
To: netdev; +Cc: Tonghao Zhang, Eric Dumazet, Willem de Bruijn, Pravin B Shelar
This patch will revert the b2504a5dbe "net: reduce
skb_warn_bad_offload() noise". The ovs will call the
__skb_gso_segment() with tx false. When segmenting UDP with UFO,
the __skb_gso_segment raises a warning as below [1], because the
ip_summed is CHECKSUM_NONE. While the net-next has removed the
UFO support, but the 4.11 and 4.12 kernel don't address that problem.
In the kernel, only qdisc_pkt_len_init() (__dev_queue_xmit call it.)
uses the SKB_GSO_DODGY to do something. Other places just set it.
The warn described in b2504a5dbe is shown [2]. We may know that:
1. the net_device don’t have qdisc.
2. the skb->ip_summed was changed to CHECKSUM_NONE. it maybe
changed in skb_checksum_help() when calling validate_xmit_skb().
or other place.
And we should not revert the 6e7bc478c9 "net: skb_needs_check() accepts
CHECKSUM_NONE for tx". The check is necessary.
[1]
[321428.168903] WARNING: CPU: 0 PID: 2279 at net/core/dev.c:2562
skb_warn_bad_offload+0xc4/0x110
[321428.168906] san0: caps=(0x000004009fbb58e9, 0x0000000000000000) len=6769
data_len=6727 gso_size=1480 gso_type=2 ip_summed=0
[321428.168955] CPU: 0 PID: 2279 Comm: ruby-mri 4.11.12-200.fc25.x86_64 #1
[321428.168956] Hardware name: Supermicro SYS-1028U-TNRTP+/X10DRU-i+, BIOS 1.1 07/22/2015
[321428.168957] Call Trace:
[321428.168962] dump_stack+0x63/0x86
[321428.168965] __warn+0xcb/0xf0
[321428.168966] warn_slowpath_fmt+0x5a/0x80
[321428.168968] skb_warn_bad_offload+0xc4/0x110
[321428.168970] __skb_gso_segment+0x190/0x1a0
[321428.168977] queue_gso_packets+0x62/0x160 [openvswitch]
[321428.168992] ovs_dp_upcall+0x31/0x60 [openvswitch]
[321428.168994] ovs_dp_process_packet+0x10d/0x130 [openvswitch]
[321428.168997] ovs_vport_receive+0x76/0xd0 [openvswitch]
[321428.169013] internal_dev_xmit+0x28/0x60 [openvswitch]
[321428.169014] dev_hard_start_xmit+0xa3/0x1f0
[321428.169016] __dev_queue_xmit+0x592/0x650
[321428.169026] dev_queue_xmit+0x10/0x20
[2]
WARNING: CPU: 1 PID: 6768 at net/core/dev.c:2439 skb_warn_bad_offload+0x2af/0x390 net/core/dev.c:2434
lo: caps=(0x000000a2803b7c69, 0x0000000000000000) len=138 data_len=0 gso_size=15883 gso_type=4 ip_summed=0
Kernel panic - not syncing: panic_on_warn set ...
CPU: 1 PID: 6768 Comm: syz-executor1 Not tainted 4.9.0 #5
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011
ffff8801c063ecd8 ffffffff82346bdf ffffffff00000001 1ffff100380c7d2e
ffffed00380c7d26 0000000041b58ab3 ffffffff84b37e38 ffffffff823468f1
ffffffff84820740 ffffffff84f289c0 dffffc0000000000 ffff8801c063ee20
Call Trace:
[<ffffffff82346bdf>] __dump_stack lib/dump_stack.c:15 [inline]
[<ffffffff82346bdf>] dump_stack+0x2ee/0x3ef lib/dump_stack.c:51
[<ffffffff81827e34>] panic+0x1fb/0x412 kernel/panic.c:179
[<ffffffff8141f704>] __warn+0x1c4/0x1e0 kernel/panic.c:542
[<ffffffff8141f7e5>] warn_slowpath_fmt+0xc5/0x100 kernel/panic.c:565
[<ffffffff8356cbaf>] skb_warn_bad_offload+0x2af/0x390 net/core/dev.c:2434
[<ffffffff83585cd2>] __skb_gso_segment+0x482/0x780 net/core/dev.c:2706
[<ffffffff83586f19>] skb_gso_segment include/linux/netdevice.h:3985 [inline]
[<ffffffff83586f19>] validate_xmit_skb+0x5c9/0xc20 net/core/dev.c:2969
[<ffffffff835892bb>] __dev_queue_xmit+0xe6b/0x1e70 net/core/dev.c:3383
[<ffffffff8358a2d7>] dev_queue_xmit+0x17/0x20 net/core/dev.c:3424
Cc: Eric Dumazet <edumazet@google.com>
Cc: Willem de Bruijn <willemb@google.com>
Cc: Pravin B Shelar <pshelar@ovn.org>
Signed-off-by: Tonghao Zhang <xiangxia.m.yue@gmail.com>
---
net/core/dev.c | 11 +++--------
1 file changed, 3 insertions(+), 8 deletions(-)
diff --git a/net/core/dev.c b/net/core/dev.c
index 1d75499..97e6989 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -2753,11 +2753,11 @@ static inline bool skb_needs_check(struct sk_buff *skb, bool tx_path)
struct sk_buff *__skb_gso_segment(struct sk_buff *skb,
netdev_features_t features, bool tx_path)
{
- struct sk_buff *segs;
-
if (unlikely(skb_needs_check(skb, tx_path))) {
int err;
+ skb_warn_bad_offload(skb);
+
/* We're going to init ->check field in TCP or UDP header */
err = skb_cow_head(skb, 0);
if (err < 0)
@@ -2786,12 +2786,7 @@ struct sk_buff *__skb_gso_segment(struct sk_buff *skb,
skb_reset_mac_header(skb);
skb_reset_mac_len(skb);
- segs = skb_mac_gso_segment(skb, features);
-
- if (unlikely(skb_needs_check(skb, tx_path)))
- skb_warn_bad_offload(skb);
-
- return segs;
+ return skb_mac_gso_segment(skb, features);
}
EXPORT_SYMBOL(__skb_gso_segment);
--
1.8.3.1
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] net: Reduce skb_warn_bad_offload() noise.
2017-08-04 13:11 [PATCH] net: Reduce skb_warn_bad_offload() noise Tonghao Zhang
@ 2017-08-04 13:29 ` Eric Dumazet
2017-08-07 0:37 ` Tonghao Zhang
0 siblings, 1 reply; 9+ messages in thread
From: Eric Dumazet @ 2017-08-04 13:29 UTC (permalink / raw)
To: Tonghao Zhang; +Cc: netdev, Eric Dumazet, Willem de Bruijn, Pravin B Shelar
On Fri, 2017-08-04 at 06:11 -0700, Tonghao Zhang wrote:
> This patch will revert the b2504a5dbe "net: reduce
> skb_warn_bad_offload() noise". The ovs will call the
> __skb_gso_segment() with tx false. When segmenting UDP with UFO,
> the __skb_gso_segment raises a warning as below [1], because the
> ip_summed is CHECKSUM_NONE. While the net-next has removed the
> UFO support, but the 4.11 and 4.12 kernel don't address that problem.
>
> In the kernel, only qdisc_pkt_len_init() (__dev_queue_xmit call it.)
> uses the SKB_GSO_DODGY to do something. Other places just set it.
> The warn described in b2504a5dbe is shown [2]. We may know that:
> 1. the net_device don’t have qdisc.
> 2. the skb->ip_summed was changed to CHECKSUM_NONE. it maybe
> changed in skb_checksum_help() when calling validate_xmit_skb().
> or other place.
>
> And we should not revert the 6e7bc478c9 "net: skb_needs_check() accepts
> CHECKSUM_NONE for tx". The check is necessary.
Why is it necessary ?
If you revert b2504a5dbe, then we also need to revert 6e7bc478c9,
unless you provide hard facts.
>
> [1]
> [321428.168903] WARNING: CPU: 0 PID: 2279 at net/core/dev.c:2562
> skb_warn_bad_offload+0xc4/0x110
> [321428.168906] san0: caps=(0x000004009fbb58e9, 0x0000000000000000) len=6769
> data_len=6727 gso_size=1480 gso_type=2 ip_summed=0
>
> [321428.168955] CPU: 0 PID: 2279 Comm: ruby-mri 4.11.12-200.fc25.x86_64 #1
> [321428.168956] Hardware name: Supermicro SYS-1028U-TNRTP+/X10DRU-i+, BIOS 1.1 07/22/2015
> [321428.168957] Call Trace:
> [321428.168962] dump_stack+0x63/0x86
> [321428.168965] __warn+0xcb/0xf0
> [321428.168966] warn_slowpath_fmt+0x5a/0x80
> [321428.168968] skb_warn_bad_offload+0xc4/0x110
> [321428.168970] __skb_gso_segment+0x190/0x1a0
> [321428.168977] queue_gso_packets+0x62/0x160 [openvswitch]
> [321428.168992] ovs_dp_upcall+0x31/0x60 [openvswitch]
> [321428.168994] ovs_dp_process_packet+0x10d/0x130 [openvswitch]
> [321428.168997] ovs_vport_receive+0x76/0xd0 [openvswitch]
> [321428.169013] internal_dev_xmit+0x28/0x60 [openvswitch]
> [321428.169014] dev_hard_start_xmit+0xa3/0x1f0
> [321428.169016] __dev_queue_xmit+0x592/0x650
> [321428.169026] dev_queue_xmit+0x10/0x20
>
> [2]
> WARNING: CPU: 1 PID: 6768 at net/core/dev.c:2439 skb_warn_bad_offload+0x2af/0x390 net/core/dev.c:2434
> lo: caps=(0x000000a2803b7c69, 0x0000000000000000) len=138 data_len=0 gso_size=15883 gso_type=4 ip_summed=0
> Kernel panic - not syncing: panic_on_warn set ...
>
> CPU: 1 PID: 6768 Comm: syz-executor1 Not tainted 4.9.0 #5
> Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011
> ffff8801c063ecd8 ffffffff82346bdf ffffffff00000001 1ffff100380c7d2e
> ffffed00380c7d26 0000000041b58ab3 ffffffff84b37e38 ffffffff823468f1
> ffffffff84820740 ffffffff84f289c0 dffffc0000000000 ffff8801c063ee20
> Call Trace:
Why are you adding this trace that was part of the b2504a5dbef3
changelog ?
> [<ffffffff82346bdf>] __dump_stack lib/dump_stack.c:15 [inline]
> [<ffffffff82346bdf>] dump_stack+0x2ee/0x3ef lib/dump_stack.c:51
> [<ffffffff81827e34>] panic+0x1fb/0x412 kernel/panic.c:179
> [<ffffffff8141f704>] __warn+0x1c4/0x1e0 kernel/panic.c:542
> [<ffffffff8141f7e5>] warn_slowpath_fmt+0xc5/0x100 kernel/panic.c:565
> [<ffffffff8356cbaf>] skb_warn_bad_offload+0x2af/0x390 net/core/dev.c:2434
> [<ffffffff83585cd2>] __skb_gso_segment+0x482/0x780 net/core/dev.c:2706
> [<ffffffff83586f19>] skb_gso_segment include/linux/netdevice.h:3985 [inline]
> [<ffffffff83586f19>] validate_xmit_skb+0x5c9/0xc20 net/core/dev.c:2969
> [<ffffffff835892bb>] __dev_queue_xmit+0xe6b/0x1e70 net/core/dev.c:3383
> [<ffffffff8358a2d7>] dev_queue_xmit+0x17/0x20 net/core/dev.c:3424
>
> Cc: Eric Dumazet <edumazet@google.com>
> Cc: Willem de Bruijn <willemb@google.com>
> Cc: Pravin B Shelar <pshelar@ovn.org>
> Signed-off-by: Tonghao Zhang <xiangxia.m.yue@gmail.com>
> ---
IMO, this description is too confusing, I do not understand this patch.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] net: Reduce skb_warn_bad_offload() noise.
2017-08-04 13:29 ` Eric Dumazet
@ 2017-08-07 0:37 ` Tonghao Zhang
2017-08-07 4:42 ` Willem de Bruijn
0 siblings, 1 reply; 9+ messages in thread
From: Tonghao Zhang @ 2017-08-07 0:37 UTC (permalink / raw)
To: Eric Dumazet
Cc: Linux Kernel Network Developers, Eric Dumazet, Willem de Bruijn,
Pravin B Shelar
On Fri, Aug 4, 2017 at 9:29 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> On Fri, 2017-08-04 at 06:11 -0700, Tonghao Zhang wrote:
>> This patch will revert the b2504a5dbe "net: reduce
>> skb_warn_bad_offload() noise". The ovs will call the
>> __skb_gso_segment() with tx false. When segmenting UDP with UFO,
>> the __skb_gso_segment raises a warning as below [1], because the
>> ip_summed is CHECKSUM_NONE. While the net-next has removed the
>> UFO support, but the 4.11 and 4.12 kernel don't address that problem.
>>
>> In the kernel, only qdisc_pkt_len_init() (__dev_queue_xmit call it.)
>> uses the SKB_GSO_DODGY to do something. Other places just set it.
>> The warn described in b2504a5dbe is shown [2]. We may know that:
>> 1. the net_device don’t have qdisc.
>> 2. the skb->ip_summed was changed to CHECKSUM_NONE. it maybe
>> changed in skb_checksum_help() when calling validate_xmit_skb().
>> or other place.
>>
>> And we should not revert the 6e7bc478c9 "net: skb_needs_check() accepts
>> CHECKSUM_NONE for tx". The check is necessary.
>
> Why is it necessary ?
I am not familiar with __skb_gso_segment() when tx_path == true. I
should say sorry to you.
>
> If you revert b2504a5dbe, then we also need to revert 6e7bc478c9,
> unless you provide hard facts.
>
The openvswitch kernel module calls the __skb_gso_segment()(and sets
tx_path = false) when passing packets to userspace. The UFO will set
the ip_summed to CHECKSUM_NONE. There are a lot of warn logs. The warn
log is shown as below. I guess we should revert the patch.
>>
>> [1]
>> [321428.168903] WARNING: CPU: 0 PID: 2279 at net/core/dev.c:2562
>> skb_warn_bad_offload+0xc4/0x110
>> [321428.168906] san0: caps=(0x000004009fbb58e9, 0x0000000000000000) len=6769
>> data_len=6727 gso_size=1480 gso_type=2 ip_summed=0
>>
>> [321428.168955] CPU: 0 PID: 2279 Comm: ruby-mri 4.11.12-200.fc25.x86_64 #1
>> [321428.168956] Hardware name: Supermicro SYS-1028U-TNRTP+/X10DRU-i+, BIOS 1.1 07/22/2015
>> [321428.168957] Call Trace:
>> [321428.168962] dump_stack+0x63/0x86
>> [321428.168965] __warn+0xcb/0xf0
>> [321428.168966] warn_slowpath_fmt+0x5a/0x80
>> [321428.168968] skb_warn_bad_offload+0xc4/0x110
>> [321428.168970] __skb_gso_segment+0x190/0x1a0
>> [321428.168977] queue_gso_packets+0x62/0x160 [openvswitch]
>> [321428.168992] ovs_dp_upcall+0x31/0x60 [openvswitch]
>> [321428.168994] ovs_dp_process_packet+0x10d/0x130 [openvswitch]
>> [321428.168997] ovs_vport_receive+0x76/0xd0 [openvswitch]
>> [321428.169013] internal_dev_xmit+0x28/0x60 [openvswitch]
>> [321428.169014] dev_hard_start_xmit+0xa3/0x1f0
>> [321428.169016] __dev_queue_xmit+0x592/0x650
>> [321428.169026] dev_queue_xmit+0x10/0x20
>>
>> [2]
>> WARNING: CPU: 1 PID: 6768 at net/core/dev.c:2439 skb_warn_bad_offload+0x2af/0x390 net/core/dev.c:2434
>> lo: caps=(0x000000a2803b7c69, 0x0000000000000000) len=138 data_len=0 gso_size=15883 gso_type=4 ip_summed=0
>> Kernel panic - not syncing: panic_on_warn set ...
>>
>> CPU: 1 PID: 6768 Comm: syz-executor1 Not tainted 4.9.0 #5
>> Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011
>> ffff8801c063ecd8 ffffffff82346bdf ffffffff00000001 1ffff100380c7d2e
>> ffffed00380c7d26 0000000041b58ab3 ffffffff84b37e38 ffffffff823468f1
>> ffffffff84820740 ffffffff84f289c0 dffffc0000000000 ffff8801c063ee20
>> Call Trace:
>
> Why are you adding this trace that was part of the b2504a5dbef3
> changelog ?
I hope we can fix the bug with other solution.
>
>> [<ffffffff82346bdf>] __dump_stack lib/dump_stack.c:15 [inline]
>> [<ffffffff82346bdf>] dump_stack+0x2ee/0x3ef lib/dump_stack.c:51
>> [<ffffffff81827e34>] panic+0x1fb/0x412 kernel/panic.c:179
>> [<ffffffff8141f704>] __warn+0x1c4/0x1e0 kernel/panic.c:542
>> [<ffffffff8141f7e5>] warn_slowpath_fmt+0xc5/0x100 kernel/panic.c:565
>> [<ffffffff8356cbaf>] skb_warn_bad_offload+0x2af/0x390 net/core/dev.c:2434
>> [<ffffffff83585cd2>] __skb_gso_segment+0x482/0x780 net/core/dev.c:2706
>> [<ffffffff83586f19>] skb_gso_segment include/linux/netdevice.h:3985 [inline]
>> [<ffffffff83586f19>] validate_xmit_skb+0x5c9/0xc20 net/core/dev.c:2969
>> [<ffffffff835892bb>] __dev_queue_xmit+0xe6b/0x1e70 net/core/dev.c:3383
>> [<ffffffff8358a2d7>] dev_queue_xmit+0x17/0x20 net/core/dev.c:3424
>>
>> Cc: Eric Dumazet <edumazet@google.com>
>> Cc: Willem de Bruijn <willemb@google.com>
>> Cc: Pravin B Shelar <pshelar@ovn.org>
>> Signed-off-by: Tonghao Zhang <xiangxia.m.yue@gmail.com>
>> ---
>
>
> IMO, this description is too confusing, I do not understand this patch.
sorry
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] net: Reduce skb_warn_bad_offload() noise.
2017-08-07 0:37 ` Tonghao Zhang
@ 2017-08-07 4:42 ` Willem de Bruijn
2017-08-07 23:44 ` Tonghao Zhang
0 siblings, 1 reply; 9+ messages in thread
From: Willem de Bruijn @ 2017-08-07 4:42 UTC (permalink / raw)
To: Tonghao Zhang
Cc: Eric Dumazet, Linux Kernel Network Developers, Eric Dumazet,
Willem de Bruijn, Pravin B Shelar
> The openvswitch kernel module calls the __skb_gso_segment()(and sets
> tx_path = false) when passing packets to userspace. The UFO will set
> the ip_summed to CHECKSUM_NONE. There are a lot of warn logs. The warn
> log is shown as below. I guess we should revert the patch.
Indeed, the software UFO code computes the checksum and
sets ip_summed to CHECKSUM_NONE, as is correct on the
egress path.
Commit 6e7bc478c9a0 ("net: skb_needs_check() accepts
CHECKSUM_NONE for tx") revised the tx_path case in
skb_needs_check to avoid the warning exactly for the UFO case.
We cannot just make an exception for CHECKSUM_NONE in the
!tx_path case, as the entire statement then becomes false:
return skb->ip_summed == CHECKSUM_NONE;
Since on egress CHECKSUM_UNNECESSARY is equivalent to
CHECKSUM_NONE, it should be fine to update the UFO code
to set that, instead:
@@ -235,7 +235,7 @@ static struct sk_buff *udp4_ufo_fragment(struct
sk_buff *skb,
if (uh->check == 0)
uh->check = CSUM_MANGLED_0;
- skb->ip_summed = CHECKSUM_NONE;
+ skb->ip_summed = CHECKSUM_UNNECESSARY;
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] net: Reduce skb_warn_bad_offload() noise.
2017-08-07 4:42 ` Willem de Bruijn
@ 2017-08-07 23:44 ` Tonghao Zhang
2017-08-08 5:26 ` Tonghao Zhang
0 siblings, 1 reply; 9+ messages in thread
From: Tonghao Zhang @ 2017-08-07 23:44 UTC (permalink / raw)
To: Willem de Bruijn
Cc: Eric Dumazet, Linux Kernel Network Developers, Eric Dumazet,
Willem de Bruijn, Pravin B Shelar
That is fine to me. I have tested it. Thanks.
On Mon, Aug 7, 2017 at 12:42 PM, Willem de Bruijn
<willemdebruijn.kernel@gmail.com> wrote:
>> The openvswitch kernel module calls the __skb_gso_segment()(and sets
>> tx_path = false) when passing packets to userspace. The UFO will set
>> the ip_summed to CHECKSUM_NONE. There are a lot of warn logs. The warn
>> log is shown as below. I guess we should revert the patch.
>
> Indeed, the software UFO code computes the checksum and
> sets ip_summed to CHECKSUM_NONE, as is correct on the
> egress path.
>
> Commit 6e7bc478c9a0 ("net: skb_needs_check() accepts
> CHECKSUM_NONE for tx") revised the tx_path case in
> skb_needs_check to avoid the warning exactly for the UFO case.
>
> We cannot just make an exception for CHECKSUM_NONE in the
> !tx_path case, as the entire statement then becomes false:
>
> return skb->ip_summed == CHECKSUM_NONE;
>
> Since on egress CHECKSUM_UNNECESSARY is equivalent to
> CHECKSUM_NONE, it should be fine to update the UFO code
> to set that, instead:
>
> @@ -235,7 +235,7 @@ static struct sk_buff *udp4_ufo_fragment(struct
> sk_buff *skb,
> if (uh->check == 0)
> uh->check = CSUM_MANGLED_0;
>
> - skb->ip_summed = CHECKSUM_NONE;
> + skb->ip_summed = CHECKSUM_UNNECESSARY;
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] net: Reduce skb_warn_bad_offload() noise.
2017-08-07 23:44 ` Tonghao Zhang
@ 2017-08-08 5:26 ` Tonghao Zhang
2017-08-08 6:29 ` Willem de Bruijn
0 siblings, 1 reply; 9+ messages in thread
From: Tonghao Zhang @ 2017-08-08 5:26 UTC (permalink / raw)
To: Willem de Bruijn
Cc: Eric Dumazet, Linux Kernel Network Developers, Eric Dumazet,
Willem de Bruijn, Pravin B Shelar
Hi Willem
In a case, there is also warn info. The test topo is shown as below.
VM01: veth1 and eth0 in the VM01 are inserted to ovs br0.
veth0(IP: 172.16.34.100/24) —— veth1--br0--eth0
iperf3 -c 172.168.100.13 -i 1 -P 10 -t 10 -u -b 1000M -l 10K
VM02
eth0(IP: 172.16.34.200/24)
iperf3 -s
The warn info is shown as below [1]. If we change the CHECKSUM_NONE to
CHECKSUM_UNNECESSARY in the udp4_ufo_fragment().
and we should add a check in skb_needs_check() when outputting a packet.
diff --git a/net/core/dev.c b/net/core/dev.c
index 416137c..8fe12a7 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -2670,6 +2670,7 @@ static inline bool skb_needs_check(struct
sk_buff *skb, bool tx_path)
{
if (tx_path)
return skb->ip_summed != CHECKSUM_PARTIAL &&
+ skb->ip_summed != CHECKSUM_UNNECESSARY &&
skb->ip_summed != CHECKSUM_NONE;
return skb->ip_summed == CHECKSUM_NONE;
diff --git a/net/ipv4/udp_offload.c b/net/ipv4/udp_offload.c
index 7812501..0932c85 100644
--- a/net/ipv4/udp_offload.c
+++ b/net/ipv4/udp_offload.c
@@ -235,7 +235,7 @@ static struct sk_buff *udp4_ufo_fragment(struct
sk_buff *skb,
if (uh->check == 0)
uh->check = CSUM_MANGLED_0;
- skb->ip_summed = CHECKSUM_NONE;
+ skb->ip_summed = CHECKSUM_UNNECESSARY;
/* If there is no outer header we can fake a checksum offload
* due to the fact that we have already done the checksum in
[1]:
[ 1291.596232] vmxnet3: caps=(0x0000006000214ba9, 0x0000000000000000)
len=10282 data_len=10240 gso_size=1480 gso_type=2 ip_summed=1
[ 1291.596239] ------------[ cut here ]------------
[ 1291.596242] WARNING: CPU: 1 PID: 2203 at net/core/dev.c:2564
skb_warn_bad_offload+0xd3/0xde
[ 1291.596242] Modules linked in: veth udp_tunnel gre openvswitch
nf_conntrack_ipv6 nf_nat_ipv6 nf_conntrack_ipv4 nf_defrag_ipv4
nf_nat_ipv4 nf_defrag_ipv6 nf_nat nf_conntrack cfg80211 rfkill ext4
jbd2 mbcache sb_edac coretemp crct10dif_pclmul crc32_pclmul
ghash_clmulni_intel pcbc aesni_intel crypto_simd glue_helper ppdev
vmw_balloon cryptd vmw_vmci sg i2c_piix4 pcspkr parport_pc parport
shpchp ip_tables xfs libcrc32c sd_mod ata_generic pata_acpi vmwgfx
drm_kms_helper syscopyarea sysfillrect sysimgblt fb_sys_fops ttm drm
crc32c_intel serio_raw vmxnet3 ata_piix mptspi libata
scsi_transport_spi mptscsih mptbase i2c_core floppy dm_mirror
dm_region_hash dm_log dm_mod
[ 1291.596280] CPU: 1 PID: 2203 Comm: iperf3 Tainted: G W
4.12.0+ #1
[ 1291.596280] Hardware name: VMware, Inc. VMware Virtual
Platform/440BX Desktop Reference Platform, BIOS 6.00 09/21/2015
[ 1291.596281] task: ffff8be36f5e9680 task.stack: ffffb6c840d44000
[ 1291.596283] RIP: 0010:skb_warn_bad_offload+0xd3/0xde
[ 1291.596284] RSP: 0018:ffff8be3796438c8 EFLAGS: 00010246
[ 1291.596285] RAX: 0000000000000074 RBX: ffff8be363ff9f00 RCX: 0000000000000006
[ 1291.596286] RDX: 0000000000000000 RSI: 0000000000000086 RDI: ffff8be37964e0a0
[ 1291.596287] RBP: ffff8be3796438f0 R08: 0000000000000000 R09: 0000000000000bf2
[ 1291.596287] R10: 0000000000000004 R11: 0000000000000bf1 R12: ffff8be3660f0000
[ 1291.596288] R13: 0000000000000001 R14: 0000000000000000 R15: ffff8be3660f0000
[ 1291.596289] FS: 00007f8d93bdb740(0000) GS:ffff8be379640000(0000)
knlGS:0000000000000000
[ 1291.596290] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 1291.596291] CR2: 0000555f94a6e0c8 CR3: 000000012b84a000 CR4: 00000000001406e0
[ 1291.596296] Call Trace:
[ 1291.596297] <IRQ>
[ 1291.596300] __skb_gso_segment+0x15d/0x170
[ 1291.596301] validate_xmit_skb+0x12d/0x2b0
[ 1291.596303] validate_xmit_skb_list+0x42/0x70
[ 1291.596306] sch_direct_xmit+0xd0/0x1b0
[ 1291.596308] __dev_queue_xmit+0x42e/0x630
[ 1291.596310] ? hrtimer_interrupt+0xd2/0x1a0
[ 1291.596312] dev_queue_xmit+0x10/0x20
[ 1291.596315] ovs_vport_send+0xc2/0x150 [openvswitch]
[ 1291.596318] do_output+0x53/0xf0 [openvswitch]
[ 1291.596322] do_execute_actions+0x9bc/0x9d0 [openvswitch]
[ 1291.596324] ? __bpf_prog_run+0x385/0x1310
[ 1291.596327] ovs_execute_actions+0x40/0x120 [openvswitch]
[ 1291.596330] ovs_dp_process_packet+0x84/0x120 [openvswitch]
[ 1291.596333] ? ovs_ct_update_key+0x9a/0xe0 [openvswitch]
[ 1291.596336] ovs_vport_receive+0x73/0xd0 [openvswitch]
[ 1291.596339] ? handle_irq_event_percpu+0x54/0x80
[ 1291.596340] ? handle_irq_event+0x46/0x60
[ 1291.596342] ? handle_edge_irq+0x8d/0x130
[ 1291.596344] ? handle_irq+0xab/0x120
[ 1291.596346] ? irq_exit+0x77/0xf0
[ 1291.596348] ? do_IRQ+0x51/0xd0
[ 1291.596352] netdev_frame_hook+0xd3/0x160 [openvswitch]
[ 1291.596355] __netif_receive_skb_core+0x1da/0x9e0
[ 1291.596358] ? vport_netdev_free+0x30/0x30 [openvswitch]
[ 1291.596360] ? kfree_skbmem+0x5a/0x60
[ 1291.596361] ? consume_skb+0x34/0x90
[ 1291.596363] __netif_receive_skb+0x18/0x60
[ 1291.596365] process_backlog+0x95/0x140
[ 1291.596367] net_rx_action+0x26c/0x3b0
[ 1291.596369] __do_softirq+0xc9/0x269
[ 1291.596371] do_softirq_own_stack+0x1c/0x30
[ 1291.596372] </IRQ>
[ 1291.596374] do_softirq+0x50/0x60
[ 1291.596375] __local_bh_enable_ip+0x5a/0x70
[ 1291.596377] ip_finish_output2+0x15e/0x390
[ 1291.596380] ip_finish_output+0x136/0x1e0
[ 1291.596382] ip_output+0x76/0xe0
[ 1291.596384] ? ip_fragment.constprop.53+0x80/0x80
[ 1291.596386] ip_local_out+0x35/0x40
[ 1291.596387] ip_send_skb+0x19/0x40
[ 1291.596390] udp_send_skb+0x172/0x280
[ 1291.596391] udp_sendmsg+0x2cf/0xa60
[ 1291.596393] ? ip_reply_glue_bits+0x50/0x50
[ 1291.596396] ? sock_has_perm+0x75/0x90
[ 1291.596398] inet_sendmsg+0x31/0xb0
[ 1291.596399] sock_sendmsg+0x38/0x50
[ 1291.596401] sock_write_iter+0x85/0xf0
[ 1291.596403] __vfs_write+0xe3/0x160
[ 1291.596405] vfs_write+0xb2/0x1b0
[ 1291.596407] ? syscall_trace_enter+0x1d0/0x2b0
[ 1291.596408] SyS_write+0x55/0xc0
[ 1291.596411] do_syscall_64+0x67/0x150
[ 1291.596412] entry_SYSCALL64_slow_path+0x25/0x25
[ 1291.596413] RIP: 0033:0x7f8d931cdc60
[ 1291.596414] RSP: 002b:00007ffc0790e9c8 EFLAGS: 00000246 ORIG_RAX:
0000000000000001
[ 1291.596415] RAX: ffffffffffffffda RBX: 0000000000002800 RCX: 00007f8d931cdc60
[ 1291.596417] RDX: 0000000000002800 RSI: 00007f8d93bc9000 RDI: 0000000000000012
[ 1291.596417] RBP: 00007f8d93bc9000 R08: 0000000000000000 R09: 000000000000003d
[ 1291.596418] R10: 0000000000000001 R11: 0000000000000246 R12: 0000000000000012
[ 1291.596419] R13: 0000000000002800 R14: 0000000000000001 R15: 00007ffc0790ea40
[ 1291.596420] Code: 8d 94 24 d0 00 00 00 48 0f 44 ce 4d 85 e4 44 89
54 24 08 48 0f 44 d6 89 3c 24 48 89 c6 48 c7 c7 80 65 ac 8f 31 c0 e8
a7 bd a5 ff <0f> ff 48 83 c4 18 5b 41 5c 5d c3 55 48 89 f2 31 c0 89 fe
48 c7
[ 1291.596447] ---[ end trace b66534f41f98ceda ]---
On Tue, Aug 8, 2017 at 7:44 AM, Tonghao Zhang <xiangxia.m.yue@gmail.com> wrote:
> That is fine to me. I have tested it. Thanks.
>
> On Mon, Aug 7, 2017 at 12:42 PM, Willem de Bruijn
> <willemdebruijn.kernel@gmail.com> wrote:
>>> The openvswitch kernel module calls the __skb_gso_segment()(and sets
>>> tx_path = false) when passing packets to userspace. The UFO will set
>>> the ip_summed to CHECKSUM_NONE. There are a lot of warn logs. The warn
>>> log is shown as below. I guess we should revert the patch.
>>
>> Indeed, the software UFO code computes the checksum and
>> sets ip_summed to CHECKSUM_NONE, as is correct on the
>> egress path.
>>
>> Commit 6e7bc478c9a0 ("net: skb_needs_check() accepts
>> CHECKSUM_NONE for tx") revised the tx_path case in
>> skb_needs_check to avoid the warning exactly for the UFO case.
>>
>> We cannot just make an exception for CHECKSUM_NONE in the
>> !tx_path case, as the entire statement then becomes false:
>>
>> return skb->ip_summed == CHECKSUM_NONE;
>>
>> Since on egress CHECKSUM_UNNECESSARY is equivalent to
>> CHECKSUM_NONE, it should be fine to update the UFO code
>> to set that, instead:
>>
>> @@ -235,7 +235,7 @@ static struct sk_buff *udp4_ufo_fragment(struct
>> sk_buff *skb,
>> if (uh->check == 0)
>> uh->check = CSUM_MANGLED_0;
>>
>> - skb->ip_summed = CHECKSUM_NONE;
>> + skb->ip_summed = CHECKSUM_UNNECESSARY;
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] net: Reduce skb_warn_bad_offload() noise.
2017-08-08 5:26 ` Tonghao Zhang
@ 2017-08-08 6:29 ` Willem de Bruijn
2017-08-08 17:46 ` Willem de Bruijn
0 siblings, 1 reply; 9+ messages in thread
From: Willem de Bruijn @ 2017-08-08 6:29 UTC (permalink / raw)
To: Tonghao Zhang
Cc: Eric Dumazet, Linux Kernel Network Developers, Eric Dumazet,
Willem de Bruijn, Pravin B Shelar
> In a case, there is also warn info. The test topo is shown as below.
Thanks for testing the patch, Tonghao.
> The warn info is shown as below [1]. If we change the CHECKSUM_NONE to
> CHECKSUM_UNNECESSARY in the udp4_ufo_fragment().
> and we should add a check in skb_needs_check() when outputting a packet.
>
> diff --git a/net/core/dev.c b/net/core/dev.c
> index 416137c..8fe12a7 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -2670,6 +2670,7 @@ static inline bool skb_needs_check(struct
> sk_buff *skb, bool tx_path)
> {
> if (tx_path)
> return skb->ip_summed != CHECKSUM_PARTIAL &&
> + skb->ip_summed != CHECKSUM_UNNECESSARY &&
> skb->ip_summed != CHECKSUM_NONE;
Good catch. Only, the CHECKSUM_NONE case was added specifically to
work around this UFO issue on the tx path in commit 6e7bc478c9a0
("net: skb_needs_check() accepts CHECKSUM_NONE for tx"). If we change
the value generated by UFO, we can remove that statement, so
+ skb->ip_summed != CHECKSUM_UNNECESSARY;
- skb->ip_summed != CHECKSUM_NONE;
Else the entire check becomes a NOOP. These are the only three valid
states on tx. With very few codepaths generating CHECKSUM_UNNECESSARY
to begin with, it arguably already is practically a NOOP. I need to
look more closely what the statement is intended to protect against,
before we relax it even further.
The patch will need the same UFO change in ipv6_ufo_fragment.
I also had to verify that it is indeed correct to use
CHECKSUM_UNNECESSARY for this case. It is not commonly used as alias
for CHECKSUM_NONE on the tx path. And indeed does not carry the exact
same meaning. It was defined as "no need to checksum" on tx for
protocols that skb_checksum_help does not support:
http://patchwork.ozlabs.org/patch/146567/
Given that, it should be fine to use in this case.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] net: Reduce skb_warn_bad_offload() noise.
2017-08-08 6:29 ` Willem de Bruijn
@ 2017-08-08 17:46 ` Willem de Bruijn
2017-08-09 12:06 ` Tonghao Zhang
0 siblings, 1 reply; 9+ messages in thread
From: Willem de Bruijn @ 2017-08-08 17:46 UTC (permalink / raw)
To: Tonghao Zhang
Cc: Eric Dumazet, Linux Kernel Network Developers, Eric Dumazet,
Willem de Bruijn, Pravin B Shelar
>> @@ -2670,6 +2670,7 @@ static inline bool skb_needs_check(struct
>> sk_buff *skb, bool tx_path)
>> {
>> if (tx_path)
>> return skb->ip_summed != CHECKSUM_PARTIAL &&
>> + skb->ip_summed != CHECKSUM_UNNECESSARY &&
>> skb->ip_summed != CHECKSUM_NONE;
>
> Good catch. Only, the CHECKSUM_NONE case was added specifically to
> work around this UFO issue on the tx path in commit 6e7bc478c9a0
> ("net: skb_needs_check() accepts CHECKSUM_NONE for tx"). If we change
> the value generated by UFO, we can remove that statement, so
>
> + skb->ip_summed != CHECKSUM_UNNECESSARY;
> - skb->ip_summed != CHECKSUM_NONE;
>
> Else the entire check becomes a NOOP. These are the only three valid
> states on tx. With very few codepaths generating CHECKSUM_UNNECESSARY
> to begin with, it arguably already is practically a NOOP. I need to
> look more closely what the statement is intended to protect against,
> before we relax it even further.
On transmit, packets entering skb_gso_segment are expected to always
have ip_summed CHECKSUM_PARTIAL. This check was added to track down
unexpected exceptions in commit 67fd1a731ff1 ("net: Add debug info to
track down GSO checksum bug").
Only when called for the second time, after skb_mac_gso_segment, do we
have to possibly handle the case where the GSO layer computes the
checksum and changes ip_summed.
Since this only goes into 4.11 to 4.13, making two separate
skb_needs_check variants for these two call sites seems overkill. I
will send the simple fix to convert CHECKSUM_NONE to
CHECKSUM_UNNECESSARY.
As a side effect of removing UFO in 4.14-rc1, we can also revert
commit 6e7bc478c9a0 ("net: skb_needs_check() accepts CHECKSUM_NONE for
tx") in net-next.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] net: Reduce skb_warn_bad_offload() noise.
2017-08-08 17:46 ` Willem de Bruijn
@ 2017-08-09 12:06 ` Tonghao Zhang
0 siblings, 0 replies; 9+ messages in thread
From: Tonghao Zhang @ 2017-08-09 12:06 UTC (permalink / raw)
To: Willem de Bruijn
Cc: Eric Dumazet, Linux Kernel Network Developers, Eric Dumazet,
Willem de Bruijn, Pravin B Shelar
Thanks, I send a patch, which will revert commit 6e7bc478c9a0 in net-next.
On Wed, Aug 9, 2017 at 1:46 AM, Willem de Bruijn
<willemdebruijn.kernel@gmail.com> wrote:
>>> @@ -2670,6 +2670,7 @@ static inline bool skb_needs_check(struct
>>> sk_buff *skb, bool tx_path)
>>> {
>>> if (tx_path)
>>> return skb->ip_summed != CHECKSUM_PARTIAL &&
>>> + skb->ip_summed != CHECKSUM_UNNECESSARY &&
>>> skb->ip_summed != CHECKSUM_NONE;
>>
>> Good catch. Only, the CHECKSUM_NONE case was added specifically to
>> work around this UFO issue on the tx path in commit 6e7bc478c9a0
>> ("net: skb_needs_check() accepts CHECKSUM_NONE for tx"). If we change
>> the value generated by UFO, we can remove that statement, so
>>
>> + skb->ip_summed != CHECKSUM_UNNECESSARY;
>> - skb->ip_summed != CHECKSUM_NONE;
>>
>> Else the entire check becomes a NOOP. These are the only three valid
>> states on tx. With very few codepaths generating CHECKSUM_UNNECESSARY
>> to begin with, it arguably already is practically a NOOP. I need to
>> look more closely what the statement is intended to protect against,
>> before we relax it even further.
>
> On transmit, packets entering skb_gso_segment are expected to always
> have ip_summed CHECKSUM_PARTIAL. This check was added to track down
> unexpected exceptions in commit 67fd1a731ff1 ("net: Add debug info to
> track down GSO checksum bug").
>
> Only when called for the second time, after skb_mac_gso_segment, do we
> have to possibly handle the case where the GSO layer computes the
> checksum and changes ip_summed.
>
> Since this only goes into 4.11 to 4.13, making two separate
> skb_needs_check variants for these two call sites seems overkill. I
> will send the simple fix to convert CHECKSUM_NONE to
> CHECKSUM_UNNECESSARY.
>
> As a side effect of removing UFO in 4.14-rc1, we can also revert
> commit 6e7bc478c9a0 ("net: skb_needs_check() accepts CHECKSUM_NONE for
> tx") in net-next.
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2017-08-09 12:06 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-08-04 13:11 [PATCH] net: Reduce skb_warn_bad_offload() noise Tonghao Zhang
2017-08-04 13:29 ` Eric Dumazet
2017-08-07 0:37 ` Tonghao Zhang
2017-08-07 4:42 ` Willem de Bruijn
2017-08-07 23:44 ` Tonghao Zhang
2017-08-08 5:26 ` Tonghao Zhang
2017-08-08 6:29 ` Willem de Bruijn
2017-08-08 17:46 ` Willem de Bruijn
2017-08-09 12:06 ` Tonghao Zhang
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.