Message-ID: <4778B505-DBF5-4F57-90AF-87F12C1E0311@redhat.com>
Date:   Thu, 12 May 2022 12:19:23 +0200
From:   Eelco Chaudron <echaudro@...hat.com>
To:     Vlad Buslov <vladbu@...dia.com>,
        Toms Atteka <cpp.code.lv@...il.com>
Cc:     Roi Dayan <roid@...dia.com>, Ilya Maximets <i.maximets@....org>,
        Aaron Conole <aconole@...hat.com>,
        Jakub Kicinski <kuba@...nel.org>,
        "David S. Miller" <davem@...emloft.net>,
        Pravin B Shelar <pshelar@....org>, netdev@...r.kernel.org,
        dev@...nvswitch.org, linux-kernel@...r.kernel.org,
        Johannes Berg <johannes@...solutions.net>,
        Maor Dickman <maord@...dia.com>
Subject: Re: [PATCH net-next v2] net: openvswitch: fix uAPI incompatibility
 with existing user space



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.maximets@....org> wrote:
>>> On 3/14/22 19:33, Roi Dayan wrote:
>>>>
>>>>
>>>> On 2022-03-10 8:44 PM, Aaron Conole wrote:
>>>>> Ilya Maximets <i.maximets@....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-6d8a0cad8a5f@nvidia.com
>>>>>> Link: https://github.com/openvswitch/ovs/commit/beb75a40fdc295bfd6521b0068b4cd12f6de507c
>>>>>> Reported-by: Roi Dayan <roid@...dia.com>
>>>>>> Signed-off-by: Ilya Maximets <i.maximets@....org>
>>>>>> ---
>>>>>
>>>>> Acked-by: Aaron Conole <aconole@...hat.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

Powered by blists - more mailing lists