All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH net-next v8] net: openvswitch: IPv6: Add IPv6 extension header support
@ 2022-02-24  0:54 Toms Atteka
  2022-02-25 10:40 ` patchwork-bot+netdevbpf
  0 siblings, 1 reply; 22+ messages in thread
From: Toms Atteka @ 2022-02-24  0:54 UTC (permalink / raw)
  To: netdev, pshelar, davem, kuba, dev, linux-kernel; +Cc: Toms Atteka

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 <cpp.code.lv@gmail.com>
Acked-by: Pravin B Shelar <pshelar@ovn.org>
---
 include/uapi/linux/openvswitch.h |   6 ++
 net/openvswitch/flow.c           | 140 +++++++++++++++++++++++++++++++
 net/openvswitch/flow.h           |  14 ++++
 net/openvswitch/flow_netlink.c   |  26 +++++-
 4 files changed, 184 insertions(+), 2 deletions(-)

diff --git a/include/uapi/linux/openvswitch.h b/include/uapi/linux/openvswitch.h
index 150bcff49b1c..9d1710f20505 100644
--- a/include/uapi/linux/openvswitch.h
+++ b/include/uapi/linux/openvswitch.h
@@ -351,6 +351,7 @@ enum ovs_key_attr {
 	OVS_KEY_ATTR_CT_ORIG_TUPLE_IPV4,   /* struct ovs_key_ct_tuple_ipv4 */
 	OVS_KEY_ATTR_CT_ORIG_TUPLE_IPV6,   /* struct ovs_key_ct_tuple_ipv6 */
 	OVS_KEY_ATTR_NSH,       /* Nested set of ovs_nsh_key_* */
+	OVS_KEY_ATTR_IPV6_EXTHDRS,  /* struct ovs_key_ipv6_exthdr */
 
 #ifdef __KERNEL__
 	OVS_KEY_ATTR_TUNNEL_INFO,  /* struct ip_tunnel_info */
@@ -430,6 +431,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;
diff --git a/net/openvswitch/flow.c b/net/openvswitch/flow.c
index f6cd24fd530c..8df73d86b968 100644
--- a/net/openvswitch/flow.c
+++ b/net/openvswitch/flow.c
@@ -241,6 +241,144 @@ static bool icmphdr_ok(struct sk_buff *skb)
 				  sizeof(struct icmphdr));
 }
 
+/**
+ * get_ipv6_ext_hdrs() - Parses packet and sets IPv6 extension header flags.
+ *
+ * @skb: buffer where extension header data starts in packet
+ * @nh: ipv6 header
+ * @ext_hdrs: flags are stored here
+ *
+ * OFPIEH12_UNREP is set if more than one of a given IPv6 extension header
+ * is unexpectedly encountered. (Two destination options headers may be
+ * expected and would not cause this bit to be set.)
+ *
+ * OFPIEH12_UNSEQ is set if IPv6 extension headers were not in the order
+ * preferred (but not required) by RFC 2460:
+ *
+ * When more than one extension header is used in the same packet, it is
+ * recommended that those headers appear in the following order:
+ *      IPv6 header
+ *      Hop-by-Hop Options header
+ *      Destination Options header
+ *      Routing header
+ *      Fragment header
+ *      Authentication header
+ *      Encapsulating Security Payload header
+ *      Destination Options header
+ *      upper-layer header
+ */
+static void get_ipv6_ext_hdrs(struct sk_buff *skb, struct ipv6hdr *nh,
+			      u16 *ext_hdrs)
+{
+	u8 next_type = nh->nexthdr;
+	unsigned int start = skb_network_offset(skb) + sizeof(struct ipv6hdr);
+	int dest_options_header_count = 0;
+
+	*ext_hdrs = 0;
+
+	while (ipv6_ext_hdr(next_type)) {
+		struct ipv6_opt_hdr _hdr, *hp;
+
+		switch (next_type) {
+		case IPPROTO_NONE:
+			*ext_hdrs |= OFPIEH12_NONEXT;
+			/* stop parsing */
+			return;
+
+		case IPPROTO_ESP:
+			if (*ext_hdrs & OFPIEH12_ESP)
+				*ext_hdrs |= OFPIEH12_UNREP;
+			if ((*ext_hdrs & ~(OFPIEH12_HOP | OFPIEH12_DEST |
+					   OFPIEH12_ROUTER | IPPROTO_FRAGMENT |
+					   OFPIEH12_AUTH | OFPIEH12_UNREP)) ||
+			    dest_options_header_count >= 2) {
+				*ext_hdrs |= OFPIEH12_UNSEQ;
+			}
+			*ext_hdrs |= OFPIEH12_ESP;
+			break;
+
+		case IPPROTO_AH:
+			if (*ext_hdrs & OFPIEH12_AUTH)
+				*ext_hdrs |= OFPIEH12_UNREP;
+			if ((*ext_hdrs &
+			     ~(OFPIEH12_HOP | OFPIEH12_DEST | OFPIEH12_ROUTER |
+			       IPPROTO_FRAGMENT | OFPIEH12_UNREP)) ||
+			    dest_options_header_count >= 2) {
+				*ext_hdrs |= OFPIEH12_UNSEQ;
+			}
+			*ext_hdrs |= OFPIEH12_AUTH;
+			break;
+
+		case IPPROTO_DSTOPTS:
+			if (dest_options_header_count == 0) {
+				if (*ext_hdrs &
+				    ~(OFPIEH12_HOP | OFPIEH12_UNREP))
+					*ext_hdrs |= OFPIEH12_UNSEQ;
+				*ext_hdrs |= OFPIEH12_DEST;
+			} else if (dest_options_header_count == 1) {
+				if (*ext_hdrs &
+				    ~(OFPIEH12_HOP | OFPIEH12_DEST |
+				      OFPIEH12_ROUTER | OFPIEH12_FRAG |
+				      OFPIEH12_AUTH | OFPIEH12_ESP |
+				      OFPIEH12_UNREP)) {
+					*ext_hdrs |= OFPIEH12_UNSEQ;
+				}
+			} else {
+				*ext_hdrs |= OFPIEH12_UNREP;
+			}
+			dest_options_header_count++;
+			break;
+
+		case IPPROTO_FRAGMENT:
+			if (*ext_hdrs & OFPIEH12_FRAG)
+				*ext_hdrs |= OFPIEH12_UNREP;
+			if ((*ext_hdrs & ~(OFPIEH12_HOP |
+					   OFPIEH12_DEST |
+					   OFPIEH12_ROUTER |
+					   OFPIEH12_UNREP)) ||
+			    dest_options_header_count >= 2) {
+				*ext_hdrs |= OFPIEH12_UNSEQ;
+			}
+			*ext_hdrs |= OFPIEH12_FRAG;
+			break;
+
+		case IPPROTO_ROUTING:
+			if (*ext_hdrs & OFPIEH12_ROUTER)
+				*ext_hdrs |= OFPIEH12_UNREP;
+			if ((*ext_hdrs & ~(OFPIEH12_HOP |
+					   OFPIEH12_DEST |
+					   OFPIEH12_UNREP)) ||
+			    dest_options_header_count >= 2) {
+				*ext_hdrs |= OFPIEH12_UNSEQ;
+			}
+			*ext_hdrs |= OFPIEH12_ROUTER;
+			break;
+
+		case IPPROTO_HOPOPTS:
+			if (*ext_hdrs & OFPIEH12_HOP)
+				*ext_hdrs |= OFPIEH12_UNREP;
+			/* OFPIEH12_HOP is set to 1 if a hop-by-hop IPv6
+			 * extension header is present as the first
+			 * extension header in the packet.
+			 */
+			if (*ext_hdrs == 0)
+				*ext_hdrs |= OFPIEH12_HOP;
+			else
+				*ext_hdrs |= OFPIEH12_UNSEQ;
+			break;
+
+		default:
+			return;
+		}
+
+		hp = skb_header_pointer(skb, start, sizeof(_hdr), &_hdr);
+		if (!hp)
+			break;
+		next_type = hp->nexthdr;
+		start += ipv6_optlen(hp);
+	};
+}
+
 static int parse_ipv6hdr(struct sk_buff *skb, struct sw_flow_key *key)
 {
 	unsigned short frag_off;
@@ -256,6 +394,8 @@ static int parse_ipv6hdr(struct sk_buff *skb, struct sw_flow_key *key)
 
 	nh = ipv6_hdr(skb);
 
+	get_ipv6_ext_hdrs(skb, nh, &key->ipv6.exthdrs);
+
 	key->ip.proto = NEXTHDR_NONE;
 	key->ip.tos = ipv6_get_dsfield(nh);
 	key->ip.ttl = nh->hop_limit;
diff --git a/net/openvswitch/flow.h b/net/openvswitch/flow.h
index 758a8c77f736..073ab73ffeaa 100644
--- a/net/openvswitch/flow.h
+++ b/net/openvswitch/flow.h
@@ -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;
diff --git a/net/openvswitch/flow_netlink.c b/net/openvswitch/flow_netlink.c
index fd1f809e9bc1..8b4124820f7d 100644
--- a/net/openvswitch/flow_netlink.c
+++ b/net/openvswitch/flow_netlink.c
@@ -346,7 +346,7 @@ size_t ovs_key_attr_size(void)
 	/* Whenever adding new OVS_KEY_ FIELDS, we should consider
 	 * updating this function.
 	 */
-	BUILD_BUG_ON(OVS_KEY_ATTR_TUNNEL_INFO != 29);
+	BUILD_BUG_ON(OVS_KEY_ATTR_TUNNEL_INFO != 30);
 
 	return    nla_total_size(4)   /* OVS_KEY_ATTR_PRIORITY */
 		+ nla_total_size(0)   /* OVS_KEY_ATTR_TUNNEL */
@@ -369,7 +369,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] = {
@@ -437,6 +438,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)
@@ -1597,6 +1600,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;
 
@@ -2099,6 +2113,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)
@@ -2113,6 +2128,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;
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 22+ messages in thread

* Re: [PATCH net-next v8] net: openvswitch: IPv6: Add IPv6 extension header support
  2022-02-24  0:54 [PATCH net-next v8] net: openvswitch: IPv6: Add IPv6 extension header support Toms Atteka
@ 2022-02-25 10:40 ` patchwork-bot+netdevbpf
  2022-03-02 10:03   ` [ovs-dev] " Roi Dayan
  0 siblings, 1 reply; 22+ messages in thread
From: patchwork-bot+netdevbpf @ 2022-02-25 10:40 UTC (permalink / raw)
  To: Toms Atteka; +Cc: netdev, pshelar, davem, kuba, dev, linux-kernel

Hello:

This patch was applied to netdev/net-next.git (master)
by David S. Miller <davem@davemloft.net>:

On Wed, 23 Feb 2022 16:54:09 -0800 you wrote:
> 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 <cpp.code.lv@gmail.com>
> Acked-by: Pravin B Shelar <pshelar@ovn.org>
> ---
>  include/uapi/linux/openvswitch.h |   6 ++
>  net/openvswitch/flow.c           | 140 +++++++++++++++++++++++++++++++
>  net/openvswitch/flow.h           |  14 ++++
>  net/openvswitch/flow_netlink.c   |  26 +++++-
>  4 files changed, 184 insertions(+), 2 deletions(-)

Here is the summary with links:
  - [net-next,v8] net: openvswitch: IPv6: Add IPv6 extension header support
    https://git.kernel.org/netdev/net-next/c/28a3f0601727

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html



^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [ovs-dev] [PATCH net-next v8] net: openvswitch: IPv6: Add IPv6 extension header support
  2022-02-25 10:40 ` patchwork-bot+netdevbpf
@ 2022-03-02 10:03   ` Roi Dayan
  2022-03-02 10:50     ` Roi Dayan
  0 siblings, 1 reply; 22+ messages in thread
