net: openvswitch: IPv6: Add IPv6 extension header support

Hello:

This patch was applied to netdev/net-next.git (master) by David S. Miller <[email protected]>:

Quoted 11 lines · 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 <[email protected]>

Acked-by Pravin B Shelar <[email protected]>

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

Quoted 18 lines · On 2022-02-25 12:40 PM, [email protected] wrote:

Hello:

This patch was applied to netdev/net-next.git (master) by David S. Miller <[email protected]>:

Quoted 11 lines · 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 <[email protected]>

Acked-by Pravin B Shelar <[email protected]>

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

Quoted 5 lines

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

Quoted 32 lines · Roi Dayan

Quoted 18 lines · On 2022-02-25 12:40 PM, [email protected] wrote:

Hello:

This patch was applied to netdev/net-next.git (master) by David S. Miller <[email protected]>:

Quoted 11 lines · 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 <[email protected]>

Acked-by Pravin B Shelar <[email protected]>

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

Quoted 5 lines

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?

Quoted 6 lines

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.

Quoted 41 lines · Roi Dayan

Quoted 32 lines · Roi Dayan

Quoted 18 lines · On 2022-02-25 12:40 PM, [email protected] wrote:

Hello:

This patch was applied to netdev/net-next.git (master) by David S. Miller <[email protected]>:

Quoted 11 lines · 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 <[email protected]>

Acked-by Pravin B Shelar <[email protected]>

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

Quoted 5 lines

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?

Quoted 6 lines

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

Quoted 7 lines

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.

Quoted 8 lines

Quoted 7 lines

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.

Quoted 3 lines

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?

Quoted 8 lines · On Mon, 7 Mar 2022 10:49:31 +0200 Roi Dayan wrote:

Quoted 8 lines

Quoted 7 lines

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.

Quoted 6 lines

Quoted 3 lines

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?

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.

Quoted 59 lines · 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.

Quoted 4 lines

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.

Quoted 3 lines

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?

Quoted 2 lines

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!

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:

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

index 9d1710f20505..ab6755621e02 100644
@@ -351,11 +351,16 @@ enum ovs_key_attr {
        OVS_KEY_ATTR_CT_ORIG_TUPLE_IPV4,   /* struct ovs_key_ct_tuple_ipv4 */

Quoted 6 lines

Quoted 2 lines

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!

Quoted 3 lines · Jakub Kicinski

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

:)

Quoted 3 lines

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

Agree.

Quoted 13 lines

but if it already happened the only fix is something like:

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

index 9d1710f20505..ab6755621e02 100644
@@ -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 */
 

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

Quoted 6 lines

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

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

Quoted 7 lines

Quoted 7 lines

Quoted 3 lines

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

Quoted 5 lines · Johannes Berg

Quoted 3 lines · Jakub Kicinski

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

Quoted 7 lines

:)

Quoted 3 lines

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.

Quoted 13 lines

Quoted 13 lines

but if it already happened the only fix is something like:

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

index 9d1710f20505..ab6755621e02 100644
@@ -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 */
 

Quoted 6 lines

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

Quoted 6 lines

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

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

Quoted 4 lines

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.

Quoted 8 lines

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.

Quoted 11 lines

Quoted 7 lines

Quoted 7 lines

Quoted 3 lines

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.

Quoted 2 lines

johannes

Quoted 17 lines · Ilya Maximets

Quoted 5 lines · Johannes Berg

Quoted 3 lines · Jakub Kicinski

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

Quoted 7 lines

:)

Quoted 3 lines

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.

Quoted 8 lines

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?

Quoted 3 lines

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?

Quoted 10 lines

Quoted 10 lines

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

index 9d1710f20505..ab6755621e02 100644
@@ -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 */
 

Quoted 6 lines

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

Quoted 6 lines

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

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

Quoted 3 lines

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.

Quoted 10 lines · On Tue, 8 Mar 2022 15:12:45 +0100 Ilya Maximets wrote:

Quoted 10 lines

Quoted 10 lines

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

index 9d1710f20505..ab6755621e02 100644
@@ -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 */
 

Quoted 12 lines

Quoted 6 lines

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

Quoted 6 lines

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

 #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?

Quoted 5 lines

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

Quoted 5 lines

Quoted 3 lines

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?

Quoted 13 lines · Jakub Kicinski

Quoted 5 lines · 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.

Quoted 3 lines

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.

Quoted 4 lines

Quoted 1 line

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

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

index 9d1710f20505..86bc951be5bc 100644
@@ -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
 };
 

net/openvswitch/flow_netlink.c +1 -2

index 8b4124820f7d..315064bada3e 100644
@@ -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.

Quoted 14 lines · On Tue, 8 Mar 2022 20:33:12 +0100 Ilya Maximets wrote:

Quoted 13 lines · Jakub Kicinski

Quoted 5 lines · 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.

Quoted 7 lines

Quoted 3 lines

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.

Quoted 5 lines

Quoted 4 lines

Quoted 1 line

I agree with that.

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

Quoted 8 lines

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?

Quoted 5 lines

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?

Quoted 19 lines

include/uapi/linux/openvswitch.h +6 -4

index 9d1710f20505..86bc951be5bc 100644
@@ -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
+	 *

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

Quoted 8 lines

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

+	 * 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
  };

Quoted 25 lines

net/openvswitch/flow_netlink.c +1 -2

index 8b4124820f7d..315064bada3e 100644
@@ -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.

Quoted 38 lines · Jakub Kicinski

Quoted 14 lines · On Tue, 8 Mar 2022 20:33:12 +0100 Ilya Maximets wrote:

Quoted 13 lines · Jakub Kicinski

Quoted 5 lines · 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.

Quoted 7 lines

Quoted 3 lines

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.

Quoted 8 lines

Quoted 4 lines

Quoted 1 line

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.

Quoted 8 lines

Quoted 5 lines

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?

Quoted 11 lines

Quoted 6 lines

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

Quoted 28 lines

Johannes, any preference?

Quoted 25 lines

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

index 9d1710f20505..86bc951be5bc 100644
@@ -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

Quoted 1 line

Quoted 1 line

  };

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.

Quoted 25 lines

Quoted 24 lines

  diff --git a/net/openvswitch/flow_netlink.c b/net/openvswitch/flow_netlink.c index 8b4124820f7d..315064bada3e 100644

net/openvswitch/flow_netlink.c +1 -1

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

Quoted 4 lines

Quoted 4 lines

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?

Quoted 8 lines

Quoted 1 line

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.