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]

Date:	Thu, 5 May 2016 08:44:09 -0700
From:	Alexander Duyck <alexander.duyck@...il.com>
To:	Tom Herbert <tom@...bertland.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 Thu, May 5, 2016 at 8:38 AM, Tom Herbert <tom@...bertland.com> wrote:
> 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...

That cannot happen.  The GRE header processing is completely skipped
as we use the inner_mac_header to determine the next header to jump
to.  The L3 types use encap_level and we reset that as well so there
should be no risk of skb->encapsulation being set for a remote
checksum offloaded frame.

- Alex