netdev - RE: [net v2 2/3] i40e: Fix Rx checksum error counter

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]

Date:	Thu, 8 Jan 2015 20:33:03 +0000
From:	"Singhai, Anjali" <anjali.singhai@...el.com>
To:	Tom Herbert <therbert@...gle.com>
CC:	"Kirsher, Jeffrey T" <jeffrey.t.kirsher@...el.com>,
	David Miller <davem@...emloft.net>,
	Linux Netdev List <netdev@...r.kernel.org>,
	"nhorman@...hat.com" <nhorman@...hat.com>,
	"sassmann@...hat.com" <sassmann@...hat.com>,
	"jogreene@...hat.com" <jogreene@...hat.com>,
	"Rose, Gregory V" <gregory.v.rose@...el.com>
Subject: RE: [net v2 2/3] i40e: Fix Rx checksum error counter

On Wed, Jan 7, 2015 at 12:14 PM, Singhai, Anjali <anjali.singhai@...el.com> wrote:
> On Tue, 6 Jan 2015 21:43:57 -0800
> Tom Herbert <therbert@...gle.com> wrote:
>>> > @@ -1337,15 +1335,19 @@ static inline void i40e_rx_checksum(struct i40e_vsi *vsi,
>>> >                                           skb->protocol == htons(ETH_P_8021AD))
>>> >                                           ? VLAN_HLEN : 0;
>>> >
>>> > -               rx_udp_csum = udp_csum(skb);
>>> > -               iph = ip_hdr(skb);
>>> > -               csum = csum_tcpudp_magic(
>>> > -                               iph->saddr, iph->daddr,
>>> > -                               (skb->len - skb_transport_offset(skb)),
>>> > -                               IPPROTO_UDP, rx_udp_csum);
>>> > +               if ((ip_hdr(skb)->protocol == IPPROTO_UDP) &&
>>> > +                   (udp_hdr(skb)->check != 0)) {
>>> > +                       rx_udp_csum = udp_csum(skb);
>>>
>>> Doesn't this compute the whole checksum of the packet making the 
>>> fact that device verified inner checksum pretty much irrelevant? It 
>>> would probably be just as well to return CHECKSUM_NONE and let the 
>>> stack deal with it and remove all this complexity.
>>
>> This is only calculating outer UDP csum, inner csums are offloaded and so is the outer IP csum. Overall this is less work than asking the stack to do all of those by marking it as CHECKSUM_UNNECESSARY. We do have a patch in line to use csum_level but I believe even with that we would be asking the stack to do more work than necessary if we indicate that only inner checksums are offloaded. With our HW we are able to offload 3 out of 4 csums.
>
>The stack will only compute the checksum over the packet zero or one times. The fact that the driver is doing it instead of the stack makes little difference, once the decision is made to checksum the packet the performance gains of offloading are lost. If you defer to the stack then this complex code is removed and there is the possibility that the checksum doesn't even need to be calculated at all (like UDP packet is bad or no listener for port).
>
>What does "we are able to offload 3 out of 4 csums" mean?

With Fortville, the HW lets you offload inner IP and TCP/UDP checksum as well as outer IP checksum. The one that is not offloaded is outer UDP. I guess in most cases, outer UDP checksum will be zero and we do check for that, in the less likely case where outer UDP  checksum is non-zero we could defer it to the stack.  I guess I will simplify the code, although this patch is just fixing bugs already present in the upstream driver.

Thanks
Anjali