Skip to contentSkip to conversation

6ae459bd skbuff: Fix skb checksum flag on skb pull

regression Mike Galbraith 4 participants 9 messages Oct 1, 2015

Subsystems (2)
Networking Network drivers
lore
Share & tools
lore

Mike Galbraith

author
#0 lore raw

Greetings network wizards,

$subject broke git-daemon here.

homer:/usr/local/src/kernel/linux-3.x.git # time strace -vvvfFtT git remote update 2> /strace.out Fetching origin

real    2m9.164s
user    0m1.616s
sys     0m0.316s

2 minutes of thumb twiddling should have been..

homer:/usr/local/src/kernel/linux-3.x.git # time git remote update Fetching origin

real    0m0.213s
user    0m0.156s
sys     0m0.036s

Daemon runs as user git like so, with all repositories living in ~git:
/usr/lib/git/git-daemon --syslog --detach --reuseaddr --user=git --group=daemon --pid-file=/var/run/git-daemon.pid --export-all --user-path

Bisected and post bisect verified by applying/removing a revert.

Log:

6ae459bdaaeebc632b16e54dcbabb490c6931d61 is the first bad commit commit 6ae459bdaaeebc632b16e54dcbabb490c6931d61

Author: Pravin B Shelar <[email protected]>
Date:   Tue Sep 22 12:57:53 2015 -0700
    skbuff: Fix skb checksum flag on skb pull
    
    VXLAN device can receive skb with checksum partial. But the checksum
    offset could be in outer header which is pulled on receive. This results
    in negative checksum offset for the skb. Such skb can cause the assert
    failure in skb_checksum_help(). Following patch fixes the bug by setting
    checksum-none while pulling outer header.
    
    Following is the kernel panic msg from old kernel hitting the bug.
    
    ------------[ cut here ]------------
    kernel BUG at net/core/dev.c:1906!
    RIP: 0010:[<ffffffff81518034>] skb_checksum_help+0x144/0x150
    Call Trace:
    <IRQ>
    [<ffffffffa0164c28>] queue_userspace_packet+0x408/0x470 [openvswitch]
    [<ffffffffa016614d>] ovs_dp_upcall+0x5d/0x60 [openvswitch]
    [<ffffffffa0166236>] ovs_dp_process_packet_with_key+0xe6/0x100 [openvswitch]
    [<ffffffffa016629b>] ovs_dp_process_received_packet+0x4b/0x80 [openvswitch]
    [<ffffffffa016c51a>] ovs_vport_receive+0x2a/0x30 [openvswitch]
    [<ffffffffa0171383>] vxlan_rcv+0x53/0x60 [openvswitch]
    [<ffffffffa01734cb>] vxlan_udp_encap_recv+0x8b/0xf0 [openvswitch]
    [<ffffffff8157addc>] udp_queue_rcv_skb+0x2dc/0x3b0
    [<ffffffff8157b56f>] __udp4_lib_rcv+0x1cf/0x6c0
    [<ffffffff8157ba7a>] udp_rcv+0x1a/0x20
    [<ffffffff8154fdbd>] ip_local_deliver_finish+0xdd/0x280
    [<ffffffff81550128>] ip_local_deliver+0x88/0x90
    [<ffffffff8154fa7d>] ip_rcv_finish+0x10d/0x370
    [<ffffffff81550365>] ip_rcv+0x235/0x300
    [<ffffffff8151ba1d>] __netif_receive_skb+0x55d/0x620
    [<ffffffff8151c360>] netif_receive_skb+0x80/0x90
    [<ffffffff81459935>] virtnet_poll+0x555/0x6f0
    [<ffffffff8151cd04>] net_rx_action+0x134/0x290
    [<ffffffff810683d8>] __do_softirq+0xa8/0x210
    [<ffffffff8162fe6c>] call_softirq+0x1c/0x30
    [<ffffffff810161a5>] do_softirq+0x65/0xa0
    [<ffffffff810687be>] irq_exit+0x8e/0xb0
    [<ffffffff81630733>] do_IRQ+0x63/0xe0
    [<ffffffff81625f2e>] common_interrupt+0x6e/0x6e
    
    Reported-by: Anupam Chanda <[email protected]>
    Signed-off-by: Pravin B Shelar <[email protected]>
    Acked-by: Tom Herbert <[email protected]>
    Signed-off-by: David S. Miller <[email protected]>

:040000 040000 3bb5c605650664362514aa1834029f478478057c f016d3e96119e2d67e582da593ca2fbfac6347d2 M include

git bisect start
# good: [1f93e4a96c9109378204c147b3eec0d0e8100fde] Linux 4.3-rc2
git bisect good 1f93e4a96c9109378204c147b3eec0d0e8100fde
# bad: [dd36d7393d6310b0c1adefb22fba79c3cf8a577c] Merge git://www.linux-watchdog.org/linux-watchdog
git bisect bad dd36d7393d6310b0c1adefb22fba79c3cf8a577c
# good: [d4a748a10e50d95992ae67677f1a1a13e2d6ed47] Merge branch 'for-4.3-fixes' of git://git.kernel.org/pub/scm/linux/kernel/git/tj/cgroup
git bisect good d4a748a10e50d95992ae67677f1a1a13e2d6ed47
# bad: [fb740f9bab3b3c049d6207b148e45de05163b6e5] Merge tag 'tty-4.3-rc3' of git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/tty
git bisect bad fb740f9bab3b3c049d6207b148e45de05163b6e5
# good: [23eedbc2435ddd226717603c4f3c8efec7bdbb4d] ch9200: Convert to use module_usb_driver
git bisect good 23eedbc2435ddd226717603c4f3c8efec7bdbb4d
# bad: [7322967bc1bd97ac9c49ecea19e5a1f681ca27ee] phy: add proper phy struct device refcounting
git bisect bad 7322967bc1bd97ac9c49ecea19e5a1f681ca27ee
# good: [41b976414c88016e2c9d9b2f6667ee67a998d388] 8139cp: Dump contents of descriptor ring on TX timeout
git bisect good 41b976414c88016e2c9d9b2f6667ee67a998d388
# bad: [d682d2bdc30650a5c7ce9908ab83ab674b658744] bnx2x: byte swap rss_key to comply to Toeplitz specs
git bisect bad d682d2bdc30650a5c7ce9908ab83ab674b658744
# bad: [d5b8d6404395641987db76e28334cae4cef771ae] net: gianfar: remove misuse of IRQF_NO_SUSPEND flag
git bisect bad d5b8d6404395641987db76e28334cae4cef771ae
# good: [da314c9923fed553a007785a901fd395b7eb6c19] netlink: Replace rhash_portid with bound
git bisect good da314c9923fed553a007785a901fd395b7eb6c19
# bad: [6ae459bdaaeebc632b16e54dcbabb490c6931d61] skbuff: Fix skb checksum flag on skb pull
git bisect bad 6ae459bdaaeebc632b16e54dcbabb490c6931d61

# first bad commit: [6ae459bdaaeebc632b16e54dcbabb490c6931d61] skbuff: Fix skb checksum flag on skb pull
#1 lore raw
Quoted 13 lines · Mike Galbraith

homer:/usr/local/src/kernel/linux-3.x.git # time strace -vvvfFtT git remote update 2> /strace.out Fetching origin

real    2m9.164s
user    0m1.616s
sys     0m0.316s

2 minutes of thumb twiddling should have been..

homer:/usr/local/src/kernel/linux-3.x.git # time git remote update Fetching origin

Quoted 8 lines
real    0m0.213s
user    0m0.156s
sys     0m0.036s

Daemon runs as user git like so, with all repositories living in ~git:
/usr/lib/git/git-daemon --syslog --detach --reuseaddr --user=git --group=daemon --pid-file=/var/run/git-daemon.pid --export-all --user-path

Bisected and post bisect verified by applying/removing a revert.

So do you use VXLANs?

If this patch is indeed causing the regression you should be able to spot some differences in the tcpdump. Can you confirm this?

Thanks,

Signature · 3 lines
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
author
#2 lore raw
Quoted 18 lines · Herbert Xu
Quoted 17 lines · Mike Galbraith

homer:/usr/local/src/kernel/linux-3.x.git # time strace -vvvfFtT git remote update 2> /strace.out Fetching origin

real    2m9.164s
user    0m1.616s
sys     0m0.316s

2 minutes of thumb twiddling should have been..

homer:/usr/local/src/kernel/linux-3.x.git # time git remote update Fetching origin

real    0m0.213s
user    0m0.156s
sys     0m0.036s
Quoted 6 lines
Quoted 4 lines

Daemon runs as user git like so, with all repositories living in ~git:
/usr/lib/git/git-daemon --syslog --detach --reuseaddr --user=git --group=daemon --pid-file=/var/run/git-daemon.pid --export-all --user-path

Bisected and post bisect verified by applying/removing a revert.

So do you use VXLANs?

Nope. It really really is that patch though.

Quoted 2 lines

If this patch is indeed causing the regression you should be able to spot some differences in the tcpdump. Can you confirm this?

Will this work instead?

---
 include/linux/skbuff.h |    4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)
include/linux/skbuff.h +3 -1
@@ -2708,8 +2708,10 @@ static inline void skb_postpull_rcsum(st
 	if (skb->ip_summed == CHECKSUM_COMPLETE)
 		skb->csum = csum_sub(skb->csum, csum_partial(start, len, 0));
 	else if (skb->ip_summed == CHECKSUM_PARTIAL &&
-		 skb_checksum_start_offset(skb) <= len)
+		 skb_checksum_start_offset(skb) <= len) {
 		skb->ip_summed = CHECKSUM_NONE;
+		trace_printk("skb_checksum_start_offset(skb) = %d len = %u\n", skb_checksum_start_offset(skb), len);
+	}
 }
 
 unsigned char *skb_pull_rcsum(struct sk_buff *skb, unsigned int len);

# tracer: nop
#

# nop latency trace v1.1.5 on 4.3.0-master
# --------------------------------------------------------------------
# latency: 0 us, #10/10, CPU#0 | (M:desktop VP:0, KP:0, SP:0 HP:0 #P:8)
#    -----------------
#    | task: -0 (uid:0 nice:0 policy:0 rt_prio:0)
#    -----------------
#
#                  _------=> CPU#            
#                 / _-----=> irqs-off        
#                | / _----=> need-resched    
#                || / _---=> hardirq/softirq 
#                ||| / _--=> preempt-depth   
#                |||| /     delay            
#  cmd     pid   ||||| time  |   caller      
#     \   /      |||||  \    |   /         
     git-10826   2..s1 10668371us+: ip6_input_finish+0x1e8/0x3e0: skb_checksum_start_offset(skb) = 0 len = 40
     git-10826   2..s1 10668386us@: <stack trace>
 => ip6_rcv_finish+0x34/0xa0
 => ipv6_rcv+0x2b7/0x4c0
 => __netif_receive_skb_core+0x634/0x990
 => __netif_receive_skb+0x18/0x60
 => process_backlog+0x8d/0x120
 => net_rx_action+0x13a/0x300
 => __do_softirq+0xcc/0x240
 => do_softirq_own_stack+0x1c/0x30
 => do_softirq+0x31/0x40
 => __local_bh_enable_ip+0x78/0x80
 => ip6_finish_output2+0x171/0x460
 => ip6_finish_output+0x89/0xe0
 => ip6_output+0x44/0xd0
 => ip6_xmit+0x208/0x500
 => inet6_csk_xmit+0x65/0xa0
 => tcp_transmit_skb+0x494/0x860
 => tcp_connect+0x63e/0x7f0
 => tcp_v6_connect+0x2f3/0x570
 => __inet_stream_connect+0x93/0x2e0
 => inet_stream_connect+0x38/0x50
 => SYSC_connect+0x69/0xd0
 => SyS_connect+0xe/0x10
 => entry_SYSCALL_64_fastpath+0x16/0x6e
  <idle>-0       2..s. 11666764us : ip6_input_finish+0x1e8/0x3e0: skb_checksum_start_offset(skb) = 0 len = 40
  <idle>-0       2..s. 11666768us$: <stack trace>
 => ip6_rcv_finish+0x34/0xa0
 => ipv6_rcv+0x2b7/0x4c0
 => __netif_receive_skb_core+0x634/0x990
 => __netif_receive_skb+0x18/0x60
 => process_backlog+0x8d/0x120
 => net_rx_action+0x13a/0x300
 => __do_softirq+0xcc/0x240
 => irq_exit+0x8c/0xa0
 => smp_apic_timer_interrupt+0x45/0x60
 => apic_timer_interrupt+0x7f/0x90
 => start_secondary+0x103/0x130
  <idle>-0       2..s. 13670053us : ip6_input_finish+0x1e8/0x3e0: skb_checksum_start_offset(skb) = 0 len = 40
  <idle>-0       2..s. 13670058us$: <stack trace>
 => ip6_rcv_finish+0x34/0xa0
 => ipv6_rcv+0x2b7/0x4c0
 => __netif_receive_skb_core+0x634/0x990
 => __netif_receive_skb+0x18/0x60
 => process_backlog+0x8d/0x120
 => net_rx_action+0x13a/0x300
 => __do_softirq+0xcc/0x240
 => irq_exit+0x8c/0xa0
 => smp_apic_timer_interrupt+0x45/0x60
 => apic_timer_interrupt+0x7f/0x90
 => cpuidle_enter+0x17/0x20
 => call_cpuidle+0x32/0x60
 => cpu_startup_entry+0x217/0x2d0
 => start_secondary+0x103/0x130
  <idle>-0       2..s. 17676620us : ip6_input_finish+0x1e8/0x3e0: skb_checksum_start_offset(skb) = 0 len = 40
  <idle>-0       2..s. 17676625us$: <stack trace>
 => ip6_rcv_finish+0x34/0xa0
 => ipv6_rcv+0x2b7/0x4c0
 => __netif_receive_skb_core+0x634/0x990
 => __netif_receive_skb+0x18/0x60
 => process_backlog+0x8d/0x120
 => net_rx_action+0x13a/0x300
 => __do_softirq+0xcc/0x240
 => irq_exit+0x8c/0xa0
 => smp_apic_timer_interrupt+0x45/0x60
 => apic_timer_interrupt+0x7f/0x90
 => cpuidle_enter+0x17/0x20
 => call_cpuidle+0x32/0x60
 => cpu_startup_entry+0x217/0x2d0
 => start_secondary+0x103/0x130
  <idle>-0       2..s. 25697779us : ip6_input_finish+0x1e8/0x3e0: skb_checksum_start_offset(skb) = 0 len = 40
  <idle>-0       2..s. 25697783us : <stack trace>
 => ip6_rcv_finish+0x34/0xa0
 => ipv6_rcv+0x2b7/0x4c0
 => __netif_receive_skb_core+0x634/0x990
 => __netif_receive_skb+0x18/0x60
 => process_backlog+0x8d/0x120
 => net_rx_action+0x13a/0x300
 => __do_softirq+0xcc/0x240
 => irq_exit+0x8c/0xa0
 => smp_apic_timer_interrupt+0x45/0x60
 => apic_timer_interrupt+0x7f/0x90
 => cpuidle_enter+0x17/0x20
 => call_cpuidle+0x32/0x60
 => cpu_startup_entry+0x217/0x2d0
 => start_secondary+0x103/0x130
#3 lore raw
Quoted 5 lines · Mike Galbraith
Quoted 3 lines

If this patch is indeed causing the regression you should be able to spot some differences in the tcpdump. Can you confirm this?

Will this work instead?

Thanks! Looks like the patch wasn't thought through. It's testing the offset after the pull but treating it as the offset before the pull.

I'll send you a patch in a couple of minutes.

Cheers,

Signature · 3 lines
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
#4 lore raw

skbuff: Fix checksum start check in skb_postpull_rcsum

In skb_postpull_rcsum the skb has already been pulled so we should be testing whether the checksum start offset is non-negative rather than how it compares to the length that we have pulled.

Fixes 6ae459bdaaee ("skbuff: Fix skb checksum flag on skb pull")
Reported-by Mike Galbraith <[email protected]>
Signed-off-by Herbert Xu <[email protected]>
include/linux/skbuff.h +1 -1
index 2b0a30a..4398411 100644
@@ -2708,7 +2708,7 @@ static inline void skb_postpull_rcsum(struct sk_buff *skb,
 	if (skb->ip_summed == CHECKSUM_COMPLETE)
 		skb->csum = csum_sub(skb->csum, csum_partial(start, len, 0));
 	else if (skb->ip_summed == CHECKSUM_PARTIAL &&
-		 skb_checksum_start_offset(skb) <= len)
+		 skb_checksum_start_offset(skb) < 0)
 		skb->ip_summed = CHECKSUM_NONE;
 }
Signature · 3 lines
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
author
#5 lore raw

Re: skbuff: Fix checksum start check in skb_postpull_rcsum

Yup, all better. Thanks.

-Mike

Quoted 21 lines · Herbert Xu

In skb_postpull_rcsum the skb has already been pulled so we should be testing whether the checksum start offset is non-negative rather than how it compares to the length that we have pulled.

Fixes 6ae459bdaaee ("skbuff: Fix skb checksum flag on skb pull")
Reported-by Mike Galbraith <[email protected]>
Signed-off-by Herbert Xu <[email protected]>
include/linux/skbuff.h +1 -1
index 2b0a30a..4398411 100644
@@ -2708,7 +2708,7 @@ static inline void skb_postpull_rcsum(struct sk_buff *skb,
 	if (skb->ip_summed == CHECKSUM_COMPLETE)
 		skb->csum = csum_sub(skb->csum, csum_partial(start, len, 0));
 	else if (skb->ip_summed == CHECKSUM_PARTIAL &&
-		 skb_checksum_start_offset(skb) <= len)
+		 skb_checksum_start_offset(skb) < 0)
 		skb->ip_summed = CHECKSUM_NONE;
 }
maintainer
#6 lore raw

Re: skbuff: Fix checksum start check in skb_postpull_rcsum

In reply to Herbert Xu

From: Herbert Xu <[email protected]>
Date: Thu, 1 Oct 2015 19:34:07 +0800

Quoted 13 lines

In skb_postpull_rcsum the skb has already been pulled so we should be testing whether the checksum start offset is non-negative rather than how it compares to the length that we have pulled.

Fixes 6ae459bdaaee ("skbuff: Fix skb checksum flag on skb pull")
Reported-by Mike Galbraith <[email protected]>
Signed-off-by Herbert Xu <[email protected]>
include/linux/skbuff.h +0 -0
index 2b0a30a..4398411 100644

include/linux/skbuff.h static inline void skb_postpull_rcsum(struct sk_buff *skb, · line 2714

Quoted 8 lines
include/linux/skbuff.h +1 -1
@@ -2708,7 +2708,7 @@ static inline void skb_postpull_rcsum(struct sk_buff *skb,
 	if (skb->ip_summed == CHECKSUM_COMPLETE)
 		skb->csum = csum_sub(skb->csum, csum_partial(start, len, 0));
 	else if (skb->ip_summed == CHECKSUM_PARTIAL &&
-		 skb_checksum_start_offset(skb) <= len)
+		 skb_checksum_start_offset(skb) < 0)
 		skb->ip_summed = CHECKSUM_NONE;
 }

A patch with this and more should be in the 'net' tree already.

====================
commit 31b33dfb0a144469dd805514c9e63f4993729a48

Author: Pravin B Shelar <[email protected]>
Date:   Mon Sep 28 17:24:25 2015 -0700

skbuff: Fix skb checksum partial check.

Earlier patch 6ae459bda tried to detect void ckecksum partial skb by comparing pull length to checksum offset. But it does not work for all cases since checksum-offset depends on updates to skb->data.

Following patch fixes it by validating checksum start offset after skb-data pointer is updated. Negative value of checksum offset start means there is no need to checksum.

Fixes 6ae459bda ("skbuff: Fix skb checksum flag on skb pull")
Reported-by Andrew Vagin <[email protected]>
Signed-off-by Pravin B Shelar <[email protected]>
Signed-off-by David S. Miller <[email protected]>
include/linux/skbuff.h +1 -1
index 2b0a30a..4398411 100644
@@ -2708,7 +2708,7 @@ static inline void skb_postpull_rcsum(struct sk_buff *skb,
 	if (skb->ip_summed == CHECKSUM_COMPLETE)
 		skb->csum = csum_sub(skb->csum, csum_partial(start, len, 0));
 	else if (skb->ip_summed == CHECKSUM_PARTIAL &&
-		 skb_checksum_start_offset(skb) <= len)
+		 skb_checksum_start_offset(skb) < 0)
 		skb->ip_summed = CHECKSUM_NONE;
 }
net/core/skbuff.c +5 -4
index dad4dd3..fab4599 100644
@@ -2958,11 +2958,12 @@ EXPORT_SYMBOL_GPL(skb_append_pagefrags);
  */
 unsigned char *skb_pull_rcsum(struct sk_buff *skb, unsigned int len)
 {
+	unsigned char *data = skb->data;
+
 	BUG_ON(len > skb->len);
-	skb->len -= len;
-	BUG_ON(skb->len < skb->data_len);
-	skb_postpull_rcsum(skb, skb->data, len);
-	return skb->data += len;
+	__skb_pull(skb, len);
+	skb_postpull_rcsum(skb, data, len);
+	return skb->data;
 }
 EXPORT_SYMBOL_GPL(skb_pull_rcsum);
#7 lore raw

Re: skbuff: Fix checksum start check in skb_postpull_rcsum

Quoted 2 lines · David Miller

A patch with this and more should be in the 'net' tree already.

Yes this looks much better. Thanks!

Signature · 3 lines
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
#8 lore raw
In reply to Mike Galbraith

On Thu-10/01/15-2015 09:55, Mike Galbraith wrote:
.....

Quoted 11 lines

Bisected and post bisect verified by applying/removing a revert.

Log:

6ae459bdaaeebc632b16e54dcbabb490c6931d61 is the first bad commit commit 6ae459bdaaeebc632b16e54dcbabb490c6931d61

Author: Pravin B Shelar <[email protected]>
Date:   Tue Sep 22 12:57:53 2015 -0700

skbuff: Fix skb checksum flag on skb pull

....

I've also chased a regression in Rawhide down to this commit. What happended here after and update from rc2 and rc3 was that all connections to ::1 failed with timeout, while v4 connections to 127.0.0.1 worked as expected.

Bisected on mainline and verified a revert of this commit fixes it.

-
Yanko