From: Roi Dayan @ 2022-03-02 10:03 UTC (permalink / raw)
  To: patchwork-bot+netdevbpf, Toms Atteka
  Cc: dev, netdev, linux-kernel, kuba, davem



On 2022-02-25 12:40 PM, patchwork-bot+netdevbpf@kernel.org wrote:
> Hello:
> 
> This patch was applied to netdev/net-next.git (master)
> by David S. Miller <davem@davemloft.net>:
> 
> On Wed, 23 Feb 2022 16:54:09 -0800 you wrote:
>> 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 <cpp.code.lv@gmail.com>
>> Acked-by: Pravin B Shelar <pshelar@ovn.org>
>> ---
>>   include/uapi/linux/openvswitch.h |   6 ++
>>   net/openvswitch/flow.c           | 140 +++++++++++++++++++++++++++++++
>>   net/openvswitch/flow.h           |  14 ++++
>>   net/openvswitch/flow_netlink.c   |  26 +++++-
>>   4 files changed, 184 insertions(+), 2 deletions(-)
> 
> Here is the summary with links:
>    - [net-next,v8] net: openvswitch: IPv6: Add IPv6 extension header support
>      https://git.kernel.org/netdev/net-next/c/28a3f0601727
> 
> You are awesome, thank you!

Hi,

After the merge of this patch I fail to do ipv6 traffic in ovs.
Am I missing something?

ovs-vswitchd.log has this msg

2022-03-02T09:52:26.604Z|00013|odp_util(handler1)|WARN|attribute 
packet_type has length 2 but should have length 4

Thanks,
Roi

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [ovs-dev] [PATCH net-next v8] net: openvswitch: IPv6: Add IPv6 extension header support
  2022-03-02 10:03   ` [ovs-dev] " Roi Dayan
@ 2022-03-02 10:50     ` Roi Dayan
  2022-03-02 13:59       ` Eelco Chaudron
  0 siblings, 1 reply; 22+ messages in thread
From: Roi Dayan @ 2022-03-02 10:50 UTC (permalink / raw)
  To: patchwork-bot+netdevbpf, Toms Atteka
  Cc: dev, netdev, linux-kernel, kuba, davem



On 2022-03-02 12:03 PM, Roi Dayan wrote:
> 
> 
> On 2022-02-25 12:40 PM, patchwork-bot+netdevbpf@kernel.org wrote:
>> Hello:
>>
>> This patch was applied to netdev/net-next.git (master)
>> by David S. Miller <davem@davemloft.net>:
>>
>> On Wed, 23 Feb 2022 16:54:09 -0800 you wrote:
>>> 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 <cpp.code.lv@gmail.com>
>>> Acked-by: Pravin B Shelar <pshelar@ovn.org>
>>> ---
>>>   include/uapi/linux/openvswitch.h |   6 ++
>>>   net/openvswitch/flow.c           | 140 +++++++++++++++++++++++++++++++
>>>   net/openvswitch/flow.h           |  14 ++++
>>>   net/openvswitch/flow_netlink.c   |  26 +++++-
>>>   4 files changed, 184 insertions(+), 2 deletions(-)
>>
>> Here is the summary with links:
>>    - [net-next,v8] net: openvswitch: IPv6: Add IPv6 extension header 
>> support
>>      https://git.kernel.org/netdev/net-next/c/28a3f0601727
>>
>> You are awesome, thank you!
> 
> Hi,
> 
> After the merge of this patch I fail to do ipv6 traffic in ovs.
> Am I missing something?
> 
> ovs-vswitchd.log has this msg
> 
> 2022-03-02T09:52:26.604Z|00013|odp_util(handler1)|WARN|attribute 
> packet_type has length 2 but should have length 4
> 
> Thanks,
> Roi


I think there is a missing userspace fix. didnt verify yet.
but in ovs userspace odp-netlink.h created from 
datapath/linux/compat/include/linux/openvswitch.h
and that file is not synced the change here.
So the new enum OVS_KEY_ATTR_IPV6_EXTHDRS is missing and also struct
ovs_key_ipv6_exthdrs which is needed in lib/udp-util.c
in struct ovs_flow_key_attr_lens to add expected len for
OVS_KEY_ATTR_IPV6_EXTHDR.


^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [ovs-dev] [PATCH net-next v8] net: openvswitch: IPv6: Add IPv6 extension header support
  2022-03-02 10:50     ` Roi Dayan
@ 2022-03-02 13:59       ` Eelco Chaudron
  2022-03-07  8:49         ` Roi Dayan
  0 siblings, 1 reply; 22+ messages in thread
From: Eelco Chaudron @ 2022-03-02 13:59 UTC (permalink / raw)
  To: Roi Dayan
  Cc: patchwork-bot+netdevbpf, Toms Atteka, dev, netdev, linux-kernel,
	kuba, davem



On 2 Mar 2022, at 11:50, Roi Dayan wrote:

> On 2022-03-02 12:03 PM, Roi Dayan wrote:
>>
>>
>> On 2022-02-25 12:40 PM, patchwork-bot+netdevbpf@kernel.org wrote:
>>> Hello:
>>>
>>> This patch was applied to netdev/net-next.git (master)
>>> by David S. Miller <davem@davemloft.net>:
>>>
>>> On Wed, 23 Feb 2022 16:54:09 -0800 you wrote:
>>>> 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 <cpp.code.lv@gmail.com>
>>>> Acked-by: Pravin B Shelar <pshelar@ovn.org>
>>>> ---
>>>>   include/uapi/linux/openvswitch.h |   6 ++
>>>>   net/openvswitch/flow.c           | 140 +++++++++++++++++++++++++++++++
>>>>   net/openvswitch/flow.h           |  14 ++++
>>>>   net/openvswitch/flow_netlink.c   |  26 +++++-
>>>>   4 files changed, 184 insertions(+), 2 deletions(-)
>>>
>>> Here is the summary with links:
>>>    - [net-next,v8] net: openvswitch: IPv6: Add IPv6 extension header support
>>>      https://git.kernel.org/netdev/net-next/c/28a3f0601727
>>>
>>> You are awesome, thank you!
>>
>> Hi,
>>
>> After the merge of this patch I fail to do ipv6 traffic in ovs.
>> Am I missing something?
>>
>> ovs-vswitchd.log has this msg
>>
>> 2022-03-02T09:52:26.604Z|00013|odp_util(handler1)|WARN|attribute packet_type has length 2 but should have length 4
>>
>> Thanks,
>> Roi
>
>
> I think there is a missing userspace fix. didnt verify yet.
> but in ovs userspace odp-netlink.h created from datapath/linux/compat/include/linux/openvswitch.h
> and that file is not synced the change here.
> So the new enum OVS_KEY_ATTR_IPV6_EXTHDRS is missing and also struct
> ovs_key_ipv6_exthdrs which is needed in lib/udp-util.c
> in struct ovs_flow_key_attr_lens to add expected len for
> OVS_KEY_ATTR_IPV6_EXTHDR.

I guess if this is creating backward compatibility issues, this patch should be reverted/fixed. As a kmod upgrade should not break existing deployments.


^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [ovs-dev] [PATCH net-next v8] net: openvswitch: IPv6: Add IPv6 extension header support
  2022-03-02 13:59       ` Eelco Chaudron
@ 2022-03-07  8:49         ` Roi Dayan
  2022-03-07 20:26           ` Jakub Kicinski
  0 siblings, 1 reply; 22+ messages in thread
From: Roi Dayan @ 2022-03-07  8:49 UTC (permalink / raw)
  To: Eelco Chaudron
  Cc: patchwork-bot+netdevbpf, Toms Atteka, dev, netdev, linux-kernel,
	kuba, davem



On 2022-03-02 3:59 PM, Eelco Chaudron wrote:
> 
> 
> On 2 Mar 2022, at 11:50, Roi Dayan wrote:
> 
>> On 2022-03-02 12:03 PM, Roi Dayan wrote:
>>>
>>>
>>> On 2022-02-25 12:40 PM, patchwork-bot+netdevbpf@kernel.org wrote:
>>>> Hello:
>>>>
>>>> This patch was applied to netdev/net-next.git (master)
>>>> by David S. Miller <davem@davemloft.net>:
>>>>
>>>> On Wed, 23 Feb 2022 16:54:09 -0800 you wrote:
>>>>> 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 <cpp.code.lv@gmail.com>
>>>>> Acked-by: Pravin B Shelar <pshelar@ovn.org>
>>>>> ---
>>>>>    include/uapi/linux/openvswitch.h |   6 ++
>>>>>    net/openvswitch/flow.c           | 140 +++++++++++++++++++++++++++++++
>>>>>    net/openvswitch/flow.h           |  14 ++++
>>>>>    net/openvswitch/flow_netlink.c   |  26 +++++-
>>>>>    4 files changed, 184 insertions(+), 2 deletions(-)
>>>>
>>>> Here is the summary with links:
>>>>     - [net-next,v8] net: openvswitch: IPv6: Add IPv6 extension header support
>>>>       https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgit.kernel.org%2Fnetdev%2Fnet-next%2Fc%2F28a3f0601727&amp;data=04%7C01%7Croid%40nvidia.com%7C41e35b6733e34dcf628708d9fc54dc9b%7C43083d15727340c1b7db39efd9ccc17a%7C0%7C0%7C637818263683241778%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&amp;sdata=iaysgYLANYFQo87ELcK9KMesF6u7pWF3cGAHiKB%2FM1E%3D&amp;reserved=0
>>>>
>>>> You are awesome, thank you!
>>>
>>> Hi,
>>>
>>> After the merge of this patch I fail to do ipv6 traffic in ovs.
>>> Am I missing something?
>>>
>>> ovs-vswitchd.log has this msg
>>>
>>> 2022-03-02T09:52:26.604Z|00013|odp_util(handler1)|WARN|attribute packet_type has length 2 but should have length 4
>>>
>>> Thanks,
>>> Roi
>>
>>
>> I think there is a missing userspace fix. didnt verify yet.
>> but in ovs userspace odp-netlink.h created from datapath/linux/compat/include/linux/openvswitch.h
>> and that file is not synced the change here.
>> So the new enum OVS_KEY_ATTR_IPV6_EXTHDRS is missing and also struct
>> ovs_key_ipv6_exthdrs which is needed in lib/udp-util.c
>> in struct ovs_flow_key_attr_lens to add expected len for
>> OVS_KEY_ATTR_IPV6_EXTHDR.
> 
> I guess if this is creating backward compatibility issues, this patch should be reverted/fixed. As a kmod upgrade should not break existing deployments.
> 

it looks like it does. we can't work with ovs without reverting this.
can we continue with reverting this commit please?

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [ovs-dev] [PATCH net-next v8] net: openvswitch: IPv6: Add IPv6 extension header support
  2022-03-07  8:49         ` Roi Dayan
@ 2022-03-07 20:26           ` Jakub Kicinski
  2022-03-07 22:14             ` Ilya Maximets
  0 siblings, 1 reply; 22+ messages in thread
From: Jakub Kicinski @ 2022-03-07 20:26 UTC (permalink / raw)
  To: Roi Dayan
  Cc: Eelco Chaudron, patchwork-bot+netdevbpf, Toms Atteka, dev,
	netdev, linux-kernel, davem

On Mon, 7 Mar 2022 10:49:31 +0200 Roi Dayan wrote:
> >> I think there is a missing userspace fix. didnt verify yet.
> >> but in ovs userspace odp-netlink.h created from datapath/linux/compat/include/linux/openvswitch.h
> >> and that file is not synced the change here.
> >> So the new enum OVS_KEY_ATTR_IPV6_EXTHDRS is missing and also struct
> >> ovs_key_ipv6_exthdrs which is needed in lib/udp-util.c
> >> in struct ovs_flow_key_attr_lens to add expected len for
> >> OVS_KEY_ATTR_IPV6_EXTHDR.  
> > 
> > I guess if this is creating backward compatibility issues, this
> > patch should be reverted/fixed. As a kmod upgrade should not break
> > existing deployments. 
> 
> it looks like it does. we can't work with ovs without reverting this.
> can we continue with reverting this commit please?

