Messages in this thread | | | Subject | Re: [ovs-dev] [PATCH net-next v8] net: openvswitch: IPv6: Add IPv6 extension header support | From | Johannes Berg <> | Date | Tue, 08 Mar 2022 09:21:18 +0100 |
| |
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
| |