| 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
| ||
|
Date: Fri, 19 Dec 2014 16:22:34 +0530 (IST)
From: Govindarajulu Varadarajan <_govind@....com>
To: Jiri Benc <jbenc@...hat.com>
cc: Jay Vosburgh <jay.vosburgh@...onical.com>,
Eric Dumazet <eric.dumazet@...il.com>,
Govindarajulu Varadarajan <_govind@....com>,
davem@...emloft.net, netdev@...r.kernel.org, ssujith@...co.com,
benve@...co.com, Stefan Assmann <sassmann@...hat.com>
Subject: Re: [PATCH net] enic: fix rx skb checksum
On Fri, 19 Dec 2014, Jiri Benc wrote:
> On Thu, 18 Dec 2014 09:39:27 -0800, Jay Vosburgh wrote:
>> I've actually been looking into this "hw csum failure" (as it
>> appears with OVS and VXLAN) the last couple of days, and, at least on
>> the sky2 hardware I have, the problem doesn't appear to be the
>> hardware's CHECKSUM_COMPLETE checksumming.
>
> With the enic driver, the problem _is_ the hardware checksumming.
>
> While debugging the "hw csum failure" messages, I verified the checksum
> returned by the hardware directly in the driver (in
> enic_rq_indicate_buf). It appears that for some packets (most notably
> ICMP ones), the hardware returns 0xffff. I did not see any other wrong
> value, in other words, the returned checksum was either correct, or
> 0xffff.
>
Hardware returns 0xffff for non tcp/udp packets. For tcp/udp packet it returns
pseudo checksum. Not the _whole_ pkt checksum.
With the following changes, I see this output:
diff --git a/drivers/net/ethernet/cisco/enic/enic_main.c b/drivers/net/ethernet/cisco/enic/enic_main.c
index e3dc629..0f2be67 100644
--- a/drivers/net/ethernet/cisco/enic/enic_main.c
+++ b/drivers/net/ethernet/cisco/enic/enic_main.c
@@ -1092,6 +1092,24 @@ static void enic_rq_indicate_buf(struct vnic_rq *rq,
}
if ((netdev->features & NETIF_F_RXCSUM) && !csum_not_calc) {
+ if (printk_ratelimit()) {
+ const struct iphdr *iph = (struct iphdr *)skb->data;
+ __u16 length_for_csum = 0;
+ __wsum pseudo_csum = 0;
+
+ length_for_csum = (be16_to_cpu(iph->tot_len) - (iph->ihl << 2));
+ pseudo_csum = csum_tcpudp_nofold(iph->saddr,
+ iph->daddr,
+ length_for_csum,
+ iph->protocol, 0);
+
+ pr_info("saddr=%x, daddr=%x, length=%d, proto=%d\n",
+ iph->saddr, iph->daddr, length_for_csum, iph->protocol);
+ pr_info("hw_checksum = %x, pseudo_checksum_32=%x, pseudo_checksum_fold=%x\n",
+ htons(checksum),
+ pseudo_csum,
+ csum_fold(pseudo_csum));
+ }
skb->csum = htons(checksum);
skb->ip_summed = CHECKSUM_COMPLETE;
}
Output:
Dec 18 11:13:18 a163 kernel: enic: saddr=96d8690a, daddr=a3ba6a0a, length=40, proto=6
Dec 18 11:13:18 a163 kernel: enic: hw_checksum = c457, pseudo_checksum_32=3a930115, pseudo_checksum_fold=c457
Dec 18 11:13:18 a163 kernel: enic: saddr=a37410a, daddr=a3ba6a0a, length=32, proto=6
Dec 18 11:13:18 a163 kernel: enic: hw_checksum = 80f9, pseudo_checksum_32=adf1d114, pseudo_checksum_fold=80f9
Dec 18 11:13:18 a163 kernel: enic: saddr=a37410a, daddr=a3ba6a0a, length=32, proto=6
Dec 18 11:13:18 a163 kernel: enic: hw_checksum = 80f9, pseudo_checksum_32=adf1d114, pseudo_checksum_fold=80f9
Clearly hw is returning folded pseudo checksum.
> I have no idea whether the hardware verified the checksum for the
> 0xffff case and is just not returning the checksum or whether such
> packets come completely unverified.
>
Yes, hardware verifies the checksum and sets tcp_udp_csum_ok flag to 1.
If pkt verification fails or pkt is not tcp/udp, tcp_udp_csum_ok is 0. And we
send the pkt to stack with CHECKSUM_NONE.
> As for Govindarajulu's patch, I believe we can do better. First, its
> description does not match what I'm seeing (I see correct checksum
> provided in most cases) and second, the driver should provide
> CHECKSUM_COMPLETE whenever possible; and it seems to be possible.
>
Driver should use CHECKSUM_COMPLETE only if it can produce _whole_ pkt checksum.
as described in include/linux/skbuff.h:75
* CHECKSUM_COMPLETE:
*
* This is the most generic way. The device supplied checksum of the _whole_
* packet as seen by netif_rx() and fills out in skb->csum. Meaning, the
* hardware doesn't need to parse L3/L4 headers to implement this.
*
* Note: Even if device supports only some protocols, but is able to produce
* skb->csum, it MUST use CHECKSUM_COMPLETE, not CHECKSUM_UNNECESSARY.
Since enic hw verifies checksum but does not provide us whole pkt checksum,
it should use CHECKSUM_UNNECESSARY. as described in include/linux/skbuff.h:47
* CHECKSUM_UNNECESSARY:
*
* The hardware you're dealing with doesn't calculate the full checksum
* (as in CHECKSUM_COMPLETE), but it does parse headers and verify checksums
* for specific protocols. For such packets it will set CHECKSUM_UNNECESSARY
* if their checksums are okay. skb->csum is still undefined in this case
Am I correct?
>> I've also not tested it on enic hardware. Govindarajulu, would
>> you be able to test this against the unmodified enic driver and see if
>> it resolves the problem for you?
>
I think Jay Vosburgh's patch and my patch are addressing two different issues.
I am trying to fix "Do not set CHECKSUM_COMPLETE, when driver does not have
checksum of whole pkt."
Is my understanding correct?
Thanks
> I'm quite sure it does not, the skb->csum field is incorrect even
> before the skb enters the stack.
>
> Jiri
>
> --
> Jiri Benc
>
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Powered by blists - more mailing lists