Re: [PATCH net-next] net: geneve: accept every ethertype

[Eyal Birger]

Hi,

On Mon, Mar 13, 2023 at 8:35 PM Simon Horman <simon.horman@xxxxxxxxxxxx> wrote:
>
> On Mon, Mar 13, 2023 at 05:14:58PM +0000, Josef Miegl wrote:
> > March 13, 2023 5:48 PM, "Simon Horman" <simon.horman@xxxxxxxxxxxx> wrote:
> >
> > > +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 Type
> > >> field, which states the Ethertype of the payload appearing after the
> > >> Geneve header.
> > >>
> > >> Commit 435fe1c0c1f7 ("net: geneve: support IPv4/IPv6 as inner protocol")
> > >> introduced a new IFLA_GENEVE_INNER_PROTO_INHERIT flag that allowed the
> > >> use of other Ethertypes than Ethernet. However, it imposed a restriction
> > >> that prohibits receiving payloads other than IPv4, IPv6 and Ethernet.
> > >>
> > >> This patch removes this restriction, making it possible to receive any
> > >> 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 will
> > 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 feeling
> > > 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 clause
> > > 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 these
> > implications exist, safeguarding should be done somewhere down the stock.
>
> 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 != 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