netdev - Re: [PATCH net-next 7/8] net: ip_tunnel: remove 'csum_help' argument to iptunnel_handle

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:	Mon, 11 Jan 2016 10:15:32 -0800
From:	Alexander Duyck <alexander.duyck@...il.com>
To:	Edward Cree <ecree@...arflare.com>
Cc:	Tom Herbert <tom@...bertland.com>,
	David Miller <davem@...emloft.net>,
	Netdev <netdev@...r.kernel.org>, linux-net-drivers@...arflare.com
Subject: Re: [PATCH net-next 7/8] net: ip_tunnel: remove 'csum_help' argument
 to iptunnel_handle_offloads

On Mon, Jan 11, 2016 at 9:31 AM, Edward Cree <ecree@...arflare.com> wrote:
> On 11/01/16 16:39, Alexander Duyck wrote:
>> Your first patch is probably the best place for it.  Then when you
>> start setting false it doesn't introduce any errors.
> Will do.
>> Also I was doing a bit more work on the lco_csum function and think I
>> have come up with something a bit more elegant[...]  What I will do is
>> email you the full patch and the GSO patch I have as RFCs to look over
>> and possibly incorporate into your own.
> <snip>
>> +static inline __wsum lco_csum(struct sk_buff *skb)
>> +{
>> +       unsigned char *csum_start = skb_checksum_start(skb);
>> +       unsigned char *l4_hdr = skb_transport_header(skb);
>> +       __wsum partial;
>> +
>> +       partial = ~csum_unfold(*(__force __sum16 *)(csum_start +
>> +                                                   skb->csum_offset));
>> +       return csum_partial(l4_hdr, csum_start - l4_hdr, partial);
>> +}
>> +
> Looks OK to me.  I'd rather tack both of your patches onto the endof the
>  series, rather than incorporating your patch [1/2] directly into my patch
>  [1/8]; that way (a) the history allows to understand regular LCO before
>  adding in the GSO flavour, (b) you're credited for your improved lco_csum.

Actually if you don't plan to incorporate the function I am fine with
you just leaving it out for now.  I can focus on the GSO portions of
all this and rewrite this to just be an update of your patch.  Then
when you submit your patches and they are accepted I will submit the
GSO implementations.

> As for your patch [2/2], I don't pretend to understand GSO right now but it
>  looks plausible enough.  Perhaps you could add a document about GSO to go
>  alongside the checksum-offloads.txt one?

I don't know if we really need to.  The logic is essentially the same
as what you already have for the local checksum offload.  The only
difference is there is the GSO logic floating around in there to also
compute the outer checksum offload based on the inner that GSO had
already retained when it did the inner offload via software.

Feel free to just exclude it if you want.  I think there may be some
more work to be done on it, for example I am not sure if I actually
needed to move oflload_csum.  If I am not mistaken I think we might be
able to drop the skb->encap_hdr_csum the same way you dropped the
value from iptunnel_handle_offloads.  I might also spend some time
working on the GRE version of the patch as well.  Maybe I can see if
skb->encap_hdr_csum can be dropped.

> What testing haveyou done on your series?  When rebasing it I'll focus on
>  the tunnel types you haven't already tested.

I have been testing with the two Intel NICs I have using just VXLAN as
that was my focus.  I just submitted some patches to the Intel guys
that enable NETIF_F_HW_CSUM for igb, ixgbe, igbvf, and ixgbevf.  With
those patches in place I have been passing traffic without seeing any
issues.  I've been using wireshark to monitor the packets and verify
checksums on the receive side periodically.  It all seems to be
working correctly.

- Alex