Sure, can someone ELI5 what the problem is?

What's "kmod upgrade" in this context a kernel upgrade or loading 
a newer module in older kernel? 

How can adding a new nl attr break user space? Does the user space
actually care about the OVS_KEY_ATTR_TUNNEL_INFO wart?

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [ovs-dev] [PATCH net-next v8] net: openvswitch: IPv6: Add IPv6 extension header support
  2022-03-07 20:26           ` Jakub Kicinski
@ 2022-03-07 22:14             ` Ilya Maximets
  2022-03-07 22:46               ` Jakub Kicinski
  0 siblings, 1 reply; 22+ messages in thread
From: Ilya Maximets @ 2022-03-07 22:14 UTC (permalink / raw)
  To: Jakub Kicinski, Roi Dayan
  Cc: dev, Toms Atteka, netdev, linux-kernel, davem, i.maximets

On 3/7/22 21:26, Jakub Kicinski wrote:
> On Mon, 7 Mar 2022 10:49:31 +0200 Roi Dayan wrote:
>>>> I think there is a missing userspace fix. didnt verify yet.
>>>> but in ovs userspace odp-netlink.h created from datapath/linux/compat/include/linux/openvswitch.h
>>>> and that file is not synced the change here.
>>>> So the new enum OVS_KEY_ATTR_IPV6_EXTHDRS is missing and also struct
>>>> ovs_key_ipv6_exthdrs which is needed in lib/udp-util.c
>>>> in struct ovs_flow_key_attr_lens to add expected len for
>>>> OVS_KEY_ATTR_IPV6_EXTHDR.  
>>>
>>> I guess if this is creating backward compatibility issues, this
>>> patch should be reverted/fixed. As a kmod upgrade should not break
>>> existing deployments. 
>>
>> it looks like it does. we can't work with ovs without reverting this.
>> can we continue with reverting this commit please?
> 
> Sure, can someone ELI5 what the problem is?
> 
> What's "kmod upgrade" in this context a kernel upgrade or loading 
> a newer module in older kernel? 
> 
> How can adding a new nl attr break user space? Does the user space
> actually care about the OVS_KEY_ATTR_TUNNEL_INFO wart?

Hi, Jakub.

The main problem is that userspace uses the modified copy of the uapi header
which looks like this:
  https://github.com/openvswitch/ovs/blob/f77dbc1eb2da2523625cd36922c6fccfcb3f3eb7/datapath/linux/compat/include/linux/openvswitch.h#L357

In short, the userspace view:

  enum ovs_key_attr {
      <common attrs>

  #ifdef __KERNEL__
      /* Only used within kernel data path. */
  #endif

  #ifndef __KERNEL__
      /* Only used within userspace data path. */
  #endif
      __OVS_KEY_ATTR_MAX
};

And the kernel view:

  enum ovs_key_attr {
      <common attrs>

  #ifdef __KERNEL__
      /* Only used within kernel data path. */
  #endif

      __OVS_KEY_ATTR_MAX
  };

This happened before my time, but the commit where userspace made a wrong
turn appears to be this one:
  https://github.com/openvswitch/ovs/commit/beb75a40fdc295bfd6521b0068b4cd12f6de507c
The attribute for userspace only was added to the common enum after the
OVS_KEY_ATTR_TUNNEL_INFO.   I'm not sure how things didn't fall apart when
OVS_KEY_ATTR_NSH was added later (no-one cared that NSH doesn't work, because
OVS didn't support it yet?).

In general, any addition of a new attribute into that enumeration leads to
inevitable clash between userpsace-only attributes and new kernel attributes.

After the kernel update, kernel provides new attributes to the userspace and
userspace tries to parse them as one of the userspace-only attributes and
fails.   In our current case userspace is trying to parse OVS_KEY_ATTR_IPV6_EXTHDR
as userspace-only OVS_KEY_ATTR_PACKET_TYPE, because they have the same value in the
enum, fails and discards the netlink message as malformed.  So, IPv6 is fully
broken, because OVS_KEY_ATTR_IPV6_EXTHDR is supplied now with every IPv6 packet
that goes to userspace.

We need to unify the view of 'enum ovs_key_attr' between userspace and kernel
before we can add any new values to it.

One way to do that should be addition of both userspace-only attributes to the
kernel header (and maybe exposing OVS_KEY_ATTR_TUNNEL_INFO too, just to keep
it flat and avoid any possible problems in the future).  Any other suggestions
are welcome.  But in any case this will require careful testing with existing
OVS userspace to avoid any unexpected issues.

Moving forward, I think, userspace OVS should find a way to not have userpsace-only
attributes, or have them as a separate enumeration.  But I'm not sure how to do
that right now.  Or we'll have to add userspace-only attributes to the kernel
uapi before using them.

Sorry for the mess.

Best regards, Ilya Maximets.

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [ovs-dev] [PATCH net-next v8] net: openvswitch: IPv6: Add IPv6 extension header support
  2022-03-07 22:14             ` Ilya Maximets
@ 2022-03-07 22:46               ` Jakub Kicinski
  2022-03-08  0:04                 ` Ilya Maximets
  0 siblings, 1 reply; 22+ messages in thread
From: Jakub Kicinski @ 2022-03-07 22:46 UTC (permalink / raw)
  To: Ilya Maximets; +Cc: Roi Dayan, dev, Toms Atteka, netdev, linux-kernel, davem

