Re: [ovs-dev] [PATCH net-next v8] net: openvswitch: IPv6: Add IPv6 extension header support

[Johannes Berg <johannes@sipsolutions.net>]

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