netdev - Re: [Patch net] mlx4: set csum_complete_sw bit when fixing complete csum

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CALx6S37LNGZEc+zPOu4fnYEYZ36dMKiimsz2XhgFs_rMOyFigg@mail.gmail.com>
Date:	Mon, 27 Jun 2016 14:47:02 -0700
From:	Tom Herbert <tom@...bertland.com>
To:	Cong Wang <xiyou.wangcong@...il.com>
Cc:	Or Gerlitz <gerlitz.or@...il.com>,
	Linux Netdev List <netdev@...r.kernel.org>,
	Shani Michaeli <shanim@...lanox.com>,
	Tariq Toukan <tariqt@...lanox.com>,
	Yishai Hadas <yishaih@...lanox.com>,
	Carol L Soto <clsoto@...ibm.com>
Subject: Re: [Patch net] mlx4: set csum_complete_sw bit when fixing complete csum

On Mon, Jun 27, 2016 at 2:44 PM, Cong Wang <xiyou.wangcong@...il.com> wrote:
> On Mon, Jun 27, 2016 at 2:08 PM, Or Gerlitz <gerlitz.or@...il.com> wrote:
>> On Mon, Jun 27, 2016 at 9:22 PM, Cong Wang <xiyou.wangcong@...il.com> wrote:
>>> The stack doesn't trust the complete csum by hardware
>>> even when it is correct.
>>
>> Can you explain that a little further?
>
> Sure, here is the code in __skb_checksum_complete():
>
>         /* skb->csum holds pseudo checksum */
>         sum = csum_fold(csum_add(skb->csum, csum));
>         if (likely(!sum)) {
>                 if (unlikely(skb->ip_summed == CHECKSUM_COMPLETE) &&
>                     !skb->csum_complete_sw)
>                         netdev_rx_csum_fault(skb->dev);
>         }
>
> So when sum == 0, it means the checksum is correct. And
> we already set ->ip_summed to CHECKSUM_COMPLETE
> after check_csum(), and ->csum_complete_sw is initialized
> to 0 when we allocate the skb. This is why we trigger
> netdev_rx_csum_fault().
>
Yes, but this also means that the driver gave the stack a checksum
complete value that was incorrect. That's an error.

Tom

>
>>
>>> In the case we fix csum by ourself
>>> probably it is safe to just mark it as completed by software.
>>
>>> This should shut up a kernel warning from netdev_rx_csum_fault()
>>> with mlx4 driver for ICMP packets.
>>
>> can you point/paste the exact warning and how to reproduce that? is
>> that as simple as running ping and/or ping6?
>
> Yes, ping is enough to reproduce it every time.
>
> The warning is below:
>
> [ 8693.680997] eth0: hw csum failure
> [ 8693.681003] CPU: 5 PID: 34 Comm: ksoftirqd/5 Not tainted 4.1.20-t6.el5 #1
> [ 8693.681005] Hardware name: SYNNEX HYVE-ZEUS/X9DRD-iF, BIOS 3.0.4 12/06/2013
> [ 8693.681008]  0000000000000000 ffff88085c15fae8 ffffffff81502872
> ffff881051397800
> [ 8693.681011]  ffff881054c08b01 ffff88085c15fb08 ffffffff814569c5
> 0000000000000000
> [ 8693.681014]  ffff8808572d7200 ffff88085c15fb38 ffffffff81450738
> ffff8808572d7200
> [ 8693.681017] Call Trace:
> [ 8693.681025]  [<ffffffff81502872>] dump_stack+0x4d/0x63
> [ 8693.681030]  [<ffffffff814569c5>] netdev_rx_csum_fault+0x38/0x3c
> [ 8693.681033]  [<ffffffff81450738>] __skb_checksum_complete+0x6e/0xb6
> [ 8693.681036]  [<ffffffff814b65d8>] icmp_rcv+0x17a/0x32f
> [ 8693.681040]  [<ffffffff8148b7f0>] ip_local_deliver_finish+0xd1/0x153
> [ 8693.681042]  [<ffffffff8148b9f7>] ip_local_deliver+0x8d/0x94
> [ 8693.681045]  [<ffffffff8148b71f>] ? xfrm4_policy_check.constprop.6+0x55/0x55
> [ 8693.681048]  [<ffffffff8148b651>] ip_rcv_finish+0x289/0x2cc
> [ 8693.681050]  [<ffffffff8148bc7b>] ip_rcv+0x27d/0x30a
> [ 8693.681053]  [<ffffffff81457e01>] __netif_receive_skb_core+0x3f2/0x483
> [ 8693.681056]  [<ffffffff81457eaa>] __netif_receive_skb+0x18/0x5a
> [ 8693.681058]  [<ffffffff81457f7c>] process_backlog+0x90/0x10c
> [ 8693.681061]  [<ffffffff814596e0>] net_rx_action+0x101/0x2aa
> [ 8693.681066]  [<ffffffff8106c39f>] __do_softirq+0x10c/0x26d
> [ 8693.681068]  [<ffffffff8106c51a>] run_ksoftirqd+0x1a/0x2f
> [ 8693.681071]  [<ffffffff81083675>] smpboot_thread_fn+0x149/0x167
> [ 8693.681074]  [<ffffffff8108352c>] ? sort_range+0x24/0x24
> [ 8693.681076]  [<ffffffff8108352c>] ? sort_range+0x24/0x24
> [ 8693.681080]  [<ffffffff81080d9c>] kthread+0xae/0xb6
> [ 8693.681082]  [<ffffffff81080303>] ? add_sysfs_param.isra.4+0xe1/0x18c
> [ 8693.681085]  [<ffffffff81080cee>] ? __kthread_parkme+0x61/0x61
> [ 8693.681088]  [<ffffffff81508152>] ret_from_fork+0x42/0x70
> [ 8693.681090]  [<ffffffff81080cee>] ? __kthread_parkme+0x61/0x61