On Mon, 7 Mar 2022 23:14:13 +0100 Ilya Maximets wrote:
> The main problem is that userspace uses the modified copy of the uapi header
> which looks like this:
>   https://github.com/openvswitch/ovs/blob/f77dbc1eb2da2523625cd36922c6fccfcb3f3eb7/datapath/linux/compat/include/linux/openvswitch.h#L357
> 
> In short, the userspace view:
> 
>   enum ovs_key_attr {
>       <common attrs>
> 
>   #ifdef __KERNEL__
>       /* Only used within kernel data path. */
>   #endif
> 
>   #ifndef __KERNEL__
>       /* Only used within userspace data path. */
>   #endif
>       __OVS_KEY_ATTR_MAX
> };
> 
> And the kernel view:
> 
>   enum ovs_key_attr {
>       <common attrs>
> 
>   #ifdef __KERNEL__
>       /* Only used within kernel data path. */
>   #endif
> 
>       __OVS_KEY_ATTR_MAX
>   };
> 
> This happened before my time, but the commit where userspace made a wrong
> turn appears to be this one:
>   https://github.com/openvswitch/ovs/commit/beb75a40fdc295bfd6521b0068b4cd12f6de507c
> The attribute for userspace only was added to the common enum after the
> OVS_KEY_ATTR_TUNNEL_INFO.   I'm not sure how things didn't fall apart when
> OVS_KEY_ATTR_NSH was added later (no-one cared that NSH doesn't work, because
> OVS didn't support it yet?).
> 
> In general, any addition of a new attribute into that enumeration leads to
> inevitable clash between userpsace-only attributes and new kernel attributes.
> 
> After the kernel update, kernel provides new attributes to the userspace and
> userspace tries to parse them as one of the userspace-only attributes and
> fails.   In our current case userspace is trying to parse OVS_KEY_ATTR_IPV6_EXTHDR
> as userspace-only OVS_KEY_ATTR_PACKET_TYPE, because they have the same value in the
> enum, fails and discards the netlink message as malformed.  So, IPv6 is fully
> broken, because OVS_KEY_ATTR_IPV6_EXTHDR is supplied now with every IPv6 packet
> that goes to userspace.
> 
> We need to unify the view of 'enum ovs_key_attr' between userspace and kernel
> before we can add any new values to it.
> 
> One way to do that should be addition of both userspace-only attributes to the
> kernel header (and maybe exposing OVS_KEY_ATTR_TUNNEL_INFO too, just to keep
> it flat and avoid any possible problems in the future).  Any other suggestions
> are welcome.  But in any case this will require careful testing with existing
> OVS userspace to avoid any unexpected issues.
> 
> Moving forward, I think, userspace OVS should find a way to not have userpsace-only
> attributes, or have them as a separate enumeration.  But I'm not sure how to do
> that right now.  Or we'll have to add userspace-only attributes to the kernel
> uapi before using them.

Thanks for the explanation, we can apply a revert if that'd help your
CI / ongoing development but sounds like the fix really is in user
space. Expecting netlink attribute lists not to grow is not fair.

Since ovs uses genetlink you should be able to dump the policy from 
the kernel and at least validate that it doesn't overlap.

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [ovs-dev] [PATCH net-next v8] net: openvswitch: IPv6: Add IPv6 extension header support
  2022-03-07 22:46               ` Jakub Kicinski
@ 2022-03-08  0:04                 ` Ilya Maximets
  2022-03-08  5:45                   ` Jakub Kicinski
  0 siblings, 1 reply; 22+ messages in thread
From: Ilya Maximets @ 2022-03-08  0:04 UTC (permalink / raw)
  To: Jakub Kicinski
  Cc: i.maximets, Roi Dayan, dev, Toms Atteka, netdev, linux-kernel, davem

On 3/7/22 23:46, Jakub Kicinski wrote:
> On Mon, 7 Mar 2022 23:14:13 +0100 Ilya Maximets wrote:
>> The main problem is that userspace uses the modified copy of the uapi header
>> which looks like this:
>>   https://github.com/openvswitch/ovs/blob/f77dbc1eb2da2523625cd36922c6fccfcb3f3eb7/datapath/linux/compat/include/linux/openvswitch.h#L357
>>
>> In short, the userspace view:
>>
>>   enum ovs_key_attr {
>>       <common attrs>
>>
>>   #ifdef __KERNEL__
>>       /* Only used within kernel data path. */
>>   #endif
>>
>>   #ifndef __KERNEL__
>>       /* Only used within userspace data path. */
>>   #endif
>>       __OVS_KEY_ATTR_MAX
>> };
>>
>> And the kernel view:
>>
>>   enum ovs_key_attr {
>>       <common attrs>
>>
>>   #ifdef __KERNEL__
>>       /* Only used within kernel data path. */
>>   #endif
>>
>>       __OVS_KEY_ATTR_MAX
>>   };
>>
>> This happened before my time, but the commit where userspace made a wrong
>> turn appears to be this one:
>>   https://github.com/openvswitch/ovs/commit/beb75a40fdc295bfd6521b0068b4cd12f6de507c
>> The attribute for userspace only was added to the common enum after the
>> OVS_KEY_ATTR_TUNNEL_INFO.   I'm not sure how things didn't fall apart when
>> OVS_KEY_ATTR_NSH was added later (no-one cared that NSH doesn't work, because
>> OVS didn't support it yet?).
>>
>> In general, any addition of a new attribute into that enumeration leads to
>> inevitable clash between userpsace-only attributes and new kernel attributes.
>>
>> After the kernel update, kernel provides new attributes to the userspace and
>> userspace tries to parse them as one of the userspace-only attributes and
>> fails.   In our current case userspace is trying to parse OVS_KEY_ATTR_IPV6_EXTHDR
>> as userspace-only OVS_KEY_ATTR_PACKET_TYPE, because they have the same value in the
>> enum, fails and discards the netlink message as malformed.  So, IPv6 is fully
>> broken, because OVS_KEY_ATTR_IPV6_EXTHDR is supplied now with every IPv6 packet
>> that goes to userspace.
>>
>> We need to unify the view of 'enum ovs_key_attr' between userspace and kernel
>> before we can add any new values to it.
>>
>> One way to do that should be addition of both userspace-only attributes to the
>> kernel header (and maybe exposing OVS_KEY_ATTR_TUNNEL_INFO too, just to keep
>> it flat and avoid any possible problems in the future).  Any other suggestions
>> are welcome.  But in any case this will require careful testing with existing
>> OVS userspace to avoid any unexpected issues.
>>
>> Moving forward, I think, userspace OVS should find a way to not have userpsace-only
>> attributes, or have them as a separate enumeration.  But I'm not sure how to do
>> that right now.  Or we'll have to add userspace-only attributes to the kernel
>> uapi before using them.
> 
> Thanks for the explanation, we can apply a revert if that'd help your
> CI / ongoing development but sounds like the fix really is in user
> space. Expecting netlink attribute lists not to grow is not fair.

I don't think it was intentional, just a careless mistake.  Unfortunately,
all OVS binaries built during the last 5 years rely on that unwanted
expectation (re-build will also not help as they are using a copy of the
uAPI header and the clash will be there anyway).  If we want to keep them
working, kernel uAPI has to be carefully updated with current userspace-only
attributes before we add any new ones.  That is not great, but I don't see
any other option right now that doesn't require code changes in userspace.

I'd say that we need to revert the current patch and re-introduce it
later when the uAPI problem is sorted out.  This way we will avoid blocking
the net-next testing and will also avoid problems in case the uAPI changes
are not ready at the moment of the new kernel release.

What do you think?

> 
> Since ovs uses genetlink you should be able to dump the policy from 
> the kernel and at least validate that it doesn't overlap.

That is interesting.  Indeed, this functionality can be used to detect
problems or to define userspace-only attributes in runtime based on the
kernel reply.  Thanks for the pointer!

Best regards, Ilya Maximets.

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [ovs-dev] [PATCH net-next v8] net: openvswitch: IPv6: Add IPv6 extension header support
  2022-03-08  0:04                 ` Ilya Maximets
@ 2022-03-08  5:45                   ` Jakub Kicinski
  2022-03-08  8:21                     ` Johannes Berg
  0 siblings, 1 reply; 22+ messages in thread
From: Jakub Kicinski @ 2022-03-08  5:45 UTC (permalink / raw)
  To: Ilya Maximets
  Cc: Roi Dayan, dev, Toms Atteka, netdev, linux-kernel, davem,
	David Ahern, Johannes Berg, Jiri Pirko, Pablo Neira Ayuso

On Tue, 8 Mar 2022 01:04:00 +0100 Ilya Maximets wrote:
> > Thanks for the explanation, we can apply a revert if that'd help your
> > CI / ongoing development but sounds like the fix really is in user
> > space. Expecting netlink attribute lists not to grow is not fair.  
> 
> I don't think it was intentional, just a careless mistake.  Unfortunately,
> all OVS binaries built during the last 5 years rely on that unwanted
> expectation (re-build will also not help as they are using a copy of the
> uAPI header and the clash will be there anyway).  If we want to keep them
> working, kernel uAPI has to be carefully updated with current userspace-only
> attributes before we add any new ones.  That is not great, but I don't see
> any other option right now that doesn't require code changes in userspace.
> 
> I'd say that we need to revert the current patch and re-introduce it
> later when the uAPI problem is sorted out.  This way we will avoid blocking
> the net-next testing and will also avoid problems in case the uAPI changes
> are not ready at the moment of the new kernel release.
> 
> What do you think?

Let me add some people I associate with genetlink work in my head
(fairly or not) to keep me fair here.

It's highly unacceptable for user space to straight up rewrite kernel
uAPI types but if it already happened the only fix is something like:

diff --git a/include/uapi/linux/openvswitch.h b/include/uapi/linux/openvswitch.h
index 9d1710f20505..ab6755621e02 100644
--- a/include/uapi/linux/openvswitch.h
+++ b/include/uapi/linux/openvswitch.h
@@ -351,11 +351,16 @@ enum ovs_key_attr {
        OVS_KEY_ATTR_CT_ORIG_TUPLE_IPV4,   /* struct ovs_key_ct_tuple_ipv4 */
        OVS_KEY_ATTR_CT_ORIG_TUPLE_IPV6,   /* struct ovs_key_ct_tuple_ipv6 */
        OVS_KEY_ATTR_NSH,       /* Nested set of ovs_nsh_key_* */
-       OVS_KEY_ATTR_IPV6_EXTHDRS,  /* struct ovs_key_ipv6_exthdr */
 
 #ifdef __KERNEL__
        OVS_KEY_ATTR_TUNNEL_INFO,  /* struct ip_tunnel_info */
 #endif
+       /* User space decided to squat on types 30 and 31 */
+       OVS_KEY_ATTR_IPV6_EXTHDRS = 32, /* struct ovs_key_ipv6_exthdr */
+       /* WARNING: <scary warning to avoid the problem coming back> */
+
        __OVS_KEY_ATTR_MAX
 };


right?

> > Since ovs uses genetlink you should be able to dump the policy from 
> > the kernel and at least validate that it doesn't overlap.  
> 
> That is interesting.  Indeed, this functionality can be used to detect
> problems or to define userspace-only attributes in runtime based on the
> kernel reply.  Thanks for the pointer!

^ permalink raw reply related	[flat|nested] 22+ messages in thread

* Re: [ovs-dev] [PATCH net-next v8] net: openvswitch: IPv6: Add IPv6 extension header support
  2022-03-08  5:45                   ` Jakub Kicinski
@ 2022-03-08  8:21                     ` Johannes Berg
  2022-03-08 14:12                       ` Ilya Maximets
  0 siblings, 1 reply; 22+ messages in thread
From: Johannes Berg @ 2022-03-08  8:21 UTC (permalink / raw)
  To: Jakub Kicinski, Ilya Maximets
  Cc: Roi Dayan, dev, Toms Atteka, netdev, linux-kernel, davem,
	David Ahern, Jiri Pirko, Pablo Neira Ayuso

On Mon, 2022-03-07 at 21:45 -0800, Jakub Kicinski wrote:
> 
> Let me add some people I associate with genetlink work in my head
> (fairly or not) to keep me fair here.

:)

> It's highly unacceptable for user space to straight up rewrite kernel
> uAPI types
> 

Agree.

> but if it already happened the only fix is something like:
> 
> diff --git a/include/uapi/linux/openvswitch.h b/include/uapi/linux/openvswitch.h
> index 9d1710f20505..ab6755621e02 100644
> --- a/include/uapi/linux/openvswitch.h
> +++ b/include/uapi/linux/openvswitch.h
> @@ -351,11 +351,16 @@ enum ovs_key_attr {
>         OVS_KEY_ATTR_CT_ORIG_TUPLE_IPV4,   /* struct ovs_key_ct_tuple_ipv4 */
>         OVS_KEY_ATTR_CT_ORIG_TUPLE_IPV6,   /* struct ovs_key_ct_tuple_ipv6 */
>         OVS_KEY_ATTR_NSH,       /* Nested set of ovs_nsh_key_* */
> -       OVS_KEY_ATTR_IPV6_EXTHDRS,  /* struct ovs_key_ipv6_exthdr */
>  
>  #ifdef __KERNEL__
>         OVS_KEY_ATTR_TUNNEL_INFO,  /* struct ip_tunnel_info */
>  #endif
> +       /* User space decided to squat on types 30 and 31 */
> +       OVS_KEY_ATTR_IPV6_EXTHDRS = 32, /* struct ovs_key_ipv6_exthdr */
> +       /* WARNING: <scary warning to avoid the problem coming back> */

It might be nicer to actually document here in what's at least supposed
to be the canonical documentation of the API what those types were used
for. Note that with strict validation at least they're rejected by the
kernel, but of course I have no idea what kind of contortions userspace
does to make it even think about defining its own types (netlink
normally sits at the kernel/userspace boundary, so where does it make
sense for userspace to have its own types?)

(Though note that technically netlink supports userspace<->userspace
communication, but that's not used much)

> > > Since ovs uses genetlink you should be able to dump the policy from 
> > > the kernel and at least validate that it doesn't overlap.  
> > 
> > That is interesting.  Indeed, this functionality can be used to detect
> > problems or to define userspace-only attributes in runtime based on the
> > kernel reply.  Thanks for the pointer!

As you note, you'd have to do that at runtime since it can change, even
the policy. And things not in the policy probably should never be sent
to the kernel even if strict validation isn't used.

johannes

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [ovs-dev] [PATCH net-next v8] net: openvswitch: IPv6: Add IPv6 extension header support
  2022-03-08  8:21                     ` Johannes Berg
@ 2022-03-08 14:12                       ` Ilya Maximets
  2022-03-08 14:39                         ` Roi Dayan
  2022-03-08 16:17                         ` Jakub Kicinski
  0 siblings, 2 replies; 22+ messages in thread
From: Ilya Maximets @ 2022-03-08 14:12 UTC (permalink / raw)
  To: Johannes Berg, Jakub Kicinski
  Cc: i.maximets, Roi Dayan, dev, Toms Atteka, netdev, linux-kernel,
	davem, David Ahern, Jiri Pirko, Pablo Neira Ayuso

On 3/8/22 09:21, Johannes Berg wrote:
> On Mon, 2022-03-07 at 21:45 -0800, Jakub Kicinski wrote:
>>
>> Let me add some people I associate with genetlink work in my head
>> (fairly or not) to keep me fair here.
> 
> :)
> 
>> It's highly unacceptable for user space to straight up rewrite kernel
>> uAPI types
>>
> 
> Agree.

I 100% agree with that and will work on the userspace part to make sure
we're not adding anything to the kernel uAPI types.

FWIW, the quick grep over usespace code shows similar problem with a few
other types, but they are less severe, because they are provided as part
of OVS actions and kernel doesn't send anything that wasn't previously
set by userspace in that case.  There still might be a problem during the
downgrade of the userspace while kernel configuration remains intact,
but that is not a common scenario.  Will work on fixing that in userspace.
No need to change the kernel uAPI for these, IMO.

> 
>> but if it already happened the only fix is something like:
>>
>> diff --git a/include/uapi/linux/openvswitch.h b/include/uapi/linux/openvswitch.h
>> index 9d1710f20505..ab6755621e02 100644
>> --- a/include/uapi/linux/openvswitch.h
>> +++ b/include/uapi/linux/openvswitch.h
>> @@ -351,11 +351,16 @@ enum ovs_key_attr {
>>         OVS_KEY_ATTR_CT_ORIG_TUPLE_IPV4,   /* struct ovs_key_ct_tuple_ipv4 */
>>         OVS_KEY_ATTR_CT_ORIG_TUPLE_IPV6,   /* struct ovs_key_ct_tuple_ipv6 */
>>         OVS_KEY_ATTR_NSH,       /* Nested set of ovs_nsh_key_* */
>> -       OVS_KEY_ATTR_IPV6_EXTHDRS,  /* struct ovs_key_ipv6_exthdr */
>>  
>>  #ifdef __KERNEL__
>>         OVS_KEY_ATTR_TUNNEL_INFO,  /* struct ip_tunnel_info */
>>  #endif
>> +       /* User space decided to squat on types 30 and 31 */
>> +       OVS_KEY_ATTR_IPV6_EXTHDRS = 32, /* struct ovs_key_ipv6_exthdr */
>> +       /* WARNING: <scary warning to avoid the problem coming back> */

Yes, that is something that I had in mind too.  The only thing that makes
me uncomfortable is OVS_KEY_ATTR_TUNNEL_INFO = 30 here.  Even though it
doesn't make a lot of difference, I'd better keep the kernel-only attributes
at the end of the enumeration.  Is there a better way to handle kernel-only
attribute?

Also, the OVS_KEY_ATTR_ND_EXTENSIONS (31) attribute used to store IPv6 Neighbor
Discovery extensions is currently implemented only for userspace, but nothing
actually prevents us having the kernel implementation.  So, we need a way to
make it usable by the kernel in the future.

> 
> It might be nicer to actually document here in what's at least supposed
> to be the canonical documentation of the API what those types were used
> for.

I agree with that.

> Note that with strict validation at least they're rejected by the
> kernel, but of course I have no idea what kind of contortions userspace
> does to make it even think about defining its own types (netlink
> normally sits at the kernel/userspace boundary, so where does it make
> sense for userspace to have its own types?)
> 
> (Though note that technically netlink supports userspace<->userspace
> communication, but that's not used much)

OVS has a common high-level interface+logic and several different
implementations of a "datapath".  One of datapaths is inside the Linux
kernel which we're discussing here, another is completely in userspace
(to make use of DPDK or AF_XDP), there is also an implementation for the
Windows kernel.  Since the way to talk with the Linux kernel is netlink,
OVS is using netlink-based communication to communicate between high-level
parts and all types of datapaths.  Some features might be supported by
one datapath and not supported by others, hence some way to extend the
communication is needed.  E.g. kernel currently doesn't parse ND extensions,
but userspace datapath does.

But yes, the current implementation is awful and OVS need to have a
different way of managing datapath-specific attributes and not touch
kernel-defined types.  We'll work on that.

> 
>>>> Since ovs uses genetlink you should be able to dump the policy from 
>>>> the kernel and at least validate that it doesn't overlap.  
>>>
>>> That is interesting.  Indeed, this functionality can be used to detect
>>> problems or to define userspace-only attributes in runtime based on the
>>> kernel reply.  Thanks for the pointer!
> 
> As you note, you'd have to do that at runtime since it can change, even
> the policy. And things not in the policy probably should never be sent
> to the kernel even if strict validation isn't used.

Agree.  AFAICT, OVS currently doesn't send to the kernel things that kernel
doesn't support.

> 
> johannes


^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [ovs-dev] [PATCH net-next v8] net: openvswitch: IPv6: Add IPv6 extension header support
  2022-03-08 14:12                       ` Ilya Maximets
@ 2022-03-08 14:39                         ` Roi Dayan
  2022-03-08 18:25                           ` Ilya Maximets
  2022-03-08 16:17                         ` Jakub Kicinski
  1 sibling, 1 reply; 22+ messages in thread
From: Roi Dayan @ 2022-03-08 14:39 UTC (permalink / raw)
  To: Ilya Maximets, Johannes Berg, Jakub Kicinski
  Cc: dev, Toms Atteka, netdev, linux-kernel, davem, David Ahern,
	Jiri Pirko, Pablo Neira Ayuso



On 2022-03-08 4:12 PM, Ilya Maximets wrote:
> On 3/8/22 09:21, Johannes Berg wrote:
>> On Mon, 2022-03-07 at 21:45 -0800, Jakub Kicinski wrote:
>>>
>>> Let me add some people I associate with genetlink work in my head
>>> (fairly or not) to keep me fair here.
>>
>> :)
>>
>>> It's highly unacceptable for user space to straight up rewrite kernel
>>> uAPI types
>>>
>>
>> Agree.
> 
> I 100% agree with that and will work on the userspace part to make sure
> we're not adding anything to the kernel uAPI types.
> 
> FWIW, the quick grep over usespace code shows similar problem with a few
> other types, but they are less severe, because they are provided as part
> of OVS actions and kernel doesn't send anything that wasn't previously
> set by userspace in that case.  There still might be a problem during the
> downgrade of the userspace while kernel configuration remains intact,
> but that is not a common scenario.  Will work on fixing that in userspace.
> No need to change the kernel uAPI for these, IMO.
> 

since its rc7 we end up with kernel and ovs broken with each other.
can we revert the kernel patches anyway and introduce them again later
when ovs userspace is also updated?

>>
>>> but if it already happened the only fix is something like:
>>>
>>> diff --git a/include/uapi/linux/openvswitch.h b/include/uapi/linux/openvswitch.h
>>> index 9d1710f20505..ab6755621e02 100644
>>> --- a/include/uapi/linux/openvswitch.h
>>> +++ b/include/uapi/linux/openvswitch.h
>>> @@ -351,11 +351,16 @@ enum ovs_key_attr {
>>>          OVS_KEY_ATTR_CT_ORIG_TUPLE_IPV4,   /* struct ovs_key_ct_tuple_ipv4 */
>>>          OVS_KEY_ATTR_CT_ORIG_TUPLE_IPV6,   /* struct ovs_key_ct_tuple_ipv6 */
>>>          OVS_KEY_ATTR_NSH,       /* Nested set of ovs_nsh_key_* */
>>> -       OVS_KEY_ATTR_IPV6_EXTHDRS,  /* struct ovs_key_ipv6_exthdr */
>>>   
>>>   #ifdef __KERNEL__
>>>          OVS_KEY_ATTR_TUNNEL_INFO,  /* struct ip_tunnel_info */
>>>   #endif
>>> +       /* User space decided to squat on types 30 and 31 */
>>> +       OVS_KEY_ATTR_IPV6_EXTHDRS = 32, /* struct ovs_key_ipv6_exthdr */
>>> +       /* WARNING: <scary warning to avoid the problem coming back> */
> 
> Yes, that is something that I had in mind too.  The only thing that makes
> me uncomfortable is OVS_KEY_ATTR_TUNNEL_INFO = 30 here.  Even though it
> doesn't make a lot of difference, I'd better keep the kernel-only attributes
> at the end of the enumeration.  Is there a better way to handle kernel-only
> attribute?
> 
> Also, the OVS_KEY_ATTR_ND_EXTENSIONS (31) attribute used to store IPv6 Neighbor
> Discovery extensions is currently implemented only for userspace, but nothing
> actually prevents us having the kernel implementation.  So, we need a way to
> make it usable by the kernel in the future.
> 
>>
>> It might be nicer to actually document here in what's at least supposed
>> to be the canonical documentation of the API what those types were used
>> for.
> 
> I agree with that.
> 
>> Note that with strict validation at least they're rejected by the
>> kernel, but of course I have no idea what kind of contortions userspace
>> does to make it even think about defining its own types (netlink
>> normally sits at the kernel/userspace boundary, so where does it make
>> sense for userspace to have its own types?)
>>
>> (Though note that technically netlink supports userspace<->userspace
>> communication, but that's not used much)
> 
> OVS has a common high-level interface+logic and several different
> implementations of a "datapath".  One of datapaths is inside the Linux
> kernel which we're discussing here, another is completely in userspace
> (to make use of DPDK or AF_XDP), there is also an implementation for the
> Windows kernel.  Since the way to talk with the Linux kernel is netlink,
> OVS is using netlink-based communication to communicate between high-level
> parts and all types of datapaths.  Some features might be supported by
> one datapath and not supported by others, hence some way to extend the
> communication is needed.  E.g. kernel currently doesn't parse ND extensions,
> but userspace datapath does.
> 
> But yes, the current implementation is awful and OVS need to have a
> different way of managing datapath-specific attributes and not touch
> kernel-defined types.  We'll work on that.
> 
>>
>>>>> Since ovs uses genetlink you should be able to dump the policy from
>>>>> the kernel and at least validate that it doesn't overlap.
>>>>
>>>> That is interesting.  Indeed, this functionality can be used to detect
>>>> problems or to define userspace-only attributes in runtime based on the
>>>> kernel reply.  Thanks for the pointer!
>>
>> As you note, you'd have to do that at runtime since it can change, even
>> the policy. And things not in the policy probably should never be sent
>> to the kernel even if strict validation isn't used.
> 
> Agree.  AFAICT, OVS currently doesn't send to the kernel things that kernel
> doesn't support.
> 
>>
>> johannes
> 

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [ovs-dev] [PATCH net-next v8] net: openvswitch: IPv6: Add IPv6 extension header support
  2022-03-08 14:12                       ` Ilya Maximets
  2022-03-08 14:39                         ` Roi Dayan
@ 2022-03-08 16:17                         ` Jakub Kicinski
  2022-03-08 19:33                           ` Ilya Maximets
  1 sibling, 1 reply; 22+ messages in thread
From: Jakub Kicinski @ 2022-03-08 16:17 UTC (permalink / raw)
  To: Ilya Maximets
  Cc: Johannes Berg, Roi Dayan, dev, Toms Atteka, netdev, linux-kernel,
	davem, David Ahern, Jiri Pirko, Pablo Neira Ayuso

On Tue, 8 Mar 2022 15:12:45 +0100 Ilya Maximets wrote:
> >> diff --git a/include/uapi/linux/openvswitch.h b/include/uapi/linux/openvswitch.h
> >> index 9d1710f20505..ab6755621e02 100644
> >> --- a/include/uapi/linux/openvswitch.h
> >> +++ b/include/uapi/linux/openvswitch.h
> >> @@ -351,11 +351,16 @@ enum ovs_key_attr {
> >>         OVS_KEY_ATTR_CT_ORIG_TUPLE_IPV4,   /* struct ovs_key_ct_tuple_ipv4 */
> >>         OVS_KEY_ATTR_CT_ORIG_TUPLE_IPV6,   /* struct ovs_key_ct_tuple_ipv6 */
> >>         OVS_KEY_ATTR_NSH,       /* Nested set of ovs_nsh_key_* */
> >> -       OVS_KEY_ATTR_IPV6_EXTHDRS,  /* struct ovs_key_ipv6_exthdr */
> >>  
> >>  #ifdef __KERNEL__
> >>         OVS_KEY_ATTR_TUNNEL_INFO,  /* struct ip_tunnel_info */
> >>  #endif
> >> +       /* User space decided to squat on types 30 and 31 */
> >> +       OVS_KEY_ATTR_IPV6_EXTHDRS = 32, /* struct ovs_key_ipv6_exthdr */
> >> +       /* WARNING: <scary warning to avoid the problem coming back> */  
> 
> Yes, that is something that I had in mind too.  The only thing that makes
> me uncomfortable is OVS_KEY_ATTR_TUNNEL_INFO = 30 here.  Even though it
> doesn't make a lot of difference, I'd better keep the kernel-only attributes
> at the end of the enumeration.  Is there a better way to handle kernel-only
> attribute?

My thought was to leave the kernel/userspace only types "behind" to
avoid perpetuating the same constructs.

Johannes's point about userspace to userspace messages makes the whole
thing a little less of an aberration.

Is there a reason for the types to be hidden under __KERNEL__? 
Or someone did that in a misguided attempt to save space in attr arrays
when parsing?

> Also, the OVS_KEY_ATTR_ND_EXTENSIONS (31) attribute used to store IPv6 Neighbor
> Discovery extensions is currently implemented only for userspace, but nothing
> actually prevents us having the kernel implementation.  So, we need a way to
> make it usable by the kernel in the future.

The "= 32" leaves the earlier attr types as reserved so nothing
prevents us from defining them later. But..

> > It might be nicer to actually document here in what's at least supposed
> > to be the canonical documentation of the API what those types were used
> > for.  
> 
> I agree with that.

Should we add the user space types to the kernel header and remove the
ifdef __KERNEL__ around TUNNEL_INFO, then?

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [ovs-dev] [PATCH net-next v8] net: openvswitch: IPv6: Add IPv6 extension header support
  2022-03-08 14:39                         ` Roi Dayan
@ 2022-03-08 18:25                           ` Ilya Maximets
  2022-03-08 20:17                             ` Jakub Kicinski
  0 siblings, 1 reply; 22+ messages in thread
From: Ilya Maximets @ 2022-03-08 18:25 UTC (permalink / raw)
  To: Roi Dayan, Johannes Berg, Jakub Kicinski
  Cc: i.maximets, dev, Toms Atteka, netdev, linux-kernel, davem,
	David Ahern, Jiri Pirko, Pablo Neira Ayuso

On 3/8/22 15:39, Roi Dayan wrote:
> 
> 
> On 2022-03-08 4:12 PM, Ilya Maximets wrote:
>> On 3/8/22 09:21, Johannes Berg wrote:
>>> On Mon, 2022-03-07 at 21:45 -0800, Jakub Kicinski wrote:
>>>>
>>>> Let me add some people I associate with genetlink work in my head
>>>> (fairly or not) to keep me fair here.
>>>
>>> :)
>>>
>>>> It's highly unacceptable for user space to straight up rewrite kernel
>>>> uAPI types
>>>>
>>>
>>> Agree.
>>
>> I 100% agree with that and will work on the userspace part to make sure
>> we're not adding anything to the kernel uAPI types.
>>
>> FWIW, the quick grep over usespace code shows similar problem with a few
>> other types, but they are less severe, because they are provided as part
>> of OVS actions and kernel doesn't send anything that wasn't previously
>> set by userspace in that case.  There still might be a problem during the
>> downgrade of the userspace while kernel configuration remains intact,
>> but that is not a common scenario.  Will work on fixing that in userspace.
>> No need to change the kernel uAPI for these, IMO.
>>
> 
> since its rc7 we end up with kernel and ovs broken with each other.
> can we revert the kernel patches anyway and introduce them again later
> when ovs userspace is also updated?

