On 3/7/22 21:26, Jakub Kicinski wrote:
> On Mon, 7 Mar 2022 10:49:31 +0200 Roi Dayan wrote:
>>>> 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.
>>
>> 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?
Hi, Jakub.
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.
Sorry for the mess.
Best regards, Ilya Maximets.
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.