netdev - Re: [PATCH net-next] net-gre-gro: Fix a bug that breaks the forwarding path

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: <CA+mtBx8m+w9bkW8L+dDM-0coCVqgwx77x0kruQnGE+X2Y3+T3g@mail.gmail.com>
Date:	Tue, 15 Jul 2014 11:44:12 -0700
From:	Tom Herbert <therbert@...gle.com>
To:	Jerry Chu <hkchu@...gle.com>
Cc:	Or Gerlitz <or.gerlitz@...il.com>,
	Eric Dumazet <edumazet@...gle.com>,
	Or Gerlitz <ogerlitz@...lanox.com>,
	Wolfgang Walter <linux@...m.de>,
	David Miller <davem@...emloft.net>,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: Re: [PATCH net-next] net-gre-gro: Fix a bug that breaks the
 forwarding path

On Tue, Jul 15, 2014 at 10:17 AM, Jerry Chu <hkchu@...gle.com> wrote:
> On Tue, Jul 15, 2014 at 7:06 AM, Or Gerlitz <or.gerlitz@...il.com> wrote:
>> On Tue, Jul 15, 2014 at 12:30 AM, Jerry Chu <hkchu@...gle.com> wrote:
>>>
>>> On Mon, Jul 14, 2014 at 1:38 PM, Or Gerlitz <or.gerlitz@...il.com> wrote:
>>> > On Mon, Jul 14, 2014 at 10:16 PM, Jerry Chu <hkchu@...gle.com> wrote:
>>> >> On Mon, Jul 14, 2014 at 12:05 PM, Or Gerlitz <or.gerlitz@...il.com> wrote:
>>> >>> On Sun, Jul 13, 2014 at 5:46 PM, H.K. Jerry Chu <hkchu@...gle.com> wrote:
>>> >>>> Fixed a bug that was introduced by my GRE-GRO patch
>>> >>>> (bf5a755f5e9186406bbf50f4087100af5bd68e40 net-gre-gro: Add GRE
>>> >>>> support to the GRO stack) that breaks the forwarding path
>>> >>>> because various GSO related fields were not set. The bug will
>>> >>>> cause on the egress path either the GSO code to fail, or a
>>> >>>> GRE-TSO capable (NETIF_F_GSO_GRE) NICs to choke. The following
>>> >>>> fix has been tested for both cases.
>>> >
>>> >>> Anything different in this version vs. the one you posted earlier on
>>> >>> February or this is a plain re-post?
>>> >
>>> >> I simply moved the patch against the latest net-next and resolved some small
>>> >> conflict so yes it's pretty much the same.
>>> >
>>> > OK, got it.
>>> >
>>> >> Also I don't see the subsequent
>>> >> discussion on skb->encapsulation affects the validity of this patch so
>>> >> i'm resubmitting it. Also the patch has been confirmed to address the problem
>>> >> Wolfgang reported last week. Feel free to test against the configuration (VXLAN?)
>>> >> you had some question about earlier.
>>> >
>>> > Well, re-reading that thread, we were not very decisive there... my
>>> > comment of setting the inner network header twice in inet_gro_complete
>>> > doesn't apply only to vxlan but to other tunneling protocols.
>>>
>>> Understood, but my reply about it applying to the inner most hdr in the end
>>> is not limited to vxlan either.
>>>
>>> > Also, if
>>> > we really need (why do we? or explained it on the Feb thread) to set
>>> > skb->encapsulation for the sake of TX in a protocol (GRE) gro
>>> > complete, looks a bit fishy to me...
>>>
>>> Why does it look fishy? When h/w support sLRO on tunneled pkts
>>> driver will set that bit. So it seems very reasonable for the GRO stack
>>> that supports tunneled pkts to set that bit too.
>>
>>
>> what do you mean by "h/w support sLRO on tunneled pkts" here? as far
>> as I understand, the driver should set the bit when they know it's an
>> encapsulated
>> packet for which the HW offloaded checksum verification.
>>
>> So my fishiness comment was referring to the fact that we see here
>> that the stack
>> is setting this bit too sometimes.
>
> There are many other places in the "stack" that will set that bit if you
> grep it. I think many of them are there to make GSO work on tunneled pkts.
> The code here is no different.
>
>>
>>> > but saying this I think brings us
>>> > back to that incomplete discussion [1]  sounds as this needs some
>>> > plumbering... Still, this way or another I understand a regression was
>>> > introduced here and should be fixed.
>>
>>> Yep. I thought we've made reasonable progress on the skb->encapsualtion
>>> even though we can't close all the questions/issues
>>
>> where/how exactly (or more or less) this progress comes into play, is it
>> by the description exchanged in the emails sent over the Feb/Mar thread
>> or in Tom's 3.15/16/17 patches?
>
> Davem wanted a more precise definition of skb->encapsulation and I thought
> some worthy discussion has happened as part of the thread of the
> original patch (even though it left with some more questions but
> that's often the case
> with complex kernel code, right?)
>
Unfortunately, skb->encapsulation is overloaded with different
meanings in TX and RX path. I think the invariant should be that if it
is set, the inner headers are valid, so this is currently only useful
on TX. For Rx, where it's used to indicate inner checksum was
validated, I intend to a add a separate field in skbuf (probably two
bits to allow for multiple level encapsulations).

>>
>>> (but we have a real bug that needs to be fixed here).
>>
>> agree...
--
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