I don't think this patch is part of 5-17-rc7.  AFAICT, it's a candidate
for 5.18, so we should still have a bit of time.  Am I missing something?

Best regards, Ilya Maximets.

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [ovs-dev] [PATCH net-next v8] net: openvswitch: IPv6: Add IPv6 extension header support
  2022-03-08 16:17                         ` Jakub Kicinski
@ 2022-03-08 19:33                           ` Ilya Maximets
  2022-03-08 20:16                             ` Jakub Kicinski
  0 siblings, 1 reply; 22+ messages in thread
From: Ilya Maximets @ 2022-03-08 19:33 UTC (permalink / raw)
  To: Jakub Kicinski
  Cc: i.maximets, Johannes Berg, Roi Dayan, dev, Toms Atteka, netdev,
	linux-kernel, davem, David Ahern, Jiri Pirko, Pablo Neira Ayuso,
	pshelar

On 3/8/22 17:17, Jakub Kicinski wrote:
> On Tue, 8 Mar 2022 15:12:45 +0100 Ilya Maximets wrote:
>>>> diff --git a/include/uapi/linux/openvswitch.h b/include/uapi/linux/openvswitch.h
>>>> index 9d1710f20505..ab6755621e02 100644
>>>> --- a/include/uapi/linux/openvswitch.h
>>>> +++ b/include/uapi/linux/openvswitch.h
>>>> @@ -351,11 +351,16 @@ enum ovs_key_attr {
>>>>         OVS_KEY_ATTR_CT_ORIG_TUPLE_IPV4,   /* struct ovs_key_ct_tuple_ipv4 */
>>>>         OVS_KEY_ATTR_CT_ORIG_TUPLE_IPV6,   /* struct ovs_key_ct_tuple_ipv6 */
>>>>         OVS_KEY_ATTR_NSH,       /* Nested set of ovs_nsh_key_* */
>>>> -       OVS_KEY_ATTR_IPV6_EXTHDRS,  /* struct ovs_key_ipv6_exthdr */
>>>>  
>>>>  #ifdef __KERNEL__
>>>>         OVS_KEY_ATTR_TUNNEL_INFO,  /* struct ip_tunnel_info */
>>>>  #endif
>>>> +       /* User space decided to squat on types 30 and 31 */
>>>> +       OVS_KEY_ATTR_IPV6_EXTHDRS = 32, /* struct ovs_key_ipv6_exthdr */
>>>> +       /* WARNING: <scary warning to avoid the problem coming back> */  
>>
>> Yes, that is something that I had in mind too.  The only thing that makes
>> me uncomfortable is OVS_KEY_ATTR_TUNNEL_INFO = 30 here.  Even though it
>> doesn't make a lot of difference, I'd better keep the kernel-only attributes
>> at the end of the enumeration.  Is there a better way to handle kernel-only
>> attribute?
> 
> My thought was to leave the kernel/userspace only types "behind" to
> avoid perpetuating the same constructs.
> 
> Johannes's point about userspace to userspace messages makes the whole
> thing a little less of an aberration.
> 
> Is there a reason for the types to be hidden under __KERNEL__? 
> Or someone did that in a misguided attempt to save space in attr arrays
> when parsing?

My impression is that OVS_KEY_ATTR_TUNNEL_INFO was hidden from the
user space just because it's not supposed to ever be used by the
user space application.  Pravin or Jesse should know for sure.

> 
>> Also, the OVS_KEY_ATTR_ND_EXTENSIONS (31) attribute used to store IPv6 Neighbor
>> Discovery extensions is currently implemented only for userspace, but nothing
>> actually prevents us having the kernel implementation.  So, we need a way to
>> make it usable by the kernel in the future.
> 
> The "= 32" leaves the earlier attr types as reserved so nothing
> prevents us from defining them later. But..
> 
>>> It might be nicer to actually document here in what's at least supposed
>>> to be the canonical documentation of the API what those types were used
>>> for.  
>>
>> I agree with that.
> 
> Should we add the user space types to the kernel header and remove the
> ifdef __KERNEL__ around TUNNEL_INFO, then?

I don't think we need to actually define them, but we may list them
in the comment.  I'm OK with either option though.

For the removal of ifdef __KERNEL__, that might be a good thing to do.
I'm just not sure what are the best practices here.
We'll need to make some code changes in user space to avoid warnings
about not all the enum members being used in 'switch'es.  But that's
not a problem.

If you think that having a flat enum without 'ifdef's is a viable
option from a kernel's point of view, I'm all for it.

Maybe something like this (only checked that this compiles; 29 and
30 are correct numbers of these userspace attributes):

---
diff --git a/include/uapi/linux/openvswitch.h b/include/uapi/linux/openvswitch.h
index 9d1710f20505..86bc951be5bc 100644
--- a/include/uapi/linux/openvswitch.h
+++ b/include/uapi/linux/openvswitch.h
@@ -351,11 +351,19 @@ enum ovs_key_attr {
 	OVS_KEY_ATTR_CT_ORIG_TUPLE_IPV4,   /* struct ovs_key_ct_tuple_ipv4 */
 	OVS_KEY_ATTR_CT_ORIG_TUPLE_IPV6,   /* struct ovs_key_ct_tuple_ipv6 */
 	OVS_KEY_ATTR_NSH,       /* Nested set of ovs_nsh_key_* */
-	OVS_KEY_ATTR_IPV6_EXTHDRS,  /* struct ovs_key_ipv6_exthdr */
 
-#ifdef __KERNEL__
-	OVS_KEY_ATTR_TUNNEL_INFO,  /* struct ip_tunnel_info */
-#endif
+	/* User space decided to squat on types 29 and 30.  They are listed
+	 * below, but should not be sent to the kernel:
+	 *
+	 * OVS_KEY_ATTR_PACKET_TYPE,   be32 packet type
+	 * OVS_KEY_ATTR_ND_EXTENSIONS, IPv6 Neighbor Discovery extensions
+	 *
+	 * WARNING: No new types should be added unless they are defined
+	 *          for both kernel and user space (no 'ifdef's).  It's hard
+	 *          to keep compatibility otherwise. */
+	OVS_KEY_ATTR_TUNNEL_INFO = 31,  /* struct ip_tunnel_info.
+					   For in-kernel use only. */
+	OVS_KEY_ATTR_IPV6_EXTHDRS,  /* struct ovs_key_ipv6_exthdr */
 	__OVS_KEY_ATTR_MAX
 };
 
diff --git a/net/openvswitch/flow_netlink.c b/net/openvswitch/flow_netlink.c
index 8b4124820f7d..315064bada3e 100644
--- a/net/openvswitch/flow_netlink.c
+++ b/net/openvswitch/flow_netlink.c
@@ -346,7 +346,7 @@ size_t ovs_key_attr_size(void)
 	/* Whenever adding new OVS_KEY_ FIELDS, we should consider
 	 * updating this function.
 	 */
-	BUILD_BUG_ON(OVS_KEY_ATTR_TUNNEL_INFO != 30);
+	BUILD_BUG_ON(OVS_KEY_ATTR_MAX != 32);
 
 	return    nla_total_size(4)   /* OVS_KEY_ATTR_PRIORITY */
 		+ nla_total_size(0)   /* OVS_KEY_ATTR_TUNNEL */
---

Thoughts?

The same change can be ported to the user-space header, but with
types actually defined and not part of the comment.  It may look
like this: https://pastebin.com/k8UWEZtR  (without IPV6_EXTHDRS yet).
For the future, we'll try to find a way to define them in a separate
enum or will define them dynamically based on the policy dumped from
the currently running kernel. In any case no new userspace-only types
should be defined in that enum.

Best regards, Ilya Maximets.

^ permalink raw reply related	[flat|nested] 22+ messages in thread

* Re: [ovs-dev] [PATCH net-next v8] net: openvswitch: IPv6: Add IPv6 extension header support
  2022-03-08 19:33                           ` Ilya Maximets
