Re: [PATCH] net: Reduce skb_warn_bad_offload() noise.
Thanks, I send a patch, which will revert commit 6e7bc478c9a0 in net-next. On Wed, Aug 9, 2017 at 1:46 AM, Willem de Bruijnwrote: >>> @@ -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.
Re: [PATCH] net: Reduce skb_warn_bad_offload() noise.
>> @@ -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.
Re: [PATCH] net: Reduce skb_warn_bad_offload() noise.
> 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.
Re: [PATCH] net: Reduce skb_warn_bad_offload() noise.
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=(0x006000214ba9, 0x) 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: GW 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: 8be36f5e9680 task.stack: b6c840d44000 [ 1291.596283] RIP: 0010:skb_warn_bad_offload+0xd3/0xde [ 1291.596284] RSP: 0018:8be3796438c8 EFLAGS: 00010246 [ 1291.596285] RAX: 0074 RBX: 8be363ff9f00 RCX: 0006 [ 1291.596286] RDX: RSI: 0086 RDI: 8be37964e0a0 [ 1291.596287] RBP: 8be3796438f0 R08: R09: 0bf2 [ 1291.596287] R10: 0004 R11: 0bf1 R12: 8be3660f [ 1291.596288] R13: 0001 R14: R15: 8be3660f [ 1291.596289] FS: 7f8d93bdb740() GS:8be37964() knlGS: [ 1291.596290] CS: 0010 DS: ES: CR0: 80050033 [ 1291.596291] CR2: 555f94a6e0c8 CR3: 00012b84a000 CR4: 001406e0 [ 1291.596296] Call Trace: [ 1291.596297] [ 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]
Re: [PATCH] net: Reduce skb_warn_bad_offload() noise.
That is fine to me. I have tested it. Thanks. On Mon, Aug 7, 2017 at 12:42 PM, Willem de Bruijnwrote: >> 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;
Re: [PATCH] net: Reduce skb_warn_bad_offload() noise.
> 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;
Re: [PATCH] net: Reduce skb_warn_bad_offload() noise.
On Fri, Aug 4, 2017 at 9:29 PM, Eric Dumazetwrote: > 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=(0x04009fbb58e9, 0x) 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=(0x00a2803b7c69, 0x) 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 >> 8801c063ecd8 82346bdf 0001 1100380c7d2e >> ed00380c7d26 41b58ab3 84b37e38 823468f1 >> 84820740 84f289c0 dc00 8801c063ee20 >> 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. > >> [] __dump_stack lib/dump_stack.c:15 [inline] >> [] dump_stack+0x2ee/0x3ef lib/dump_stack.c:51 >> [] panic+0x1fb/0x412 kernel/panic.c:179 >> [] __warn+0x1c4/0x1e0 kernel/panic.c:542 >> [] warn_slowpath_fmt+0xc5/0x100 kernel/panic.c:565 >> [] skb_warn_bad_offload+0x2af/0x390 net/core/dev.c:2434 >> [] __skb_gso_segment+0x482/0x780 net/core/dev.c:2706 >> [] skb_gso_segment include/linux/netdevice.h:3985 [inline] >> [] validate_xmit_skb+0x5c9/0xc20 net/core/dev.c:2969 >> [] __dev_queue_xmit+0xe6b/0x1e70 net/core/dev.c:3383 >> [] dev_queue_xmit+0x17/0x20 net/core/dev.c:3424 >> >> Cc: Eric Dumazet >> Cc: Willem de Bruijn >> Cc: Pravin B Shelar >> Signed-off-by: Tonghao Zhang >> --- > > > IMO, this description is too confusing, I do not understand this patch. sorry
Re: [PATCH] net: Reduce skb_warn_bad_offload() noise.
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=(0x04009fbb58e9, 0x) 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=(0x00a2803b7c69, 0x) 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 > 8801c063ecd8 82346bdf 0001 1100380c7d2e > ed00380c7d26 41b58ab3 84b37e38 823468f1 > 84820740 84f289c0 dc00 8801c063ee20 > Call Trace: Why are you adding this trace that was part of the b2504a5dbef3 changelog ? > [] __dump_stack lib/dump_stack.c:15 [inline] > [] dump_stack+0x2ee/0x3ef lib/dump_stack.c:51 > [] panic+0x1fb/0x412 kernel/panic.c:179 > [] __warn+0x1c4/0x1e0 kernel/panic.c:542 > [] warn_slowpath_fmt+0xc5/0x100 kernel/panic.c:565 > [] skb_warn_bad_offload+0x2af/0x390 net/core/dev.c:2434 > [] __skb_gso_segment+0x482/0x780 net/core/dev.c:2706 > [] skb_gso_segment include/linux/netdevice.h:3985 [inline] > [] validate_xmit_skb+0x5c9/0xc20 net/core/dev.c:2969 > [] __dev_queue_xmit+0xe6b/0x1e70 net/core/dev.c:3383 > [] dev_queue_xmit+0x17/0x20 net/core/dev.c:3424 > > Cc: Eric Dumazet> Cc: Willem de Bruijn > Cc: Pravin B Shelar > Signed-off-by: Tonghao Zhang > --- IMO, this description is too confusing, I do not understand this patch.
[PATCH] net: Reduce skb_warn_bad_offload() noise.
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=(0x04009fbb58e9, 0x) 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=(0x00a2803b7c69, 0x) 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 8801c063ecd8 82346bdf 0001 1100380c7d2e ed00380c7d26 41b58ab3 84b37e38 823468f1 84820740 84f289c0 dc00 8801c063ee20 Call Trace: [] __dump_stack lib/dump_stack.c:15 [inline] [] dump_stack+0x2ee/0x3ef lib/dump_stack.c:51 [] panic+0x1fb/0x412 kernel/panic.c:179 [] __warn+0x1c4/0x1e0 kernel/panic.c:542 [] warn_slowpath_fmt+0xc5/0x100 kernel/panic.c:565 [] skb_warn_bad_offload+0x2af/0x390 net/core/dev.c:2434 [] __skb_gso_segment+0x482/0x780 net/core/dev.c:2706 [] skb_gso_segment include/linux/netdevice.h:3985 [inline] [] validate_xmit_skb+0x5c9/0xc20 net/core/dev.c:2969 [] __dev_queue_xmit+0xe6b/0x1e70 net/core/dev.c:3383 [] dev_queue_xmit+0x17/0x20 net/core/dev.c:3424 Cc: Eric DumazetCc: Willem de Bruijn Cc: Pravin B Shelar Signed-off-by: Tonghao Zhang --- 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