On 3/25/22 15:23, Aaron Conole wrote: > Ilya Maximets <[email protected]> writes: > >> Upstream commit: >> commit 1926407a4ab0e59d5a27bed7b82029b356d80fa0 >> Author: Ilya Maximets <[email protected]> >> Date: Wed Mar 9 23:20:33 2022 +0100 >> >> net: openvswitch: fix uAPI incompatibility with existing user space >> >> Few years ago OVS user space made a strange choice in the commit [1] >> to define types only valid for the user space inside the copy of a >> kernel uAPI header. '#ifndef __KERNEL__' and another attribute was >> added later. >> >> This leads to the inevitable clash between user space and kernel types >> when the kernel uAPI is extended. The issue was unveiled with the >> addition of a new type for IPv6 extension header in kernel uAPI. >> >> When kernel provides the OVS_KEY_ATTR_IPV6_EXTHDRS attribute to the >> older user space application, application tries to parse it as >> OVS_KEY_ATTR_PACKET_TYPE and discards the whole netlink message as >> malformed. Since OVS_KEY_ATTR_IPV6_EXTHDRS is supplied along with >> every IPv6 packet that goes to the user space, IPv6 support is fully >> broken. >> >> Fixing that by bringing these user space attributes to the kernel >> uAPI to avoid the clash. Strictly speaking this is not the problem >> of the kernel uAPI, but changing it is the only way to avoid breakage >> of the older user space applications at this point. >> >> These 2 types are explicitly rejected now since they should not be >> passed to the kernel. Additionally, OVS_KEY_ATTR_TUNNEL_INFO moved >> out from the '#ifdef __KERNEL__' as there is no good reason to hide >> it from the userspace. And it's also explicitly rejected now, because >> it's for in-kernel use only. >> >> Comments with warnings were added to avoid the problem coming back. >> >> (1 << type) converted to (1ULL << type) to avoid integer overflow on >> OVS_KEY_ATTR_IPV6_EXTHDRS, since it equals 32 now. >> >> [1] beb75a40fdc2 ("userspace: Switching of L3 packets in L2 pipeline") >> >> Fixes: 28a3f0601727 ("net: openvswitch: IPv6: Add IPv6 extension header >> support") >> Link: >> https://lore.kernel.org/netdev/[email protected] >> Link: >> https://github.com/openvswitch/ovs/commit/beb75a40fdc295bfd6521b0068b4cd12f6de507c >> Reported-by: Roi Dayan <[email protected]> >> Signed-off-by: Ilya Maximets <[email protected]> >> Acked-by: Nicolas Dichtel <[email protected]> >> Acked-by: Aaron Conole <[email protected]> >> Link: >> https://lore.kernel.org/r/[email protected] >> Signed-off-by: Jakub Kicinski <[email protected]> >> >> Not adding OVS_KEY_ATTR_IPV6_EXTHDRS in this commit as this is not >> necessary. Will be added along with the actual userspace >> implementation. >> >> This change should help avoiding incompatibility issues in the future. >> >> Signed-off-by: Ilya Maximets <[email protected]> >> --- > > Acked-by: Aaron Conole <[email protected]>
Applied. Thanks! > > Especially with the warning. Maybe we can make a buildtool / checkpatch > enhancement to catch the case where someone adds a new uapi? Needs a > little bit of thought. Might be a good idea. In general, we need to find a different way to define userspace-only arguments. Either define them dynamically (e.g. based on the policy dump from the running kernel), or define them in the separate enum and make sure it's not mixed up with the common one. Best regards, Ilya Maximets. _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