@ 2022-03-08 20:16                             ` Jakub Kicinski
  2022-03-09  7:49                               ` Roi Dayan
  0 siblings, 1 reply; 22+ messages in thread
From: Jakub Kicinski @ 2022-03-08 20:16 UTC (permalink / raw)
  To: Ilya Maximets
  Cc: Johannes Berg, Roi Dayan, dev, Toms Atteka, netdev, linux-kernel,
	davem, David Ahern, Jiri Pirko, Pablo Neira Ayuso, pshelar

On Tue, 8 Mar 2022 20:33:12 +0100 Ilya Maximets wrote:
> On 3/8/22 17:17, Jakub Kicinski wrote:
> > On Tue, 8 Mar 2022 15:12:45 +0100 Ilya Maximets wrote:  
> >> Yes, that is something that I had in mind too.  The only thing that makes
> >> me uncomfortable is OVS_KEY_ATTR_TUNNEL_INFO = 30 here.  Even though it
> >> doesn't make a lot of difference, I'd better keep the kernel-only attributes
> >> at the end of the enumeration.  Is there a better way to handle kernel-only
> >> attribute?  
> > 
> > My thought was to leave the kernel/userspace only types "behind" to
> > avoid perpetuating the same constructs.
> > 
> > Johannes's point about userspace to userspace messages makes the whole
> > thing a little less of an aberration.
> > 
> > Is there a reason for the types to be hidden under __KERNEL__? 
> > Or someone did that in a misguided attempt to save space in attr arrays
> > when parsing?  
> 
> My impression is that OVS_KEY_ATTR_TUNNEL_INFO was hidden from the
> user space just because it's not supposed to ever be used by the
> user space application.  Pravin or Jesse should know for sure.

Hard to make any assumptions about what user space that takes 
the liberty of re-defining kernel uAPI types will or will not
do ;) The only way to be safe would be to actively reject the
attr ID on the kernel side. Unless user space uses the same
exact name for something else IMHO hiding the value doesn't
afford us any extra protection.

> >> I agree with that.  
> > 
> > Should we add the user space types to the kernel header and remove the
> > ifdef __KERNEL__ around TUNNEL_INFO, then?  
> 
> I don't think we need to actually define them, but we may list them
> in the comment.  I'm OK with either option though.
> 
> For the removal of ifdef __KERNEL__, that might be a good thing to do.
> I'm just not sure what are the best practices here.
> We'll need to make some code changes in user space to avoid warnings
> about not all the enum members being used in 'switch'es.  But that's
> not a problem.

Presumably only as the headers are updated? IOW we would not break 
the build for older sources?

> If you think that having a flat enum without 'ifdef's is a viable
> option from a kernel's point of view, I'm all for it.
> 
> Maybe something like this (only checked that this compiles; 29 and
> 30 are correct numbers of these userspace attributes):

