[regression] 6ae459bd skbuff: Fix skb checksum flag on skb pull

[regression] 6ae459bd skbuff: Fix skb checksum flag on skb pull

[Mike Galbraith]

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 <pshelar@nicira.com>
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 <achanda@vmware.com>
    Signed-off-by: Pravin B Shelar <pshelar@nicira.com>
    Acked-by: Tom Herbert <tom@herbertland.com>
    Signed-off-by: David S. Miller <davem@davemloft.net>

: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

Re: [regression] 6ae459bd skbuff: Fix skb checksum flag on skb pull

[Herbert Xu]

Mike Galbraith <umgwanakikbuti@gmail.com> wrote:
> 
> 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.

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,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [regression] 6ae459bd skbuff: Fix skb checksum flag on skb pull

[Yanko Kaneti]

On Thu-10/01/15-2015 09:55, Mike Galbraith wrote:
..... 
> 
> Bisected and post bisect verified by applying/removing a revert.
> 
> Log:
> 
> 6ae459bdaaeebc632b16e54dcbabb490c6931d61 is the first bad commit
> commit 6ae459bdaaeebc632b16e54dcbabb490c6931d61
> Author: Pravin B Shelar <pshelar@nicira.com>
> 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

Re: [regression] 6ae459bd skbuff: Fix skb checksum flag on skb pull

[Mike Galbraith]

On Thu, 2015-10-01 at 16:42 +0800, Herbert Xu wrote:
> Mike Galbraith <umgwanakikbuti@gmail.com> wrote:
> > 
> > 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.
> 
> So do you use VXLANs?

Nope.  It really really is that patch though.

> 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(-)

--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -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

Re: [regression] 6ae459bd skbuff: Fix skb checksum flag on skb pull

[Herbert Xu]

On Thu, Oct 01, 2015 at 01:16:17PM +0200, Mike Galbraith wrote:
> 
> > 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,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

skbuff: Fix checksum start check in skb_postpull_rcsum

[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 <umgwanakikbuti@gmail.com>
Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index 2b0a30a..4398411 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -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;
 }
 
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: skbuff: Fix checksum start check in skb_postpull_rcsum

[Mike Galbraith]

Yup, all better.  Thanks.

	-Mike

On Thu, 2015-10-01 at 19:34 +0800, Herbert Xu wrote:
> 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 <umgwanakikbuti@gmail.com>
> Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
> 
> diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
> index 2b0a30a..4398411 100644
> --- a/include/linux/skbuff.h
> +++ b/include/linux/skbuff.h
> @@ -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;
>  }
>  

Re: skbuff: Fix checksum start check in skb_postpull_rcsum

[David Miller]

From: Herbert Xu <herbert@gondor.apana.org.au>
Date: Thu, 1 Oct 2015 19:34:07 +0800

> 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 <umgwanakikbuti@gmail.com>
> Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
> 
> diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
> index 2b0a30a..4398411 100644
> --- a/include/linux/skbuff.h
> +++ b/include/linux/skbuff.h
> @@ -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 <pshelar@nicira.com>
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 <avagin@odin.com>
    Signed-off-by: Pravin B Shelar <pshelar@nicira.com>
    Signed-off-by: David S. Miller <davem@davemloft.net>

diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index 2b0a30a..4398411 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -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;
 }
 
diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index dad4dd3..fab4599 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -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);
 

Re: skbuff: Fix checksum start check in skb_postpull_rcsum

[Herbert Xu]

On Thu, Oct 01, 2015 at 08:15:43AM -0700, David Miller wrote:
> 
> A patch with this and more should be in the 'net' tree already.

Yes this looks much better.  Thanks!
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt