[PATCH v2 0/1] net: geneve: accept every ethertype

[PATCH v2 0/1] 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, for a reason not known
to me, 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, LLC) and the data-plane
traffic (MPLS) can be encapsulated without an Ethernet frame, making
lightweight overlay networks a possibility.

Changes in v2:
  - added a cover letter
  - lines no longer exceed 80 columns


Josef Miegl (1):
  net: geneve: accept every ethertype

 drivers/net/geneve.c | 15 ++++-----------
 1 file changed, 4 insertions(+), 11 deletions(-)

-- 
2.37.1

[PATCH v2 1/1] net: geneve: accept every ethertype

[Josef Miegl]

This patch removes a restriction that prohibited receiving encapsulated
ethertypes other than IPv4, IPv6 and Ethernet.

With IFLA_GENEVE_INNER_PROTO_INHERIT flag set, GENEVE interface can now
receive ethertypes such as MPLS.

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

diff --git a/drivers/net/geneve.c b/drivers/net/geneve.c
index 89ff7f8e8c7e..7973659a891f 100644
--- a/drivers/net/geneve.c
+++ b/drivers/net/geneve.c
@@ -353,7 +353,6 @@ static int geneve_udp_encap_recv(struct sock *sk, struct sk_buff *skb)
 	struct genevehdr *geneveh;
 	struct geneve_dev *geneve;
 	struct geneve_sock *gs;
-	__be16 inner_proto;
 	int opts_len;
 
 	/* Need UDP and Geneve header to be present */
@@ -365,13 +364,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;
@@ -381,14 +373,15 @@ static int geneve_udp_encap_recv(struct sock *sk, struct sk_buff *skb)
 		goto drop;
 
 	if (unlikely((!geneve->cfg.inner_proto_inherit &&
-		      inner_proto != htons(ETH_P_TEB)))) {
+		      geneveh->proto_type != htons(ETH_P_TEB)))) {
 		geneve->dev->stats.rx_dropped++;
 		goto drop;
 	}
 
 	opts_len = geneveh->opt_len * 4;