It's a bit of an uncharted territory, hard to say what's right.
It may be a little easier to code up the rejection if we have 
the types defined (which I think we need to do in
__parse_flow_nlattrs()? seems OvS does its own nla parsing?)

Johannes, any preference?

> diff --git a/include/uapi/linux/openvswitch.h b/include/uapi/linux/openvswitch.h
> index 9d1710f20505..86bc951be5bc 100644
> --- a/include/uapi/linux/openvswitch.h
> +++ b/include/uapi/linux/openvswitch.h
> @@ -351,11 +351,19 @@ enum ovs_key_attr {
>  	OVS_KEY_ATTR_CT_ORIG_TUPLE_IPV4,   /* struct ovs_key_ct_tuple_ipv4 */
>  	OVS_KEY_ATTR_CT_ORIG_TUPLE_IPV6,   /* struct ovs_key_ct_tuple_ipv6 */
>  	OVS_KEY_ATTR_NSH,       /* Nested set of ovs_nsh_key_* */
> -	OVS_KEY_ATTR_IPV6_EXTHDRS,  /* struct ovs_key_ipv6_exthdr */
>  
> -#ifdef __KERNEL__
> -	OVS_KEY_ATTR_TUNNEL_INFO,  /* struct ip_tunnel_info */
> -#endif
> +	/* User space decided to squat on types 29 and 30.  They are listed
> +	 * below, but should not be sent to the kernel:
> +	 *
> +	 * OVS_KEY_ATTR_PACKET_TYPE,   be32 packet type
> +	 * OVS_KEY_ATTR_ND_EXTENSIONS, IPv6 Neighbor Discovery extensions
> +	 *
> +	 * WARNING: No new types should be added unless they are defined
> +	 *          for both kernel and user space (no 'ifdef's).  It's hard
> +	 *          to keep compatibility otherwise. */
> +	OVS_KEY_ATTR_TUNNEL_INFO = 31,  /* struct ip_tunnel_info.
> +					   For in-kernel use only. */
> +	OVS_KEY_ATTR_IPV6_EXTHDRS,  /* struct ovs_key_ipv6_exthdr */
>  	__OVS_KEY_ATTR_MAX
>  };
>  
> diff --git a/net/openvswitch/flow_netlink.c b/net/openvswitch/flow_netlink.c
> index 8b4124820f7d..315064bada3e 100644
> --- a/net/openvswitch/flow_netlink.c
> +++ b/net/openvswitch/flow_netlink.c
> @@ -346,7 +346,7 @@ size_t ovs_key_attr_size(void)
>  	/* Whenever adding new OVS_KEY_ FIELDS, we should consider
>  	 * updating this function.
>  	 */
> -	BUILD_BUG_ON(OVS_KEY_ATTR_TUNNEL_INFO != 30);
> +	BUILD_BUG_ON(OVS_KEY_ATTR_MAX != 32);
>  
>  	return    nla_total_size(4)   /* OVS_KEY_ATTR_PRIORITY */
>  		+ nla_total_size(0)   /* OVS_KEY_ATTR_TUNNEL */
> ---
> 
> Thoughts?
> 
> The same change can be ported to the user-space header, but with
> types actually defined and not part of the comment.  It may look
> like this: https://pastebin.com/k8UWEZtR  (without IPV6_EXTHDRS yet).
> For the future, we'll try to find a way to define them in a separate
> enum or will define them dynamically based on the policy dumped from
> the currently running kernel. In any case no new userspace-only types
> should be defined in that enum.


^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [ovs-dev] [PATCH net-next v8] net: openvswitch: IPv6: Add IPv6 extension header support
  2022-03-08 18:25                           ` Ilya Maximets
@ 2022-03-08 20:17                             ` Jakub Kicinski
  0 siblings, 0 replies; 22+ messages in thread
From: Jakub Kicinski @ 2022-03-08 20:17 UTC (permalink / raw)
  To: Ilya Maximets
  Cc: Roi Dayan, Johannes Berg, dev, Toms Atteka, netdev, linux-kernel,
	davem, David Ahern, Jiri Pirko, Pablo Neira Ayuso

On Tue, 8 Mar 2022 19:25:31 +0100 Ilya Maximets wrote:
> > since its rc7 we end up with kernel and ovs broken with each other.
> > can we revert the kernel patches anyway and introduce them again later
> > when ovs userspace is also updated?  
> 
> I don't think this patch is part of 5-17-rc7.  AFAICT, it's a candidate
> for 5.18, so we should still have a bit of time.  Am I missing something?

You are correct, it's going to hit Linus's tree during the 5.18 merge
window. It seems we're close enough to a resolution to focus on a fix
instead.

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [ovs-dev] [PATCH net-next v8] net: openvswitch: IPv6: Add IPv6 extension header support
  2022-03-08 20:16                             ` Jakub Kicinski
@ 2022-03-09  7:49                               ` Roi Dayan
  2022-03-09 13:43                                 ` Ilya Maximets
  0 siblings, 1 reply; 22+ messages in thread
From: Roi Dayan @ 2022-03-09  7:49 UTC (permalink / raw)
  To: Jakub Kicinski, Ilya Maximets
  Cc: Johannes Berg, dev, Toms Atteka, netdev, linux-kernel, davem,
	David Ahern, Jiri Pirko, Pablo Neira Ayuso, pshelar



On 2022-03-08 10:16 PM, Jakub Kicinski wrote:
> On Tue, 8 Mar 2022 20:33:12 +0100 Ilya Maximets wrote:
>> On 3/8/22 17:17, Jakub Kicinski wrote:
>>> On Tue, 8 Mar 2022 15:12:45 +0100 Ilya Maximets wrote:
>>>> Yes, that is something that I had in mind too.  The only thing that makes
>>>> me uncomfortable is OVS_KEY_ATTR_TUNNEL_INFO = 30 here.  Even though it
>>>> doesn't make a lot of difference, I'd better keep the kernel-only attributes
>>>> at the end of the enumeration.  Is there a better way to handle kernel-only
>>>> attribute?
>>>
>>> My thought was to leave the kernel/userspace only types "behind" to
>>> avoid perpetuating the same constructs.
>>>
>>> Johannes's point about userspace to userspace messages makes the whole
>>> thing a little less of an aberration.
>>>
>>> Is there a reason for the types to be hidden under __KERNEL__?
>>> Or someone did that in a misguided attempt to save space in attr arrays
>>> when parsing?
>>
>> My impression is that OVS_KEY_ATTR_TUNNEL_INFO was hidden from the
>> user space just because it's not supposed to ever be used by the
>> user space application.  Pravin or Jesse should know for sure.
> 
> Hard to make any assumptions about what user space that takes
> the liberty of re-defining kernel uAPI types will or will not
> do ;) The only way to be safe would be to actively reject the
> attr ID on the kernel side. Unless user space uses the same
> exact name for something else IMHO hiding the value doesn't
> afford us any extra protection.
> 
>>>> I agree with that.
>>>
>>> Should we add the user space types to the kernel header and remove the
>>> ifdef __KERNEL__ around TUNNEL_INFO, then?
>>
>> I don't think we need to actually define them, but we may list them
>> in the comment.  I'm OK with either option though.
>>
>> For the removal of ifdef __KERNEL__, that might be a good thing to do.
>> I'm just not sure what are the best practices here.
>> We'll need to make some code changes in user space to avoid warnings
>> about not all the enum members being used in 'switch'es.  But that's
>> not a problem.
> 
> Presumably only as the headers are updated? IOW we would not break
> the build for older sources?
> 
>> If you think that having a flat enum without 'ifdef's is a viable
>> option from a kernel's point of view, I'm all for it.
>>
>> Maybe something like this (only checked that this compiles; 29 and
>> 30 are correct numbers of these userspace attributes):
> 
> It's a bit of an uncharted territory, hard to say what's right.
> It may be a little easier to code up the rejection if we have
> the types defined (which I think we need to do in
> __parse_flow_nlattrs()? seems OvS does its own nla parsing?)
> 
> Johannes, any preference?
> 
>> diff --git a/include/uapi/linux/openvswitch.h b/include/uapi/linux/openvswitch.h
>> index 9d1710f20505..86bc951be5bc 100644
>> --- a/include/uapi/linux/openvswitch.h
>> +++ b/include/uapi/linux/openvswitch.h
>> @@ -351,11 +351,19 @@ enum ovs_key_attr {
>>   	OVS_KEY_ATTR_CT_ORIG_TUPLE_IPV4,   /* struct ovs_key_ct_tuple_ipv4 */
>>   	OVS_KEY_ATTR_CT_ORIG_TUPLE_IPV6,   /* struct ovs_key_ct_tuple_ipv6 */
>>   	OVS_KEY_ATTR_NSH,       /* Nested set of ovs_nsh_key_* */
>> -	OVS_KEY_ATTR_IPV6_EXTHDRS,  /* struct ovs_key_ipv6_exthdr */
>>   
>> -#ifdef __KERNEL__
>> -	OVS_KEY_ATTR_TUNNEL_INFO,  /* struct ip_tunnel_info */
>> -#endif
>> +	/* User space decided to squat on types 29 and 30.  They are listed
>> +	 * below, but should not be sent to the kernel:
>> +	 *
>> +	 * OVS_KEY_ATTR_PACKET_TYPE,   be32 packet type
>> +	 * OVS_KEY_ATTR_ND_EXTENSIONS, IPv6 Neighbor Discovery extensions
>> +	 *
>> +	 * WARNING: No new types should be added unless they are defined
>> +	 *          for both kernel and user space (no 'ifdef's).  It's hard
>> +	 *          to keep compatibility otherwise. */
>> +	OVS_KEY_ATTR_TUNNEL_INFO = 31,  /* struct ip_tunnel_info.
>> +					   For in-kernel use only. */
>> +	OVS_KEY_ATTR_IPV6_EXTHDRS,  /* struct ovs_key_ipv6_exthdr */
>>   	__OVS_KEY_ATTR_MAX
>>   };

so why not again just flat enum without ifdefs and without values
commented out? even if we leave values in comments like above it doesn't
mean the userspace won't use them by mistake and send to the kernel.
but the kernel will probably ignore as not being used and at least
there won't be a conflict again even if by mistake.. and it's easiest
to read.

>>   
>> diff --git a/net/openvswitch/flow_netlink.c b/net/openvswitch/flow_netlink.c
>> index 8b4124820f7d..315064bada3e 100644
>> --- a/net/openvswitch/flow_netlink.c
>> +++ b/net/openvswitch/flow_netlink.c
>> @@ -346,7 +346,7 @@ size_t ovs_key_attr_size(void)
>>   	/* Whenever adding new OVS_KEY_ FIELDS, we should consider
>>   	 * updating this function.
>>   	 */
>> -	BUILD_BUG_ON(OVS_KEY_ATTR_TUNNEL_INFO != 30);
>> +	BUILD_BUG_ON(OVS_KEY_ATTR_MAX != 32);
>>   
>>   	return    nla_total_size(4)   /* OVS_KEY_ATTR_PRIORITY */
>>   		+ nla_total_size(0)   /* OVS_KEY_ATTR_TUNNEL */
>> ---
>>
>> Thoughts?
>>
>> The same change can be ported to the user-space header, but with
>> types actually defined and not part of the comment.  It may look
>> like this: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpastebin.com%2Fk8UWEZtR&amp;data=04%7C01%7Croid%40nvidia.com%7C3f34544168c14459f44608da01408358%7C43083d15727340c1b7db39efd9ccc17a%7C0%7C0%7C637823673860054963%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&amp;sdata=HYfuir9d21MFFxPibJz%2FppfxsylDMLz0CZIOcSCLQQw%3D&amp;reserved=0  (without IPV6_EXTHDRS yet).
>> For the future, we'll try to find a way to define them in a separate
>> enum or will define them dynamically based on the policy dumped from
>> the currently running kernel. In any case no new userspace-only types
>> should be defined in that enum.
> 

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [ovs-dev] [PATCH net-next v8] net: openvswitch: IPv6: Add IPv6 extension header support
  2022-03-09  7:49                               ` Roi Dayan
