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

From: Jakub Kicinski <kuba@kernel.org>
To: Ilya Maximets <i.maximets@ovn.org>
Cc: Roi Dayan <roid@nvidia.com>,
	dev@openvswitch.org, Toms Atteka <cpp.code.lv@gmail.com>,
	netdev@vger.kernel.org, linux-kernel@vger.kernel.org,
	davem@davemloft.net
Subject: Re: [ovs-dev] [PATCH net-next v8] net: openvswitch: IPv6: Add IPv6 extension header support
Date: Mon, 7 Mar 2022 14:46:16 -0800	[thread overview]
Message-ID: <20220307144616.05317297@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com> (raw)
In-Reply-To: <3a96b606-c3aa-c39b-645e-a3af0c82e44b@ovn.org>

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.