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

[Ilya Maximets <i.maximets@ovn.org>]

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.