@ 2022-03-09 13:43                                 ` Ilya Maximets
  2022-03-09 16:17                                   ` Jakub Kicinski
  0 siblings, 1 reply; 22+ messages in thread
From: Ilya Maximets @ 2022-03-09 13:43 UTC (permalink / raw)
  To: Roi Dayan, Jakub Kicinski
  Cc: i.maximets, Johannes Berg, dev, Toms Atteka, netdev,
	linux-kernel, davem, David Ahern, Jiri Pirko, Pablo Neira Ayuso,
	pshelar

On 3/9/22 08:49, Roi Dayan wrote:
> 
> 
> On 2022-03-08 10:16 PM, Jakub Kicinski wrote:
>> On Tue, 8 Mar 2022 20:33:12 +0100 Ilya Maximets wrote:
>>> On 3/8/22 17:17, Jakub Kicinski wrote:
>>>> On Tue, 8 Mar 2022 15:12:45 +0100 Ilya Maximets wrote:
>>>>> Yes, that is something that I had in mind too.  The only thing that makes
>>>>> me uncomfortable is OVS_KEY_ATTR_TUNNEL_INFO = 30 here.  Even though it
>>>>> doesn't make a lot of difference, I'd better keep the kernel-only attributes
>>>>> at the end of the enumeration.  Is there a better way to handle kernel-only
>>>>> attribute?
>>>>
>>>> My thought was to leave the kernel/userspace only types "behind" to
>>>> avoid perpetuating the same constructs.
>>>>
>>>> Johannes's point about userspace to userspace messages makes the whole
>>>> thing a little less of an aberration.
>>>>
>>>> Is there a reason for the types to be hidden under __KERNEL__?
>>>> Or someone did that in a misguided attempt to save space in attr arrays
>>>> when parsing?
>>>
>>> My impression is that OVS_KEY_ATTR_TUNNEL_INFO was hidden from the
>>> user space just because it's not supposed to ever be used by the
>>> user space application.  Pravin or Jesse should know for sure.
>>
>> Hard to make any assumptions about what user space that takes
>> the liberty of re-defining kernel uAPI types will or will not
>> do ;) The only way to be safe would be to actively reject the
>> attr ID on the kernel side. Unless user space uses the same
>> exact name for something else IMHO hiding the value doesn't
>> afford us any extra protection.
>>
>>>>> I agree with that.
>>>>
>>>> Should we add the user space types to the kernel header and remove the
>>>> ifdef __KERNEL__ around TUNNEL_INFO, then?
>>>
>>> I don't think we need to actually define them, but we may list them
>>> in the comment.  I'm OK with either option though.
>>>
>>> For the removal of ifdef __KERNEL__, that might be a good thing to do.
>>> I'm just not sure what are the best practices here.
>>> We'll need to make some code changes in user space to avoid warnings
>>> about not all the enum members being used in 'switch'es.  But that's
>>> not a problem.
>>
>> Presumably only as the headers are updated? IOW we would not break
>> the build for older sources?

Yep.  OVS doesn't use kernel header directly, so the change will be
needed only when the header in OVS repo is updated.  Current/older
sources will be fine.

>>
>>> If you think that having a flat enum without 'ifdef's is a viable
>>> option from a kernel's point of view, I'm all for it.
>>>
>>> Maybe something like this (only checked that this compiles; 29 and
>>> 30 are correct numbers of these userspace attributes):
>>
>> It's a bit of an uncharted territory, hard to say what's right.
>> It may be a little easier to code up the rejection if we have
>> the types defined (which I think we need to do in
>> __parse_flow_nlattrs()? seems OvS does its own nla parsing?)

Ack.  And, yes, __parse_flow_nlattrs() seems to be the right spot.
OVS has lots of nested attributes with some special treatment in a
few cases and dependency tracking, AFAICT, so it parses attributes
on it's own.  Is there a better way?

>>
>> Johannes, any preference?
>>
>>> diff --git a/include/uapi/linux/openvswitch.h b/include/uapi/linux/openvswitch.h
>>> index 9d1710f20505..86bc951be5bc 100644
>>> --- a/include/uapi/linux/openvswitch.h
>>> +++ b/include/uapi/linux/openvswitch.h
>>> @@ -351,11 +351,19 @@ enum ovs_key_attr {
>>>       OVS_KEY_ATTR_CT_ORIG_TUPLE_IPV4,   /* struct ovs_key_ct_tuple_ipv4 */
>>>       OVS_KEY_ATTR_CT_ORIG_TUPLE_IPV6,   /* struct ovs_key_ct_tuple_ipv6 */
>>>       OVS_KEY_ATTR_NSH,       /* Nested set of ovs_nsh_key_* */
>>> -    OVS_KEY_ATTR_IPV6_EXTHDRS,  /* struct ovs_key_ipv6_exthdr */
>>>   -#ifdef __KERNEL__
>>> -    OVS_KEY_ATTR_TUNNEL_INFO,  /* struct ip_tunnel_info */
>>> -#endif
>>> +    /* User space decided to squat on types 29 and 30.  They are listed
>>> +     * below, but should not be sent to the kernel:
>>> +     *
>>> +     * OVS_KEY_ATTR_PACKET_TYPE,   be32 packet type
>>> +     * OVS_KEY_ATTR_ND_EXTENSIONS, IPv6 Neighbor Discovery extensions
>>> +     *
>>> +     * WARNING: No new types should be added unless they are defined
>>> +     *          for both kernel and user space (no 'ifdef's).  It's hard
>>> +     *          to keep compatibility otherwise. */
>>> +    OVS_KEY_ATTR_TUNNEL_INFO = 31,  /* struct ip_tunnel_info.
>>> +                       For in-kernel use only. */
>>> +    OVS_KEY_ATTR_IPV6_EXTHDRS,  /* struct ovs_key_ipv6_exthdr */
>>>       __OVS_KEY_ATTR_MAX
>>>   };
> 
> so why not again just flat enum without ifdefs and without values
> commented out? even if we leave values in comments like above it doesn't
> mean the userspace won't use them by mistake and send to the kernel.
> but the kernel will probably ignore as not being used and at least
> there won't be a conflict again even if by mistake.. and it's easiest
> to read.

OK.  Seems like we have some votes and a reason (explicit reject) to have
them defined.  This will also make current user space and kernel definitions
equal.  Let me put together a patch and we'll continue the discussion there.

> 
>>>   diff --git a/net/openvswitch/flow_netlink.c b/net/openvswitch/flow_netlink.c
>>> index 8b4124820f7d..315064bada3e 100644
>>> --- a/net/openvswitch/flow_netlink.c
>>> +++ b/net/openvswitch/flow_netlink.c
>>> @@ -346,7 +346,7 @@ size_t ovs_key_attr_size(void)
>>>       /* Whenever adding new OVS_KEY_ FIELDS, we should consider
>>>        * updating this function.
>>>        */
>>> -    BUILD_BUG_ON(OVS_KEY_ATTR_TUNNEL_INFO != 30);
>>> +    BUILD_BUG_ON(OVS_KEY_ATTR_MAX != 32);
>>>         return    nla_total_size(4)   /* OVS_KEY_ATTR_PRIORITY */
>>>           + nla_total_size(0)   /* OVS_KEY_ATTR_TUNNEL */
>>> ---
>>>
>>> Thoughts?
>>>
>>> The same change can be ported to the user-space header, but with
>>> types actually defined and not part of the comment.  It may look
>>> like this: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpastebin.com%2Fk8UWEZtR&amp;data=04%7C01%7Croid%40nvidia.com%7C3f34544168c14459f44608da01408358%7C43083d15727340c1b7db39efd9ccc17a%7C0%7C0%7C637823673860054963%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&amp;sdata=HYfuir9d21MFFxPibJz%2FppfxsylDMLz0CZIOcSCLQQw%3D&amp;reserved=0  (without IPV6_EXTHDRS yet).
>>> For the future, we'll try to find a way to define them in a separate
>>> enum or will define them dynamically based on the policy dumped from
>>> the currently running kernel. In any case no new userspace-only types
>>> should be defined in that enum.
>>


^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [ovs-dev] [PATCH net-next v8] net: openvswitch: IPv6: Add IPv6 extension header support
  2022-03-09 13:43                                 ` Ilya Maximets
@ 2022-03-09 16:17                                   ` Jakub Kicinski
  0 siblings, 0 replies; 22+ messages in thread
From: Jakub Kicinski @ 2022-03-09 16:17 UTC (permalink / raw)
  To: Ilya Maximets
  Cc: Roi Dayan, Johannes Berg, dev, Toms Atteka, netdev, linux-kernel,
	davem, David Ahern, Jiri Pirko, Pablo Neira Ayuso, pshelar

On Wed, 9 Mar 2022 14:43:07 +0100 Ilya Maximets wrote:
> >> It's a bit of an uncharted territory, hard to say what's right.
> >> It may be a little easier to code up the rejection if we have
> >> the types defined (which I think we need to do in
> >> __parse_flow_nlattrs()? seems OvS does its own nla parsing?)  
> 
> Ack.  And, yes, __parse_flow_nlattrs() seems to be the right spot.
> OVS has lots of nested attributes with some special treatment in a
> few cases and dependency tracking, AFAICT, so it parses attributes
> on it's own.  Is there a better way?

Looks like OvS has extra requirements like attributes can't be
duplicated and zeroed out attrs are ignored. I don't think generic
parsers can do that today, although the former seems like a useful
addition.

A problem for another time.

> >> Johannes, any preference?
> >
> > so why not again just flat enum without ifdefs and without values
> > commented out? even if we leave values in comments like above it doesn't
> > mean the userspace won't use them by mistake and send to the kernel.
> > but the kernel will probably ignore as not being used and at least
> > there won't be a conflict again even if by mistake.. and it's easiest
> > to read.  
> 
> OK.  Seems like we have some votes and a reason (explicit reject) to have
> them defined.  This will also make current user space and kernel definitions
> equal.  Let me put together a patch and we'll continue the discussion there.

Thanks!

^ permalink raw reply	[flat|nested] 22+ messages in thread

end of thread, other threads:[~2022-03-09 16:32 UTC | newest]

Thread overview: 22+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-02-24  0:54 [PATCH net-next v8] net: openvswitch: IPv6: Add IPv6 extension header support Toms Atteka
2022-02-25 10:40 ` patchwork-bot+netdevbpf
2022-03-02 10:03   ` [ovs-dev] " Roi Dayan
2022-03-02 10:50     ` Roi Dayan
2022-03-02 13:59       ` Eelco Chaudron
2022-03-07  8:49         ` Roi Dayan
2022-03-07 20:26           ` Jakub Kicinski
2022-03-07 22:14             ` Ilya Maximets
2022-03-07 22:46               ` Jakub Kicinski
2022-03-08  0:04                 ` Ilya Maximets
2022-03-08  5:45                   ` Jakub Kicinski
2022-03-08  8:21                     ` Johannes Berg
2022-03-08 14:12                       ` Ilya Maximets
2022-03-08 14:39                         ` Roi Dayan
2022-03-08 18:25                           ` Ilya Maximets
2022-03-08 20:17                             ` Jakub Kicinski
2022-03-08 16:17                         ` Jakub Kicinski
2022-03-08 19:33                           ` Ilya Maximets
2022-03-08 20:16                             ` Jakub Kicinski
2022-03-09  7:49                               ` Roi Dayan
2022-03-09 13:43                                 ` Ilya Maximets
2022-03-09 16:17                                   ` Jakub Kicinski

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.