MIME-Version: 1.0
References: <ZA9T14Ks66HOlwH+@corigine.com> <20230312163726.55257-1-josef@miegl.cz>
 <57238dfc519a27b1b8d604879caa7b1b@miegl.cz> <ZA9s2Ti9PlUzsq/m@corigine.com>
In-Reply-To: <ZA9s2Ti9PlUzsq/m@corigine.com>
From:   Eyal Birger <eyal.birger@gmail.com>
Date:   Tue, 14 Mar 2023 11:55:07 +0200
Message-ID: <CAHsH6GsUAzye2puFES_5iemTtQZyoiR590NRPC8ZXrTg4B+OMA@mail.gmail.com>
Subject: Re: [PATCH net-next] net: geneve: accept every ethertype
To:     Simon Horman <simon.horman@corigine.com>
Cc:     Josef Miegl <josef@miegl.cz>,
        "David S. Miller" <davem@davemloft.net>,
        Eric Dumazet <edumazet@google.com>,
        Jakub Kicinski <kuba@kernel.org>,
        Paolo Abeni <pabeni@redhat.com>, netdev@vger.kernel.org,
        linux-kernel@vger.kernel.org, Pravin B Shelar <pshelar@ovn.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

Hi,

On Mon, Mar 13, 2023 at 8:35=E2=80=AFPM Simon Horman <simon.horman@corigine=
.com> wrote:
>
> On Mon, Mar 13, 2023 at 05:14:58PM +0000, Josef Miegl wrote:
> > March 13, 2023 5:48 PM, "Simon Horman" <simon.horman@corigine.com> wrot=
e:
> >
> > > +Pravin
> > >
> > > On Sun, Mar 12, 2023 at 05:37:26PM +0100, Josef Miegl wrote:
> > >
> > >> The Geneve encapsulation, as defined in RFC 8926, has a Protocol Typ=
e
> > >> field, which states the Ethertype of the payload appearing after the
> > >> Geneve header.
> > >>
> > >> Commit 435fe1c0c1f7 ("net: geneve: support IPv4/IPv6 as inner protoc=
ol")
> > >> introduced a new IFLA_GENEVE_INNER_PROTO_INHERIT flag that allowed t=
he
> > >> use of other Ethertypes than Ethernet. However, it imposed a restric=
tion
> > >> that prohibits receiving payloads other than IPv4, IPv6 and Ethernet=
.
> > >>
> > >> This patch removes this restriction, making it possible to receive a=
ny
> > >> Ethertype as a payload, if the IFLA_GENEVE_INNER_PROTO_INHERIT flag =
is
> > >> set.
> > >>
> > >> This is especially useful if one wants to encapsulate MPLS, because =
with
> > >> this patch the control-plane traffic (IP, IS-IS) and the data-plane
> > >> traffic (MPLS) can be encapsulated without an Ethernet frame, making
> > >> lightweight overlay networks a possibility.
> > >
> > > Hi Josef,
> > >
> > > I could be mistaken. But I believe that the thinking at the time,
> > > was based on the idea that it was better to only allow protocols that
> > > were known to work. And allow more as time goes on.
> >
> > Thanks for the reply Simon!
> >
> > What does "known to work" mean? Protocols that the net stack handles wi=
ll
> > work, protocols that Linux doesn't handle will not.
>
> Yes, a good question. But perhaps it was more "known to have been tested"=
.
>
> > > Perhaps we have moved away from that thinking (I have no strong feeli=
ng
> > > either way). Or perhaps this is safe because of some other guard. But=
 if
> > > not perhaps it is better to add the MPLS ethertype(s) to the if claus=
e
> > > rather than remove it.
> >
> > The thing is it is not just adding one ethertype. For my own use-case,
> > I would need to whitelist MPLS UC and 0x00fe for IS-IS. But I am sure
> > other people will want to use GENEVE` for xx other protocols.
>
> Right, so the list could be expanded for known cases.
> But I also understand your point,
> which I might describe as this adding friction.
>
> > The protocol handling seems to work, what I am not sure about is if
> > allowing all Ethertypes has any security implications. However, if thes=
e
> > implications exist, safeguarding should be done somewhere down the stoc=
k.
>
> Yes, I believe that the idea was to limit the scope of such risks.
> (Really, it was a long time ago, so I very likely don't recall everything=
.)

Digging a little into the history of this code I found this discussion [1]
where this specific point was raised:

<quote>
>> +       if (unlikely(geneveh->proto_type !=3D htons(ETH_P_TEB)))
>
> Why? I thought the point of geneve carrying protocol field was to
> allow protocols other than Ethernet... is this temporary maybe?

Yes, it is temporary. Currently OVS only handles Ethernet packets but
this restriction can be lifted once we have a consumer that is capable
of handling other protocols.
</quote>

This seems to have been ported as is when moving to a generic net device.

So now that the consumer is capable of other protocols, the question is
whether the restrictions should be lifted for any protocol or moderately.

I went with the moderate approach when adding IP support, but I do see the
merits in allowing any protocol without having to fiddle with this code.

https://www.spinics.net/lists/netdev/msg290579.html