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. _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev