On 17 May 2022, at 13:10, Eelco Chaudron wrote:
> On 12 May 2022, at 12:08, Vlad Buslov wrote:
>
>> On Thu 12 May 2022 at 12:19, Eelco Chaudron <echau...@redhat.com> wrote:
>>> 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 <i.maxim...@ovn.org> wrote:
>>>>>> On 3/14/22 19:33, Roi Dayan wrote:
>>>>>>>
>>>>>>>
>>>>>>> On 2022-03-10 8:44 PM, Aaron Conole wrote:
>>>>>>>> Ilya Maximets <i.maxim...@ovn.org> 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/3adf00c7-fe65-3ef4-b6d7-6d8a0cad8...@nvidia.com
>>>>>>>>> Link:
>>>>>>>>> https://github.com/openvswitch/ovs/commit/beb75a40fdc295bfd6521b0068b4cd12f6de507c
>>>>>>>>> Reported-by: Roi Dayan <r...@nvidia.com>
>>>>>>>>> Signed-off-by: Ilya Maximets <i.maxim...@ovn.org>
>>>>>>>>> ---
>>>>>>>>
>>>>>>>> Acked-by: Aaron Conole <acon...@redhat.com>
>>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> 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
>>
>> Hi Eelco,
>>
>> My simple fix for OvS was rejected and I don't have time to rework it at
>> the moment.
>
> That’s a pity, Tom do you maybe have time as your patch left OVS in this
> error state?
Looks like everybody is busy, and as the patched kernel is now available, let
me try to fix this on the OVS side.
//Eelco
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
