On 7 Apr 2022, at 12:22, Ilya Maximets wrote:
> On 4/7/22 10:02, Vlad Buslov wrote: >> On Mon 14 Mar 2022 at 20:40, Ilya Maximets <[email protected]> wrote: >>> On 3/14/22 19:33, Roi Dayan wrote: >>>> >>>> >>>> On 2022-03-10 8:44 PM, Aaron Conole wrote: >>>>> Ilya Maximets <[email protected]> writes: >>>>> >>>>>> 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: Aaron Conole <[email protected]> >>>>> >>>> >>>> >>>> >>>> I got to check traffic with the fix and I do get some traffic >>>> but something is broken. I didn't investigate much but the quick >>>> test shows me rules are not offloaded and dumping ovs rules gives >>>> error like this >>>> >>>> recirc_id(0),in_port(enp8s0f0_1),ct_state(-trk),eth(),eth_type(0x86dd),ipv6(frag=no)(bad >>>> key length 2, expected -1)(00 00/(bad mask length 2, expected -1)(00 00), >>>> packets:2453, bytes:211594, used:0.004s, flags:S., actions:ct,recirc(0x2) >>> >>> Such a dump is expected, because kernel parses fields that current >>> userspace doesn't understand, and at the same time OVS by design is >>> using kernel provided key/mask while installing datapath rules, IIRC. >>> It should be possible to make these dumps a bit more friendly though. >>> >>> For the offloading not working, see my comment in the v2 patch email >>> I sent (top email of this thread). In short, it's a problem in user >>> space and it can not be fixed from the kernel side, unless we revert >>> IPv6 extension header support and never add any new types, which is >>> unreasonable. I didn't test any actual offloading, but I had a >>> successful run of 'make check-offloads' with my quick'n'dirty fix from >>> the top email. >> >> Hi Ilya, >> >> I can confirm that with latest OvS master IPv6 rules offload still fails >> without your pastebin code applied. >> >>> >>> Since we're here: >>> >>> Toms, do you plan to submit user space patches for this feature? >> >> I see there is a patch from you that is supposed to fix compatibility >> issues caused by this change in OvS d96d14b14733 ("openvswitch.h: Align >> uAPI definition with the kernel."), but it doesn't fix offload for me >> without pastebin patch. > > Yes. OVS commit d96d14b14733 is intended to only fix the uAPI. > Issue with offload is an OVS bug that should be fixed separately. > The fix will also need to be backported to OVS stable branches. > >> Do you plan to merge that code into OvS or you >> require some help from our side? > > I could do that, but I don't really have enough time. So, if you > can work on that fix, it would be great. Note that comments inside > the OVS's lib/odp-util.c:parse_key_and_mask_to_match() was blindly > copied from the userspace datapath and are incorrect for the general > case, so has to be fixed alongside the logic of that function. Tom or Vlad, are you working on this? Asking, as the release of a kernel with Tom’s “net: openvswitch: IPv6: Add IPv6 extension header support” patch will break OVS. //Eelco _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
