net: openvswitch: IPv6: Add IPv6 extension header support

PATCH Superseded Toms Atteka 4 participants 11 messages Sep 28, 2021 – Oct 15, 2021

Subsystems (3)

Networking Network drivers Openvswitch

lore

Share & tools

lore

Toms Atteka

author

2021-09-28 19:47 UTC #0 lore raw

PATCH v6 1/1 net: openvswitch: IPv6: Add IPv6 extension header support

This change adds a new OpenFlow field OFPXMT_OFB_IPV6_EXTHDR and packets can be filtered using ipv6_ext flag.

Signed-off-by Toms Atteka <[email protected]>

---
 include/uapi/linux/openvswitch.h |  12 +++
 net/openvswitch/flow.c           | 140 +++++++++++++++++++++++++++++++
 net/openvswitch/flow.h           |  14 ++++
 net/openvswitch/flow_netlink.c   |  24 +++++-
 4 files changed, 189 insertions(+), 1 deletion(-)

include/uapi/linux/openvswitch.h +12 -0

index a87b44cd5590..dc6eb5f6399f 100644
@@ -346,6 +346,13 @@ enum ovs_key_attr {
 #ifdef __KERNEL__
 	OVS_KEY_ATTR_TUNNEL_INFO,  /* struct ip_tunnel_info */
 #endif
+
+#ifndef __KERNEL__
+	PADDING,  /* Padding so kernel and non kernel field count would match */
+#endif
+
+	OVS_KEY_ATTR_IPV6_EXTHDRS,  /* struct ovs_key_ipv6_exthdr */
+
 	__OVS_KEY_ATTR_MAX
 };

@@ -421,6 +428,11 @@ struct ovs_key_ipv6 {
 	__u8   ipv6_frag;	/* One of OVS_FRAG_TYPE_*. */
 };
 
+/* separate structure to support backward compatibility with older user space */
+struct ovs_key_ipv6_exthdrs {
+	__u16  hdrs;
+};
+
 struct ovs_key_tcp {
 	__be16 tcp_src;
 	__be16 tcp_dst;

net/openvswitch/flow.c +140 -0

152 lines not on this page · raw message

net/openvswitch/flow.h +14 -0

index 758a8c77f736..073ab73ffeaa 100644
@@ -32,6 +32,19 @@ enum sw_flow_mac_proto {
 #define SW_FLOW_KEY_INVALID	0x80
 #define MPLS_LABEL_DEPTH       3
 
+/* Bit definitions for IPv6 Extension Header pseudo-field. */
+enum ofp12_ipv6exthdr_flags {
+	OFPIEH12_NONEXT = 1 << 0,   /* "No next header" encountered. */
+	OFPIEH12_ESP    = 1 << 1,   /* Encrypted Sec Payload header present. */
+	OFPIEH12_AUTH   = 1 << 2,   /* Authentication header present. */
+	OFPIEH12_DEST   = 1 << 3,   /* 1 or 2 dest headers present. */
+	OFPIEH12_FRAG   = 1 << 4,   /* Fragment header present. */
+	OFPIEH12_ROUTER = 1 << 5,   /* Router header present. */
+	OFPIEH12_HOP    = 1 << 6,   /* Hop-by-hop header present. */
+	OFPIEH12_UNREP  = 1 << 7,   /* Unexpected repeats encountered. */
+	OFPIEH12_UNSEQ  = 1 << 8    /* Unexpected sequencing encountered. */
+};
+
 /* Store options at the end of the array if they are less than the
  * maximum size. This allows us to get the benefits of variable length
  * matching for small options.

@@ -121,6 +134,7 @@ struct sw_flow_key {
 				struct in6_addr dst;	/* IPv6 destination address. */
 			} addr;
 			__be32 label;			/* IPv6 flow label. */
+			u16 exthdrs;	/* IPv6 extension header flags */
 			union {
 				struct {
 					struct in6_addr src;

net/openvswitch/flow_netlink.c +23 -1

index 65c2e3458ff5..2fbf324fcfff 100644
@@ -367,7 +367,8 @@ size_t ovs_key_attr_size(void)
 		+ nla_total_size(2)   /* OVS_KEY_ATTR_ETHERTYPE */
 		+ nla_total_size(40)  /* OVS_KEY_ATTR_IPV6 */
 		+ nla_total_size(2)   /* OVS_KEY_ATTR_ICMPV6 */
-		+ nla_total_size(28); /* OVS_KEY_ATTR_ND */
+		+ nla_total_size(28)  /* OVS_KEY_ATTR_ND */
+		+ nla_total_size(2);  /* OVS_KEY_ATTR_IPV6_EXTHDRS */
 }
 
 static const struct ovs_len_tbl ovs_vxlan_ext_key_lens[OVS_VXLAN_EXT_MAX + 1] = {

@@ -435,6 +436,8 @@ static const struct ovs_len_tbl ovs_key_lens[OVS_KEY_ATTR_MAX + 1] = {
 		.len = sizeof(struct ovs_key_ct_tuple_ipv6) },
 	[OVS_KEY_ATTR_NSH]       = { .len = OVS_ATTR_NESTED,
 				     .next = ovs_nsh_key_attr_lens, },
+	[OVS_KEY_ATTR_IPV6_EXTHDRS] = {
+		.len = sizeof(struct ovs_key_ipv6_exthdrs) },
 };
 
 static bool check_attr_len(unsigned int attr_len, unsigned int expected_len)

@@ -1595,6 +1598,17 @@ static int ovs_key_from_nlattrs(struct net *net, struct sw_flow_match *match,
 		attrs &= ~(1 << OVS_KEY_ATTR_IPV6);
 	}
 
+	if (attrs & (1ULL << OVS_KEY_ATTR_IPV6_EXTHDRS)) {
+		const struct ovs_key_ipv6_exthdrs *ipv6_exthdrs_key;
+
+		ipv6_exthdrs_key = nla_data(a[OVS_KEY_ATTR_IPV6_EXTHDRS]);
+
+		SW_FLOW_KEY_PUT(match, ipv6.exthdrs,
+				ipv6_exthdrs_key->hdrs, is_mask);
+
+		attrs &= ~(1ULL << OVS_KEY_ATTR_IPV6_EXTHDRS);
+	}
+
 	if (attrs & (1 << OVS_KEY_ATTR_ARP)) {
 		const struct ovs_key_arp *arp_key;

@@ -2097,6 +2111,7 @@ static int __ovs_nla_put_key(const struct sw_flow_key *swkey,
 		ipv4_key->ipv4_frag = output->ip.frag;
 	} else if (swkey->eth.type == htons(ETH_P_IPV6)) {
 		struct ovs_key_ipv6 *ipv6_key;
+		struct ovs_key_ipv6_exthdrs *ipv6_exthdrs_key;
 
 		nla = nla_reserve(skb, OVS_KEY_ATTR_IPV6, sizeof(*ipv6_key));
 		if (!nla)

@@ -2111,6 +2126,13 @@ static int __ovs_nla_put_key(const struct sw_flow_key *swkey,
 		ipv6_key->ipv6_tclass = output->ip.tos;
 		ipv6_key->ipv6_hlimit = output->ip.ttl;
 		ipv6_key->ipv6_frag = output->ip.frag;
+
+		nla = nla_reserve(skb, OVS_KEY_ATTR_IPV6_EXTHDRS,
+				  sizeof(*ipv6_exthdrs_key));
+		if (!nla)
+			goto nla_put_failure;
+		ipv6_exthdrs_key = nla_data(nla);
+		ipv6_exthdrs_key->hdrs = output->ipv6.exthdrs;
 	} else if (swkey->eth.type == htons(ETH_P_NSH)) {
 		if (nsh_key_to_nlattr(&output->nsh, is_mask, skb))
 			goto nla_put_failure;

Signature · 1 line

2.25.1

Jakub Kicinski

maintainer

2021-09-29 00:49 UTC #1 lore raw

include/uapi/linux/openvswitch.h enum ovs_key_attr { · line 350

Quoted 10 lines · On Tue, 28 Sep 2021 12:47:27 -0700 Toms Atteka wrote:

include/uapi/linux/openvswitch.h +2 -0

index a87b44cd5590..dc6eb5f6399f 100644
@@ -346,6 +346,13 @@ enum ovs_key_attr {
 #ifdef __KERNEL__
 	OVS_KEY_ATTR_TUNNEL_INFO,  /* struct ip_tunnel_info */
 #endif
+
+#ifndef __KERNEL__

#else

include/uapi/linux/openvswitch.h enum ovs_key_attr { · line 351

Quoted 1 line

include/uapi/linux/openvswitch.h +1 -0
+	PADDING,  /* Padding so kernel and non kernel field count would match */

The name PADDING seems rather risky, collisions will be likely. OVS_KEY_ATTR_PADDING maybe?

But maybe we don't need to define this special value and bake it into the uAPI, why can't we add something like this to the kernel header (i.e. include/linux/openvswitch.h):

/* Insert a kernel only KEY_ATTR */
#define OVS_KEY_ATTR_TUNNEL_INFO __OVS_KEY_ATTR_MAX
#undef OVS_KEY_ATTR_MAX
#define OVS_KEY_ATTR_MAX __OVS_KEY_ATTR_MAX

Quoted 1 line

include/uapi/linux/openvswitch.h +1 -0
+#endif

Nicolas Dichtel

2021-09-29 06:19 UTC #2 lore raw

Le 29/09/2021 à 02:48, Jakub Kicinski a écrit :

Quoted 19 lines · On Tue, 28 Sep 2021 12:47:27 -0700 Toms Atteka wrote:

include/uapi/linux/openvswitch.h enum ovs_key_attr { · line 350
Quoted 10 lines
include/uapi/linux/openvswitch.h +2 -0
index a87b44cd5590..dc6eb5f6399f 100644
@@ -346,6 +346,13 @@ enum ovs_key_attr {
 #ifdef __KERNEL__
 	OVS_KEY_ATTR_TUNNEL_INFO,  /* struct ip_tunnel_info */
 #endif
+
+#ifndef __KERNEL__
#else
include/uapi/linux/openvswitch.h enum ovs_key_attr { · line 351
Quoted 1 line
include/uapi/linux/openvswitch.h +1 -0
+	PADDING,  /* Padding so kernel and non kernel field count would match */
The name PADDING seems rather risky, collisions will be likely. OVS_KEY_ATTR_PADDING maybe?

Quoted 8 lines

But maybe we don't need to define this special value and bake it into the uAPI, why can't we add something like this to the kernel header (i.e. include/linux/openvswitch.h):
/* Insert a kernel only KEY_ATTR */
#define OVS_KEY_ATTR_TUNNEL_INFO __OVS_KEY_ATTR_MAX
#undef OVS_KEY_ATTR_MAX
#define OVS_KEY_ATTR_MAX __OVS_KEY_ATTR_MAX

Following the other thread [1], this will break if a new app runs over an old kernel.
Why not simply expose this attribute to userspace and throw an error if a userspace app uses it?

[1]
https://lore.kernel.org/lkml/CAASuNyUWoZ1wToEUYbdehux=yVnWQ=suKDyRkQfRD-72DOLziw@mail.gmail.com/

Quoted 2 lines

Quoted 2 lines
include/uapi/linux/openvswitch.h +1 -0
+#endif

Jakub Kicinski

maintainer

2021-09-29 13:19 UTC #3 lore raw

Quoted 6 lines · On Wed, 29 Sep 2021 08:19:05 +0200 Nicolas Dichtel wrote:

Quoted 4 lines
/* Insert a kernel only KEY_ATTR */
#define OVS_KEY_ATTR_TUNNEL_INFO __OVS_KEY_ATTR_MAX
#undef OVS_KEY_ATTR_MAX
#define OVS_KEY_ATTR_MAX __OVS_KEY_ATTR_MAX
Following the other thread [1], this will break if a new app runs over an old kernel.

Good point.

Quoted 2 lines

Why not simply expose this attribute to userspace and throw an error if a userspace app uses it?

Does it matter if it's exposed or not? Either way the parsing policy for attrs coming from user space should have a reject for the value. (I say that not having looked at the code, so maybe I shouldn't...)

Cpp Code

author

2021-09-30 16:12 UTC #4 lore raw

Quoted 9 lines · Jakub Kicinski

Quoted 6 lines · On Wed, 29 Sep 2021 08:19:05 +0200 Nicolas Dichtel wrote:
Quoted 4 lines
/* Insert a kernel only KEY_ATTR */
#define OVS_KEY_ATTR_TUNNEL_INFO __OVS_KEY_ATTR_MAX
#undef OVS_KEY_ATTR_MAX
#define OVS_KEY_ATTR_MAX __OVS_KEY_ATTR_MAX
Following the other thread [1], this will break if a new app runs over an old kernel.

Quoted 8 lines

Good point.
Quoted 2 lines
Why not simply expose this attribute to userspace and throw an error if a userspace app uses it?
Does it matter if it's exposed or not? Either way the parsing policy for attrs coming from user space should have a reject for the value. (I say that not having looked at the code, so maybe I shouldn't...)

To remove some confusion, there are some architectural nuances if we want to extend code without large refactor.
The ovs_key_attr is defined only in kernel side. Userspace side is generated from this file. As well the code can be built without kernel modules.
The code inside OVS repository and net-next is not identical, but I try to keep some consistency.

JFYI This is the file responsible for generating userspace part:
https://github.com/openvswitch/ovs/blob/master/build-aux/extract-odp-netlink-h
This is the how corresponding file for ovs_key_attr looks inside OVS:
https://github.com/openvswitch/ovs/blob/master/datapath/linux/compat/include/linux/openvswitch.h
one can see there are more values than in net-next version.

Nicolas Dichtel

2021-10-01 07:21 UTC #5 lore raw

Le 30/09/2021 à 18:11, Cpp Code a écrit :

Quoted 19 lines · Jakub Kicinski

Quoted 9 lines · On Wed, 29 Sep 2021 08:19:05 +0200 Nicolas Dichtel wrote:
Quoted 6 lines
Quoted 4 lines
/* Insert a kernel only KEY_ATTR */
#define OVS_KEY_ATTR_TUNNEL_INFO __OVS_KEY_ATTR_MAX
#undef OVS_KEY_ATTR_MAX
#define OVS_KEY_ATTR_MAX __OVS_KEY_ATTR_MAX
Following the other thread [1], this will break if a new app runs over an old kernel.
Quoted 8 lines
Good point.
Quoted 2 lines
Why not simply expose this attribute to userspace and throw an error if a userspace app uses it?
Does it matter if it's exposed or not? Either way the parsing policy for attrs coming from user space should have a reject for the value. (I say that not having looked at the code, so maybe I shouldn't...)

Quoted 7 lines

To remove some confusion, there are some architectural nuances if we want to extend code without large refactor.
The ovs_key_attr is defined only in kernel side. Userspace side is generated from this file. As well the code can be built without kernel modules.
The code inside OVS repository and net-next is not identical, but I try to keep some consistency.

I didn't get why OVS_KEY_ATTR_TUNNEL_INFO cannot be exposed to userspace.

Quoted 6 lines

JFYI This is the file responsible for generating userspace part:
https://github.com/openvswitch/ovs/blob/master/build-aux/extract-odp-netlink-h
This is the how corresponding file for ovs_key_attr looks inside OVS:
https://github.com/openvswitch/ovs/blob/master/datapath/linux/compat/include/linux/openvswitch.h
one can see there are more values than in net-next version.

There are still some '#ifdef __KERNEL__'. The standard 'make headers_install' filters them. Why not using this standard mechanism?

In this file, there are two attributes (OVS_KEY_ATTR_PACKET_TYPE and OVS_KEY_ATTR_ND_EXTENSIONS) that doesn't exist in the kernel. This will also breaks if an old app runs over a new kernel. I don't see how it is possible to keep the compat between {old|new} {kernel|app}.

Regards,
Nicolas

Cpp Code

author

2021-10-01 20:42 UTC #6 lore raw

Quoted 21 lines · Nicolas Dichtel

Quoted 19 lines · Cpp Code
Quoted 17 lines · Jakub Kicinski
Quoted 6 lines · On Wed, 29 Sep 2021 08:19:05 +0200 Nicolas Dichtel wrote:
Quoted 4 lines
/* Insert a kernel only KEY_ATTR */
#define OVS_KEY_ATTR_TUNNEL_INFO __OVS_KEY_ATTR_MAX
#undef OVS_KEY_ATTR_MAX
#define OVS_KEY_ATTR_MAX __OVS_KEY_ATTR_MAX
Following the other thread [1], this will break if a new app runs over an old kernel.
Good point.
Quoted 2 lines
Why not simply expose this attribute to userspace and throw an error if a userspace app uses it?
Does it matter if it's exposed or not? Either way the parsing policy for attrs coming from user space should have a reject for the value. (I say that not having looked at the code, so maybe I shouldn't...)

Quoted 8 lines

Quoted 7 lines
To remove some confusion, there are some architectural nuances if we want to extend code without large refactor.
The ovs_key_attr is defined only in kernel side. Userspace side is generated from this file. As well the code can be built without kernel modules.
The code inside OVS repository and net-next is not identical, but I try to keep some consistency.
I didn't get why OVS_KEY_ATTR_TUNNEL_INFO cannot be exposed to userspace.

OVS_KEY_ATTR_TUNNEL_INFO is compressed version of OVS_KEY_ATTR_TUNNEL and for clarity purposes its not exposed to userspace as it will never use it.
I would say it's a coding style as it would not brake anything if exposed.

Quoted 9 lines

Quoted 7 lines
JFYI This is the file responsible for generating userspace part:
https://github.com/openvswitch/ovs/blob/master/build-aux/extract-odp-netlink-h
This is the how corresponding file for ovs_key_attr looks inside OVS:
https://github.com/openvswitch/ovs/blob/master/datapath/linux/compat/include/linux/openvswitch.h
one can see there are more values than in net-next version.
There are still some '#ifdef __KERNEL__'. The standard 'make headers_install' filters them. Why not using this standard mechanism?

Could you elaborate on this, I don't quite understand the idea!? Which ifdef you are referring, the one along OVS_KEY_ATTR_TUNNEL_INFO or some other?

Quoted 5 lines

In this file, there are two attributes (OVS_KEY_ATTR_PACKET_TYPE and OVS_KEY_ATTR_ND_EXTENSIONS) that doesn't exist in the kernel. This will also breaks if an old app runs over a new kernel. I don't see how it is possible to keep the compat between {old|new} {kernel|app}.

Looks like this most likely is a bug while working on multiple versions of code. Need to do add more padding.

Quoted 4 lines

Regards,
Nicolas

Nicolas Dichtel

2021-10-05 06:41 UTC #7 lore raw

Le 01/10/2021 à 22:42, Cpp Code a écrit :

Quoted 30 lines

Quoted 28 lines · Nicolas Dichtel
Quoted 26 lines · Cpp Code
Quoted 9 lines · Jakub Kicinski
Quoted 6 lines · On Wed, 29 Sep 2021 08:19:05 +0200 Nicolas Dichtel wrote:
Quoted 4 lines
/* Insert a kernel only KEY_ATTR */
#define OVS_KEY_ATTR_TUNNEL_INFO __OVS_KEY_ATTR_MAX
#undef OVS_KEY_ATTR_MAX
#define OVS_KEY_ATTR_MAX __OVS_KEY_ATTR_MAX
Following the other thread [1], this will break if a new app runs over an old kernel.
Quoted 8 lines
Good point.
Quoted 2 lines
Why not simply expose this attribute to userspace and throw an error if a userspace app uses it?
Does it matter if it's exposed or not? Either way the parsing policy for attrs coming from user space should have a reject for the value. (I say that not having looked at the code, so maybe I shouldn't...)
To remove some confusion, there are some architectural nuances if we want to extend code without large refactor.
The ovs_key_attr is defined only in kernel side. Userspace side is generated from this file. As well the code can be built without kernel modules.
The code inside OVS repository and net-next is not identical, but I try to keep some consistency.

Quoted 6 lines

Quoted 1 line
I didn't get why OVS_KEY_ATTR_TUNNEL_INFO cannot be exposed to userspace.
OVS_KEY_ATTR_TUNNEL_INFO is compressed version of OVS_KEY_ATTR_TUNNEL and for clarity purposes its not exposed to userspace as it will never use it.
I would say it's a coding style as it would not brake anything if exposed.

In fact, it's the best way to keep the compatibility in the long term. You can define it like this:
OVS_KEY_ATTR_TUNNEL_INFO, /* struct ip_tunnel_info, reserved for kernel use */

Quoted 8 lines

Quoted 8 lines
Quoted 8 lines
JFYI This is the file responsible for generating userspace part:
https://github.com/openvswitch/ovs/blob/master/build-aux/extract-odp-netlink-h
This is the how corresponding file for ovs_key_attr looks inside OVS:
https://github.com/openvswitch/ovs/blob/master/datapath/linux/compat/include/linux/openvswitch.h
one can see there are more values than in net-next version.

Quoted 6 lines

Quoted 2 lines
There are still some '#ifdef __KERNEL__'. The standard 'make headers_install' filters them. Why not using this standard mechanism?
Could you elaborate on this, I don't quite understand the idea!? Which ifdef you are referring, the one along OVS_KEY_ATTR_TUNNEL_INFO or some other?

My understanding is that this file is used for the userland third party, thus, theoretically, there should be no '#ifdef __KERNEL__'. uapi headers generated with 'make headers_install' are filtered to remove them.

Quoted 9 lines

Quoted 6 lines
In this file, there are two attributes (OVS_KEY_ATTR_PACKET_TYPE and OVS_KEY_ATTR_ND_EXTENSIONS) that doesn't exist in the kernel. This will also breaks if an old app runs over a new kernel. I don't see how it is possible to keep the compat between {old|new} {kernel|app}.
Looks like this most likely is a bug while working on multiple versions of code. Need to do add more padding.

As said above, just define the same uapi for everybody and the problem is gone forever.

Regards,
Nicolas

Cpp Code

author

2021-10-14 21:12 UTC #8 lore raw

Quoted 53 lines · Nicolas Dichtel

Quoted 30 lines · Cpp Code
Quoted 28 lines · Nicolas Dichtel
Quoted 26 lines · Cpp Code
Quoted 9 lines · Jakub Kicinski
Quoted 6 lines · On Wed, 29 Sep 2021 08:19:05 +0200 Nicolas Dichtel wrote:
Quoted 4 lines
/* Insert a kernel only KEY_ATTR */
#define OVS_KEY_ATTR_TUNNEL_INFO __OVS_KEY_ATTR_MAX
#undef OVS_KEY_ATTR_MAX
#define OVS_KEY_ATTR_MAX __OVS_KEY_ATTR_MAX
Following the other thread [1], this will break if a new app runs over an old kernel.
Quoted 8 lines
Good point.
Quoted 2 lines
Why not simply expose this attribute to userspace and throw an error if a userspace app uses it?
Does it matter if it's exposed or not? Either way the parsing policy for attrs coming from user space should have a reject for the value. (I say that not having looked at the code, so maybe I shouldn't...)
To remove some confusion, there are some architectural nuances if we want to extend code without large refactor.
The ovs_key_attr is defined only in kernel side. Userspace side is generated from this file. As well the code can be built without kernel modules.
The code inside OVS repository and net-next is not identical, but I try to keep some consistency.
Quoted 6 lines
Quoted 1 line
I didn't get why OVS_KEY_ATTR_TUNNEL_INFO cannot be exposed to userspace.
OVS_KEY_ATTR_TUNNEL_INFO is compressed version of OVS_KEY_ATTR_TUNNEL and for clarity purposes its not exposed to userspace as it will never use it.
I would say it's a coding style as it would not brake anything if exposed.
In fact, it's the best way to keep the compatibility in the long term. You can define it like this:
OVS_KEY_ATTR_TUNNEL_INFO, /* struct ip_tunnel_info, reserved for kernel use */
Quoted 11 lines
Quoted 10 lines
Quoted 8 lines
JFYI This is the file responsible for generating userspace part:
https://github.com/openvswitch/ovs/blob/master/build-aux/extract-odp-netlink-h
This is the how corresponding file for ovs_key_attr looks inside OVS:
https://github.com/openvswitch/ovs/blob/master/datapath/linux/compat/include/linux/openvswitch.h
one can see there are more values than in net-next version.
There are still some '#ifdef __KERNEL__'. The standard 'make headers_install' filters them. Why not using this standard mechanism?

Quoted 6 lines

Quoted 3 lines
Could you elaborate on this, I don't quite understand the idea!? Which ifdef you are referring, the one along OVS_KEY_ATTR_TUNNEL_INFO or some other?
My understanding is that this file is used for the userland third party, thus, theoretically, there should be no '#ifdef __KERNEL__'. uapi headers generated with 'make headers_install' are filtered to remove them.

From https://lwn.net/Articles/507794/ I understand that is the goal, but this part of the code is still used in the kernel part.

Quoted 8 lines

Quoted 8 lines
Quoted 7 lines
In this file, there are two attributes (OVS_KEY_ATTR_PACKET_TYPE and OVS_KEY_ATTR_ND_EXTENSIONS) that doesn't exist in the kernel. This will also breaks if an old app runs over a new kernel. I don't see how it is possible to keep the compat between {old|new} {kernel|app}.

Quoted 5 lines

Quoted 2 lines
Looks like this most likely is a bug while working on multiple versions of code. Need to do add more padding.
As said above, just define the same uapi for everybody and the problem is gone forever.

As this part of the code was already there, I think the correct way would be to refactor that in a separate commit if necessary.

Quoted 3 lines

Regards,
Nicolas

Best,
Tom

Ilya Maximets

maintainer

2021-10-15 13:52 UTC #9 lore raw

Quoted 58 lines · Cpp Code

Quoted 56 lines · Nicolas Dichtel
Quoted 30 lines · Cpp Code
Quoted 28 lines · Nicolas Dichtel
Quoted 26 lines · Cpp Code
Quoted 9 lines · Jakub Kicinski
Quoted 6 lines · On Wed, 29 Sep 2021 08:19:05 +0200 Nicolas Dichtel wrote:
Quoted 4 lines
/* Insert a kernel only KEY_ATTR */
#define OVS_KEY_ATTR_TUNNEL_INFO __OVS_KEY_ATTR_MAX
#undef OVS_KEY_ATTR_MAX
#define OVS_KEY_ATTR_MAX __OVS_KEY_ATTR_MAX
Following the other thread [1], this will break if a new app runs over an old kernel.
Quoted 8 lines
Good point.
Quoted 2 lines
Why not simply expose this attribute to userspace and throw an error if a userspace app uses it?
Does it matter if it's exposed or not? Either way the parsing policy for attrs coming from user space should have a reject for the value. (I say that not having looked at the code, so maybe I shouldn't...)
To remove some confusion, there are some architectural nuances if we want to extend code without large refactor.
The ovs_key_attr is defined only in kernel side. Userspace side is generated from this file. As well the code can be built without kernel modules.
The code inside OVS repository and net-next is not identical, but I try to keep some consistency.
Quoted 6 lines
Quoted 1 line
I didn't get why OVS_KEY_ATTR_TUNNEL_INFO cannot be exposed to userspace.
OVS_KEY_ATTR_TUNNEL_INFO is compressed version of OVS_KEY_ATTR_TUNNEL and for clarity purposes its not exposed to userspace as it will never use it.
I would say it's a coding style as it would not brake anything if exposed.
In fact, it's the best way to keep the compatibility in the long term. You can define it like this:
OVS_KEY_ATTR_TUNNEL_INFO, /* struct ip_tunnel_info, reserved for kernel use */
Quoted 14 lines
Quoted 10 lines
Quoted 8 lines
JFYI This is the file responsible for generating userspace part:
https://github.com/openvswitch/ovs/blob/master/build-aux/extract-odp-netlink-h
This is the how corresponding file for ovs_key_attr looks inside OVS:
https://github.com/openvswitch/ovs/blob/master/datapath/linux/compat/include/linux/openvswitch.h
one can see there are more values than in net-next version.
There are still some '#ifdef __KERNEL__'. The standard 'make headers_install' filters them. Why not using this standard mechanism?
Could you elaborate on this, I don't quite understand the idea!? Which ifdef you are referring, the one along OVS_KEY_ATTR_TUNNEL_INFO or some other?

Quoted 14 lines

Quoted 3 lines
My understanding is that this file is used for the userland third party, thus, theoretically, there should be no '#ifdef __KERNEL__'. uapi headers generated with 'make headers_install' are filtered to remove them.
From https://lwn.net/Articles/507794/ I understand that is the goal, but this part of the code is still used in the kernel part.
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
In this file, there are two attributes (OVS_KEY_ATTR_PACKET_TYPE and OVS_KEY_ATTR_ND_EXTENSIONS) that doesn't exist in the kernel. This will also breaks if an old app runs over a new kernel. I don't see how it is possible to keep the compat between {old|new} {kernel|app}.

I don't know why the initial design looked like this, but here some thoughts on why it works without noticeable issues:

OVS_KEY_ATTR_TUNNEL_INFO is defined only for kernel and not used by userspace application. If we have newer app and older kernel, new app can send a different attribute, kernel than will interpret it as OVS_KEY_ATTR_TUNNEL_INFO. This will pass the
'type > OVS_KEY_ATTR_MAX' check. However, this will always fail the check_attr_len() check, because the length for OVS_KEY_ATTR_TUNNEL_INFO is not defined in 'ovs_key_lens' and will be treated as zero, while correct attributes always has non-zero length (at least current ones has). Kind of ugly, but should work. And yes, more explicit rejection should be implemented, I think.

OVS_KEY_ATTR_PACKET_TYPE and OVS_KEY_ATTR_ND_EXTENSIONS doesn't exist in kernel and moreover not even used by the application for kernel datapath, so it's fine. And it's application's responsibility to never send them to kernel as they are not intended to be sent. So, this is in realm of the app, kernel should not care about these two attributes.
If newer kernel will send some different attributes to the old app, app will drop them as these are not expected to be ever received from a kernel (similar attribute length check as in above case with OVS_KEY_ATTR_TUNNEL_INFO). Another point is that kernel should not normally return attributes not previously set by userspace application, so this should only happen in a runtime application downgrade scenario.

All-in-all it should be safe to add new attributes before the OVS_KEY_ATTR_TUNNEL_INFO inside the kernel without introducing any new paddings. At least, as long as there are no common (valid for both kernel AND the app) attributes defined after the OVS_KEY_ATTR_TUNNEL_INFO.

But, well, I agree that current design doesn't look great. OTOH, paddings makes it even worse.

Quoted 5 lines

Quoted 5 lines
Quoted 3 lines
Looks like this most likely is a bug while working on multiple versions of code. Need to do add more padding.
As said above, just define the same uapi for everybody and the problem is gone forever.

That should be a right change to do. We can start with exposing the OVS_KEY_ATTR_TUNNEL_INFO. At the same time userspace-only attributes in the OVS codebase should, likely, be moved to a separate enum/structure to avoid confusion and keep kernel uapi clean. Though this will require some code changes on the OVS side.

Quoted 4 lines

Quoted 1 line
As this part of the code was already there, I think the correct way would be to refactor that in a separate commit if necessary.

It's OK to make this a separate change, but, please, don't add paddings in a current one. Add the new attribute before the OVS_KEY_ATTR_TUNNEL_INFO instead.

Best regards, Ilya Maximets.

Quoted 12 lines

Quoted 4 lines
Regards,
Nicolas
Best,
Tom
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Cpp Code

author

2021-09-30 16:13 UTC #10 lore raw

In reply to Jakub Kicinski

Quoted 20 lines · Jakub Kicinski

include/uapi/linux/openvswitch.h enum ovs_key_attr { · line 350
Quoted 10 lines · On Tue, 28 Sep 2021 12:47:27 -0700 Toms Atteka wrote:
include/uapi/linux/openvswitch.h +2 -0
index a87b44cd5590..dc6eb5f6399f 100644
@@ -346,6 +346,13 @@ enum ovs_key_attr {
 #ifdef __KERNEL__
      OVS_KEY_ATTR_TUNNEL_INFO,  /* struct ip_tunnel_info */
 #endif
+
+#ifndef __KERNEL__
#else
include/uapi/linux/openvswitch.h enum ovs_key_attr { · line 351
Quoted 1 line
include/uapi/linux/openvswitch.h +1 -0
+     PADDING,  /* Padding so kernel and non kernel field count would match */
The name PADDING seems rather risky, collisions will be likely. OVS_KEY_ATTR_PADDING maybe?

Quoted 10 lines

But maybe we don't need to define this special value and bake it into the uAPI, why can't we add something like this to the kernel header (i.e. include/linux/openvswitch.h):
/* Insert a kernel only KEY_ATTR */
#define OVS_KEY_ATTR_TUNNEL_INFO __OVS_KEY_ATTR_MAX
#undef OVS_KEY_ATTR_MAX
#define OVS_KEY_ATTR_MAX __OVS_KEY_ATTR_MAX
Quoted 1 line
include/uapi/linux/openvswitch.h +1 -0
+#endif

Agree, name should be changed, I think I will go with __OVS_KEY_ATTR_PADDING