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

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

[Josef Miegl]

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.

Signed-off-by: Josef Miegl <josef@miegl.cz>
---
 drivers/net/geneve.c | 9 ++-------
 1 file changed, 2 insertions(+), 7 deletions(-)

diff --git a/drivers/net/geneve.c b/drivers/net/geneve.c
index 89ff7f8e8c7e..32684e94eb4f 100644
--- a/drivers/net/geneve.c
+++ b/drivers/net/geneve.c
@@ -365,13 +365,6 @@ static int geneve_udp_encap_recv(struct sock *sk, struct sk_buff *skb)
 	if (unlikely(geneveh->ver != GENEVE_VER))
 		goto drop;
 
-	inner_proto = geneveh->proto_type;
-
-	if (unlikely((inner_proto != htons(ETH_P_TEB) &&
-		      inner_proto != htons(ETH_P_IP) &&
-		      inner_proto != htons(ETH_P_IPV6))))
-		goto drop;
-
 	gs = rcu_dereference_sk_user_data(sk);
 	if (!gs)
 		goto drop;
@@ -380,6 +373,8 @@ static int geneve_udp_encap_recv(struct sock *sk, struct sk_buff *skb)
 	if (!geneve)
 		goto drop;
 
+	inner_proto = geneveh->proto_type;
+
 	if (unlikely((!geneve->cfg.inner_proto_inherit &&
 		      inner_proto != htons(ETH_P_TEB)))) {
 		geneve->dev->stats.rx_dropped++;
-- 
2.37.1

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

[Simon Horman]

+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.

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. This would be after any patches that enhance the
stack to actually support this (I'm thinking of [1], though I haven't
looked at it closely).

[1] [PATCH net-next] net: geneve: set IFF_POINTOPOINT with IFLA_GENEVE_INNER_PROTO_INHERIT
    Link: https://lore.kernel.org/netdev/20230312164557.55354-1-josef@miegl.cz/


> Signed-off-by: Josef Miegl <josef@miegl.cz>
> ---
>  drivers/net/geneve.c | 9 ++-------
>  1 file changed, 2 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/net/geneve.c b/drivers/net/geneve.c
> index 89ff7f8e8c7e..32684e94eb4f 100644
> --- a/drivers/net/geneve.c
> +++ b/drivers/net/geneve.c
> @@ -365,13 +365,6 @@ static int geneve_udp_encap_recv(struct sock *sk, struct sk_buff *skb)
>  	if (unlikely(geneveh->ver != GENEVE_VER))
>  		goto drop;
>  
> -	inner_proto = geneveh->proto_type;
> -
> -	if (unlikely((inner_proto != htons(ETH_P_TEB) &&
> -		      inner_proto != htons(ETH_P_IP) &&
> -		      inner_proto != htons(ETH_P_IPV6))))
> -		goto drop;
> -
>  	gs = rcu_dereference_sk_user_data(sk);
>  	if (!gs)
>  		goto drop;
> @@ -380,6 +373,8 @@ static int geneve_udp_encap_recv(struct sock *sk, struct sk_buff *skb)
>  	if (!geneve)
>  		goto drop;
>  
> +	inner_proto = geneveh->proto_type;
> +
>  	if (unlikely((!geneve->cfg.inner_proto_inherit &&
>  		      inner_proto != htons(ETH_P_TEB)))) {
>  		geneve->dev->stats.rx_dropped++;
> -- 
> 2.37.1
> 

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

[Josef Miegl]

March 13, 2023 5:48 PM, "Simon Horman" <simon.horman@corigine.com> 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.

> 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.

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.

> This would be after any patches that enhance the
> stack to actually support this (I'm thinking of [1], though I haven't
> looked at it closely).
> 
> [1] [PATCH net-next] net: geneve: set IFF_POINTOPOINT with IFLA_GENEVE_INNER_PROTO_INHERIT
> Link: https://lore.kernel.org/netdev/20230312164557.55354-1-josef@miegl.cz

This patch just adds IFF_POINTOPOINT to a GENEVE device, it is unrelated.

>> Signed-off-by: Josef Miegl <josef@miegl.cz>
>> ---
>> drivers/net/geneve.c | 9 ++-------
>> 1 file changed, 2 insertions(+), 7 deletions(-)
>> 
>> diff --git a/drivers/net/geneve.c b/drivers/net/geneve.c
>> index 89ff7f8e8c7e..32684e94eb4f 100644
>> --- a/drivers/net/geneve.c
>> +++ b/drivers/net/geneve.c
>> @@ -365,13 +365,6 @@ static int geneve_udp_encap_recv(struct sock *sk, struct sk_buff *skb)
>> if (unlikely(geneveh->ver != GENEVE_VER))
>> goto drop;
>> 
>> - inner_proto = geneveh->proto_type;
>> -
>> - if (unlikely((inner_proto != htons(ETH_P_TEB) &&
>> - inner_proto != htons(ETH_P_IP) &&
>> - inner_proto != htons(ETH_P_IPV6))))
>> - goto drop;
>> -
>> gs = rcu_dereference_sk_user_data(sk);
>> if (!gs)
>> goto drop;
>> @@ -380,6 +373,8 @@ static int geneve_udp_encap_recv(struct sock *sk, struct sk_buff *skb)
>> if (!geneve)
>> goto drop;
>> 
>> + inner_proto = geneveh->proto_type;
>> +
>> if (unlikely((!geneve->cfg.inner_proto_inherit &&
>> inner_proto != htons(ETH_P_TEB)))) {
>> geneve->dev->stats.rx_dropped++;
>> --
>> 2.37.1

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

[Simon Horman]

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> 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.)

As I said in my previous email, I'm somewhat ambivalent towards this.
My main purpose in mentioning it is to make sure any changes
made in this area are made deliberately with due consideration.

> > This would be after any patches that enhance the
> > stack to actually support this (I'm thinking of [1], though I haven't
> > looked at it closely).
> > 
> > [1] [PATCH net-next] net: geneve: set IFF_POINTOPOINT with IFLA_GENEVE_INNER_PROTO_INHERIT
> > Link: https://lore.kernel.org/netdev/20230312164557.55354-1-josef@miegl.cz
> 
> This patch just adds IFF_POINTOPOINT to a GENEVE device, it is unrelated.

Understood.

> >> Signed-off-by: Josef Miegl <josef@miegl.cz>
> >> ---
> >> drivers/net/geneve.c | 9 ++-------
> >> 1 file changed, 2 insertions(+), 7 deletions(-)
> >> 
> >> diff --git a/drivers/net/geneve.c b/drivers/net/geneve.c
> >> index 89ff7f8e8c7e..32684e94eb4f 100644
> >> --- a/drivers/net/geneve.c
> >> +++ b/drivers/net/geneve.c
> >> @@ -365,13 +365,6 @@ static int geneve_udp_encap_recv(struct sock *sk, struct sk_buff *skb)
> >> if (unlikely(geneveh->ver != GENEVE_VER))
> >> goto drop;
> >> 
> >> - inner_proto = geneveh->proto_type;
> >> -
> >> - if (unlikely((inner_proto != htons(ETH_P_TEB) &&
> >> - inner_proto != htons(ETH_P_IP) &&
> >> - inner_proto != htons(ETH_P_IPV6))))
> >> - goto drop;
> >> -
> >> gs = rcu_dereference_sk_user_data(sk);
> >> if (!gs)
> >> goto drop;
> >> @@ -380,6 +373,8 @@ static int geneve_udp_encap_recv(struct sock *sk, struct sk_buff *skb)
> >> if (!geneve)
> >> goto drop;
> >> 
> >> + inner_proto = geneveh->proto_type;
> >> +
> >> if (unlikely((!geneve->cfg.inner_proto_inherit &&
> >> inner_proto != htons(ETH_P_TEB)))) {
> >> geneve->dev->stats.rx_dropped++;
> >> --
> >> 2.37.1

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@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> 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

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

[Simon Horman]

On Tue, Mar 14, 2023 at 11:55:07AM +0200, Eyal Birger wrote:
> Hi,
> 
> On Mon, Mar 13, 2023 at 8:35 PM 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> 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

Thanks.

I think I would be comfortable with this patch if the patch description
was updated to include some of the information above. I.e. why
this was not done before, and why it is felt that it is appropriate now.