netdev - Re: [PATCH net-next 03/14] udp: Don't set skb->encapsulation with RCO

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: <CALx6S37tNu-F3EU1ES37FCzm6-0TCfuFNig1==hnCa08A3FrQw@mail.gmail.com>
Date:	Thu, 5 May 2016 08:38:01 -0700
From:	Tom Herbert <tom@...bertland.com>
To:	Alexander Duyck <alexander.duyck@...il.com>
Cc:	David Miller <davem@...emloft.net>,
	Netdev <netdev@...r.kernel.org>, Kernel Team <kernel-team@...com>
Subject: Re: [PATCH net-next 03/14] udp: Don't set skb->encapsulation with RCO

On Wed, May 4, 2016 at 7:42 PM, Alexander Duyck
<alexander.duyck@...il.com> wrote:
> On Wed, May 4, 2016 at 6:02 PM, Tom Herbert <tom@...bertland.com> wrote:
>> When RCO is in effect we want to ensure that the outer checksum is
>> properly offloaded. Don't set skb->encapsulation in this case to
>> ensure that checksum offload is later considered for hw_features
>> instead of hw_enc_features.
>>
>> Signed-off-by: Tom Herbert <tom@...bertland.com>
>> ---
>>  net/ipv4/udp_offload.c | 10 ++++++----
>>  1 file changed, 6 insertions(+), 4 deletions(-)
>>
>> diff --git a/net/ipv4/udp_offload.c b/net/ipv4/udp_offload.c
>> index b556ef6..4eedec6 100644
>> --- a/net/ipv4/udp_offload.c
>> +++ b/net/ipv4/udp_offload.c
>> @@ -94,11 +94,13 @@ static struct sk_buff *__skb_udp_tunnel_segment(struct sk_buff *skb,
>>         do {
>>                 unsigned int len;
>>
>> -               if (remcsum)
>> +               if (remcsum) {
>>                         skb->ip_summed = CHECKSUM_NONE;
>> -
>> -               /* Set up inner headers if we are offloading inner checksum */
>> -               if (skb->ip_summed == CHECKSUM_PARTIAL) {
>> +                       skb->encapsulation = 0;
>> +               } else if (skb->ip_summed == CHECKSUM_PARTIAL) {
>> +                       /* Set up inner headers if we are offloading inner
>> +                        * checksum
>> +                        */
>>                         skb_reset_inner_headers(skb);
>>                         skb->encapsulation = 1;
>>                 }
>
>
> Why are you wasting cycles clearing a value that should have already
> been cleared?
>
> We set skb->encapsulation to 0 for the incoming skb before we segment
> it.  As such all of the segments we get should also not have it set.
> It seems like you are just wasting cycles writing it again even though
> it isn't written.
>
I believe this is needed because skb->encapsulation could have been
set at a lower inner GRO, like if we were encapsulating GRE in UDP...

Tom

> - Alex