-	if (iptunnel_pull_header(skb, GENEVE_BASE_HLEN + opts_len, inner_proto,
-				 !net_eq(geneve->net, dev_net(geneve->dev)))) {
+	if (iptunnel_pull_header(skb, GENEVE_BASE_HLEN + opts_len,
+				 geneveh->proto_type, !net_eq(geneve->net,
+				 dev_net(geneve->dev)))) {
 		geneve->dev->stats.rx_dropped++;
 		goto drop;
 	}
-- 
2.37.1

Re: [PATCH v2 1/1] net: geneve: accept every ethertype

[Eyal Birger]

On Mon, Feb 27, 2023 at 10:14 AM Josef Miegl <josef@miegl.cz> wrote:
>
> This patch removes a restriction that prohibited receiving encapsulated
> ethertypes other than IPv4, IPv6 and Ethernet.
>
> With IFLA_GENEVE_INNER_PROTO_INHERIT flag set, GENEVE interface can now
> receive ethertypes such as MPLS.
>
> Signed-off-by: Josef Miegl <josef@miegl.cz>
> ---
>  drivers/net/geneve.c | 15 ++++-----------
>  1 file changed, 4 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/net/geneve.c b/drivers/net/geneve.c
> index 89ff7f8e8c7e..7973659a891f 100644
> --- a/drivers/net/geneve.c
> +++ b/drivers/net/geneve.c
> @@ -353,7 +353,6 @@ static int geneve_udp_encap_recv(struct sock *sk, struct sk_buff *skb)
>         struct genevehdr *geneveh;
>         struct geneve_dev *geneve;
>         struct geneve_sock *gs;
> -       __be16 inner_proto;

nit: why remove the variable? - it's still used in two places and this
change just makes the patch longer.

>         int opts_len;
>
>         /* Need UDP and Geneve header to be present */
> @@ -365,13 +364,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;
> @@ -381,14 +373,15 @@ static int geneve_udp_encap_recv(struct sock *sk, struct sk_buff *skb)
>                 goto drop;
>
>         if (unlikely((!geneve->cfg.inner_proto_inherit &&
> -                     inner_proto != htons(ETH_P_TEB)))) {
> +                     geneveh->proto_type != htons(ETH_P_TEB)))) {
>                 geneve->dev->stats.rx_dropped++;
>                 goto drop;
>         }
>
>         opts_len = geneveh->opt_len * 4;
> -       if (iptunnel_pull_header(skb, GENEVE_BASE_HLEN + opts_len, inner_proto,
> -                                !net_eq(geneve->net, dev_net(geneve->dev)))) {
> +       if (iptunnel_pull_header(skb, GENEVE_BASE_HLEN + opts_len,
> +                                geneveh->proto_type, !net_eq(geneve->net,
> +                                dev_net(geneve->dev)))) {
>                 geneve->dev->stats.rx_dropped++;
>                 goto drop;
>         }
> --
> 2.37.1
>

Re: [PATCH v2 1/1] net: geneve: accept every ethertype

[Josef Miegl]

February 27, 2023 10:31 AM, "Eyal Birger" <eyal.birger@gmail.com> wrote:

> On Mon, Feb 27, 2023 at 10:14 AM Josef Miegl <josef@miegl.cz> wrote:
> 
>> This patch removes a restriction that prohibited receiving encapsulated
>> ethertypes other than IPv4, IPv6 and Ethernet.
>> 
>> With IFLA_GENEVE_INNER_PROTO_INHERIT flag set, GENEVE interface can now
>> receive ethertypes such as MPLS.
>> 
>> Signed-off-by: Josef Miegl <josef@miegl.cz>
>> ---
>> drivers/net/geneve.c | 15 ++++-----------
>> 1 file changed, 4 insertions(+), 11 deletions(-)
>> 
>> diff --git a/drivers/net/geneve.c b/drivers/net/geneve.c
>> index 89ff7f8e8c7e..7973659a891f 100644
>> --- a/drivers/net/geneve.c
>> +++ b/drivers/net/geneve.c
>> @@ -353,7 +353,6 @@ static int geneve_udp_encap_recv(struct sock *sk, struct sk_buff *skb)
>> struct genevehdr *geneveh;
>> struct geneve_dev *geneve;
>> struct geneve_sock *gs;
>> - __be16 inner_proto;
> 
> nit: why remove the variable? - it's still used in two places and this
> change just makes the patch longer.

I thought making the code shorter would be a better option, usage in two
places doesn't justify a dedicated variable in my mind.

>> int opts_len;
>> 
>> /* Need UDP and Geneve header to be present */
>> @@ -365,13 +364,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;
>> @@ -381,14 +373,15 @@ static int geneve_udp_encap_recv(struct sock *sk, struct sk_buff *skb)
>> goto drop;
>> 
>> if (unlikely((!geneve->cfg.inner_proto_inherit &&
>> - inner_proto != htons(ETH_P_TEB)))) {
>> + geneveh->proto_type != htons(ETH_P_TEB)))) {
>> geneve->dev->stats.rx_dropped++;
>> goto drop;
>> }
>> 
>> opts_len = geneveh->opt_len * 4;
>> - if (iptunnel_pull_header(skb, GENEVE_BASE_HLEN + opts_len, inner_proto,
>> - !net_eq(geneve->net, dev_net(geneve->dev)))) {
>> + if (iptunnel_pull_header(skb, GENEVE_BASE_HLEN + opts_len,
>> + geneveh->proto_type, !net_eq(geneve->net,
>> + dev_net(geneve->dev)))) {
>> geneve->dev->stats.rx_dropped++;
>> goto drop;
>> }
>> --
>> 2.37.1

Re: [PATCH v2 0/1] net: geneve: accept every ethertype

[Eyal Birger]

Hi,

On Mon, Feb 27, 2023 at 10:19 AM Josef Miegl <josef@miegl.cz> 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, for a reason not known
> to me, it imposed a restriction that prohibits receiving payloads other
> than IPv4, IPv6 and Ethernet.

FWIW I added support for IPv4/IPv6 because these are the use cases I had
and could validate. I don't know what problems could arise from supporting
all possible ethertypes and can't test that.

>
> 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 seems like an addition not a bugfix so personally seems like it should
be targeting net-next (which is currently closed afaik).

Eyal.

>
> This is especially useful if one wants to encapsulate MPLS, because with
> this patch the control-plane traffic (IP, LLC) and the data-plane
> traffic (MPLS) can be encapsulated without an Ethernet frame, making
> lightweight overlay networks a possibility.
>
> Changes in v2:
>   - added a cover letter
>   - lines no longer exceed 80 columns
>
>
> Josef Miegl (1):
>   net: geneve: accept every ethertype
>
>  drivers/net/geneve.c | 15 ++++-----------
>  1 file changed, 4 insertions(+), 11 deletions(-)
>
> --
> 2.37.1
>

Re: [PATCH v2 0/1] net: geneve: accept every ethertype

[Josef Miegl]

February 27, 2023 10:30 AM, "Eyal Birger" <eyal.birger@gmail.com> wrote:

> Hi,
> 
> On Mon, Feb 27, 2023 at 10:19 AM Josef Miegl <josef@miegl.cz> 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, for a reason not known
>> to me, it imposed a restriction that prohibits receiving payloads other
>> than IPv4, IPv6 and Ethernet.
> 
> FWIW I added support for IPv4/IPv6 because these are the use cases I had
> and could validate. I don't know what problems could arise from supporting
> all possible ethertypes and can't test that.

Yeah, I am hoping someone knowledgeable will tell whether this is a good
or bad idea. However I think that if any problem could arise, this is not
the place to artificially restrict payload types and potentional safeguarding
should be done somewhere down the packet chain.

I can't imagine adding a payload Ethertype every time someone needs a
specific use-case would be a good idea.

>> 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 seems like an addition not a bugfix so personally seems like it should
> be targeting net-next (which is currently closed afaik).

One could say the receive function should have behaved like that, the
transmit function already encapsulates every possible Ethertype and
IFLA_GENEVE_INNER_PROTO_INHERIT doesn't sound like it should be limited to
IPv4 and IPv6.

If no further modifications down the packet chain are required, I'd say it's
50/50. However I haven't contributed to the Linux kernel ever before, so I
really have no clue as to how things go.

> Eyal.
> 
>> This is especially useful if one wants to encapsulate MPLS, because with
>> this patch the control-plane traffic (IP, LLC) and the data-plane
>> traffic (MPLS) can be encapsulated without an Ethernet frame, making
>> lightweight overlay networks a possibility.
>> 
>> Changes in v2:
>> - added a cover letter
>> - lines no longer exceed 80 columns
>> 
>> Josef Miegl (1):
>> net: geneve: accept every ethertype
>> 
>> drivers/net/geneve.c | 15 ++++-----------
>> 1 file changed, 4 insertions(+), 11 deletions(-)
>> 
>> --
>> 2.37.1

Re: [PATCH v2 0/1] net: geneve: accept every ethertype

[Eyal Birger]

On Mon, Feb 27, 2023 at 11:57 AM Josef Miegl <josef@miegl.cz> wrote:
>
> February 27, 2023 10:30 AM, "Eyal Birger" <eyal.birger@gmail.com> wrote:
>
> > Hi,
> >
> > On Mon, Feb 27, 2023 at 10:19 AM Josef Miegl <josef@miegl.cz> 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, for a reason not known
> >> to me, it imposed a restriction that prohibits receiving payloads other
> >> than IPv4, IPv6 and Ethernet.
> >
> > FWIW I added support for IPv4/IPv6 because these are the use cases I had
> > and could validate. I don't know what problems could arise from supporting
> > all possible ethertypes and can't test that.
>
> Yeah, I am hoping someone knowledgeable will tell whether this is a good
> or bad idea. However I think that if any problem could arise, this is not
> the place to artificially restrict payload types and potentional safeguarding
> should be done somewhere down the packet chain.
>
> I can't imagine adding a payload Ethertype every time someone needs a
> specific use-case would be a good idea.

I guess it's just a matter of practicality - which decision imposes more
burden on future maintenance.

>
> >> 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 seems like an addition not a bugfix so personally seems like it should
> > be targeting net-next (which is currently closed afaik).
>
> One could say the receive function should have behaved like that, the
> transmit function already encapsulates every possible Ethertype and
> IFLA_GENEVE_INNER_PROTO_INHERIT doesn't sound like it should be limited to
> IPv4 and IPv6.

Indeed the flag is intentionally generic to allow for future extensions
without having to rename. But both in the commit message, and in the iproute2
man page I noted support for IPv4/IPv6.

>
> If no further modifications down the packet chain are required, I'd say it's
> 50/50. However I haven't contributed to the Linux kernel ever before, so I
> really have no clue as to how things go.
>
> > Eyal.
> >
> >> This is especially useful if one wants to encapsulate MPLS, because with
> >> this patch the control-plane traffic (IP, LLC) and the data-plane
> >> traffic (MPLS) can be encapsulated without an Ethernet frame, making
> >> lightweight overlay networks a possibility.
> >>
> >> Changes in v2:
> >> - added a cover letter
> >> - lines no longer exceed 80 columns
> >>
> >> Josef Miegl (1):
> >> net: geneve: accept every ethertype
> >>
> >> drivers/net/geneve.c | 15 ++++-----------
> >> 1 file changed, 4 insertions(+), 11 deletions(-)
> >>
> >> --
> >> 2.37.1

Re: [PATCH v2 0/1] net: geneve: accept every ethertype

[Jakub Kicinski]

On Mon, 27 Feb 2023 12:05:51 +0200 Eyal Birger wrote:
> > > This seems like an addition not a bugfix so personally seems like it should
> > > be targeting net-next (which is currently closed afaik).  
> >
> > One could say the receive function should have behaved like that, the
> > transmit function already encapsulates every possible Ethertype and
> > IFLA_GENEVE_INNER_PROTO_INHERIT doesn't sound like it should be limited to
> > IPv4 and IPv6.  
> 
> Indeed the flag is intentionally generic to allow for future extensions
> without having to rename. But both in the commit message, and in the iproute2
> man page I noted support for IPv4/IPv6.
> 
> > If no further modifications down the packet chain are required, I'd say it's
> > 50/50. However I haven't contributed to the Linux kernel ever before, so I
> > really have no clue as to how things go.

I think net-next is a better target, Eyal's explanation that the check
is intentional seems quite reasonable. FWIW, when you repost feel free
to fold the info from the cover letter into the patch description. 
With a single patch cover letters just split the info unnecessarily.