[PATCH net v2] skbuff: Fix skb checksum flag on skb pull

[PATCH net v2] skbuff: Fix skb checksum flag on skb pull

[Pravin B Shelar]

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>
---
v1-v2:
Set skb to CHECKSUM_NONE rather than CHECKSUM_UNNECESSARY.
---
 include/linux/skbuff.h |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index 9987af0..2b0a30a 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -2707,6 +2707,9 @@ 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->ip_summed = CHECKSUM_NONE;
 }
 
 unsigned char *skb_pull_rcsum(struct sk_buff *skb, unsigned int len);
-- 
1.7.1

Re: [PATCH net v2] skbuff: Fix skb checksum flag on skb pull

[Tom Herbert]

On Tue, Sep 22, 2015 at 12:57 PM, Pravin B Shelar <pshelar@nicira.com> wrote:
> 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>
> ---
> v1-v2:
> Set skb to CHECKSUM_NONE rather than CHECKSUM_UNNECESSARY.
> ---
>  include/linux/skbuff.h |    3 +++
>  1 files changed, 3 insertions(+), 0 deletions(-)
>
> diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
> index 9987af0..2b0a30a 100644
> --- a/include/linux/skbuff.h
> +++ b/include/linux/skbuff.h
> @@ -2707,6 +2707,9 @@ 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->ip_summed = CHECKSUM_NONE;
>  }
>
Acked-by: Tom Herbert <tom@herbertland.com>

>  unsigned char *skb_pull_rcsum(struct sk_buff *skb, unsigned int len);
> --
> 1.7.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH net v2] skbuff: Fix skb checksum flag on skb pull

[David Miller]

From: Pravin B Shelar <pshelar@nicira.com>
Date: Tue, 22 Sep 2015 12:57:53 -0700

> 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.
 ...
> Reported-by: Anupam Chanda <achanda@vmware.com>
> Signed-off-by: Pravin B Shelar <pshelar@nicira.com>

Applied, thanks.

Re: [PATCH net v2] skbuff: Fix skb checksum flag on skb pull

[Pravin Shelar]

On Thu, Sep 24, 2015 at 2:09 PM, David Miller <davem@davemloft.net> wrote:
> From: Pravin B Shelar <pshelar@nicira.com>
> Date: Tue, 22 Sep 2015 12:57:53 -0700
>
>> 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.
>  ...
>> Reported-by: Anupam Chanda <achanda@vmware.com>
>> Signed-off-by: Pravin B Shelar <pshelar@nicira.com>
>
> Applied, thanks.

Thanks for applying it. Since I have seen this bug on older kernel can
you also queue it for -stable.

Re: [PATCH net v2] skbuff: Fix skb checksum flag on skb pull

[David Miller]

From: Pravin Shelar <pshelar@nicira.com>
Date: Fri, 25 Sep 2015 19:48:57 -0700

> On Thu, Sep 24, 2015 at 2:09 PM, David Miller <davem@davemloft.net> wrote:
>> From: Pravin B Shelar <pshelar@nicira.com>
>> Date: Tue, 22 Sep 2015 12:57:53 -0700
>>
>>> 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.
>>  ...
>>> Reported-by: Anupam Chanda <achanda@vmware.com>
>>> Signed-off-by: Pravin B Shelar <pshelar@nicira.com>
>>
>> Applied, thanks.
> 
> Thanks for applying it. Since I have seen this bug on older kernel can
> you also queue it for -stable.

Not until we resolve all of the regressions caused by it.

Re: [PATCH net v2] skbuff: Fix skb checksum flag on skb pull

[Eric Dumazet]

On Thu, 2015-09-24 at 14:09 -0700, David Miller wrote:
> From: Pravin B Shelar <pshelar@nicira.com>
> Date: Tue, 22 Sep 2015 12:57:53 -0700
> 
> > 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.
>  ...
> > Reported-by: Anupam Chanda <achanda@vmware.com>
> > Signed-off-by: Pravin B Shelar <pshelar@nicira.com>
> 
> Applied, thanks.


It looks like we also should clear skb->csum ?

__skb_checksum_complete() definitely would be confused by garbage in
skb->csum

diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index 6b6bd42d6134..43e6f6163e07 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -2799,8 +2799,10 @@ 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) < 0)
+		 skb_checksum_start_offset(skb) < 0) {
 		skb->ip_summed = CHECKSUM_NONE;
+		skb->csum = 0;
+	}
 }
 
 unsigned char *skb_pull_rcsum(struct sk_buff *skb, unsigned int len);

Re: [PATCH net v2] skbuff: Fix skb checksum flag on skb pull

[David Miller]

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Wed, 30 Dec 2015 13:05:45 -0500

> On Thu, 2015-09-24 at 14:09 -0700, David Miller wrote:
>> From: Pravin B Shelar <pshelar@nicira.com>
>> Date: Tue, 22 Sep 2015 12:57:53 -0700
>> 
>> > 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.
>>  ...
>> > Reported-by: Anupam Chanda <achanda@vmware.com>
>> > Signed-off-by: Pravin B Shelar <pshelar@nicira.com>
>> 
>> Applied, thanks.
> 
> 
> It looks like we also should clear skb->csum ?
> 
> __skb_checksum_complete() definitely would be confused by garbage in
> skb->csum

Pravin, please take a look at this.

Re: [PATCH net v2] skbuff: Fix skb checksum flag on skb pull

[Eric Dumazet]

On Mon, 2016-01-04 at 23:08 -0500, David Miller wrote:
> From: Eric Dumazet <eric.dumazet@gmail.com>
> Date: Wed, 30 Dec 2015 13:05:45 -0500
> 
> > On Thu, 2015-09-24 at 14:09 -0700, David Miller wrote:
> >> From: Pravin B Shelar <pshelar@nicira.com>
> >> Date: Tue, 22 Sep 2015 12:57:53 -0700
> >> 
> >> > 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.
> >>  ...
> >> > Reported-by: Anupam Chanda <achanda@vmware.com>
> >> > Signed-off-by: Pravin B Shelar <pshelar@nicira.com>
> >> 
> >> Applied, thanks.
> > 
> > 
> > It looks like we also should clear skb->csum ?
> > 
> > __skb_checksum_complete() definitely would be confused by garbage in
> > skb->csum
> 
> Pravin, please take a look at this.

This might be ok :

Apparently users are supposed to init skb->csum in this case, as in 

udp4_csum_init()
-> skb_checksum_init_zero_check()
  -> __skb_checksum_validate()

-> __skb_checksum_validate_complete
   skb->csum = psum;