Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   Eelco Chaudron <echaudro@redhat.com>
To:     Vlad Buslov <vladbu@nvidia.com>,
        Toms Atteka <cpp.code.lv@gmail.com>
Cc:     Roi Dayan <roid@nvidia.com>, Ilya Maximets <i.maximets@ovn.org>,
        Aaron Conole <aconole@redhat.com>,
        Jakub Kicinski <kuba@kernel.org>,
        "David S. Miller" <davem@davemloft.net>,
        Pravin B Shelar <pshelar@ovn.org>, netdev@vger.kernel.org,
        dev@openvswitch.org, linux-kernel@vger.kernel.org,
        Johannes Berg <johannes@sipsolutions.net>,
        Maor Dickman <maord@nvidia.com>
Subject: Re: [PATCH net-next v2] net: openvswitch: fix uAPI incompatibility
 with existing user space
Date:   Tue, 31 May 2022 16:39:47 +0200
Message-ID: <0DEFEF9E-96A0-4B65-A6A3-611EBAC97F6A@redhat.com>
In-Reply-To: <CD6F1EAA-7A32-46D2-9806-98CDB98DC537@redhat.com>
References: <20220309222033.3018976-1-i.maximets@ovn.org>
 <f7ty21hir5v.fsf@redhat.com>
 <44eeb550-3310-d579-91cc-ec18b59966d2@nvidia.com>
 <1a185332-3693-2750-fef2-f6938bbc8500@ovn.org> <87k0c171ml.fsf@nvidia.com>
 <9cc34fbc-3fd6-b529-7a05-554224510452@ovn.org>
 <4778B505-DBF5-4F57-90AF-87F12C1E0311@redhat.com> <87lev783k8.fsf@nvidia.com>
 <FFBEB52B-FA8C-4989-BDC1-1F3908F024B8@redhat.com>
 <CD6F1EAA-7A32-46D2-9806-98CDB98DC537@redhat.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: quoted-printable
Precedence: bulk


On 23 May 2022, at 14:54, Eelco Chaudron wrote:

> 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 <echaudro@redhat.com> wro=
te:
>>>> 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@ovn.org> wr=
ote:
>>>>>>> On 3/14/22 19:33, Roi Dayan wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>> On 2022-03-10 8:44 PM, Aaron Conole wrote:
>>>>>>>>> Ilya Maximets <i.maximets@ovn.org> writes:
>>>>>>>>>
>>>>>>>>>> Few years ago OVS user space made a strange choice in the comm=
it [1]
>>>>>>>>>> to define types only valid for the user space inside the copy =
of a
>>>>>>>>>> kernel uAPI header.=C2=A0 '#ifndef __KERNEL__' and another att=
ribute was
>>>>>>>>>> added later.
>>>>>>>>>>
>>>>>>>>>> This leads to the inevitable clash between user space and kern=
el types
>>>>>>>>>> when the kernel uAPI is extended.=C2=A0 The issue was unveiled=
 with the
>>>>>>>>>> addition of a new type for IPv6 extension header in kernel uAP=
I.
>>>>>>>>>>
>>>>>>>>>> When kernel provides the OVS_KEY_ATTR_IPV6_EXTHDRS attribute t=
o the
>>>>>>>>>> older user space application, application tries to parse it as=

>>>>>>>>>> OVS_KEY_ATTR_PACKET_TYPE and discards the whole netlink messag=
e as
>>>>>>>>>> malformed.=C2=A0 Since OVS_KEY_ATTR_IPV6_EXTHDRS is supplied a=
long 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 ker=
nel
>>>>>>>>>> uAPI to avoid the clash.=C2=A0 Strictly speaking this is not t=
he problem
>>>>>>>>>> of the kernel uAPI, but changing it is the only way to avoid b=
reakage
>>>>>>>>>> of the older user space applications at this point.
>>>>>>>>>>
>>>>>>>>>> These 2 types are explicitly rejected now since they should no=
t be
>>>>>>>>>> passed to the kernel.=C2=A0 Additionally, OVS_KEY_ATTR_TUNNEL_=
INFO moved
>>>>>>>>>> out from the '#ifdef __KERNEL__' as there is no good reason to=
 hide
>>>>>>>>>> it from the userspace.=C2=A0 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 overf=
low on
>>>>>>>>>> OVS_KEY_ATTR_IPV6_EXTHDRS, since it equals 32 now.
>>>>>>>>>>
>>>>>>>>>> =C2=A0 [1] beb75a40fdc2 ("userspace: Switching of L3 packets i=
n L2 pipeline")
>>>>>>>>>>
>>>>>>>>>> Fixes: 28a3f0601727 ("net: openvswitch: IPv6: Add IPv6 extensi=
on header support")
>>>>>>>>>> Link: https://lore.kernel.org/netdev/3adf00c7-fe65-3ef4-b6d7-6=
d8a0cad8a5f@nvidia.com
>>>>>>>>>> Link: https://github.com/openvswitch/ovs/commit/beb75a40fdc295=
bfd6521b0068b4cd12f6de507c
>>>>>>>>>> Reported-by: Roi Dayan <roid@nvidia.com>
>>>>>>>>>> Signed-off-by: Ilya Maximets <i.maximets@ovn.org>
>>>>>>>>>> ---
>>>>>>>>>
>>>>>>>>> Acked-by: Aaron Conole <aconole@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 give=
s
>>>>>>>> error like this
>>>>>>>>
>>>>>>>> recirc_id(0),in_port(enp8s0f0_1),ct_state(-trk),eth(),eth_type(0=
x86dd),ipv6(frag=3Dno)(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,re=
circ(0x2)
>>>>>>>
>>>>>>> Such a dump is expected, because kernel parses fields that curren=
t
>>>>>>> userspace doesn't understand, and at the same time OVS by design =
is
>>>>>>> using kernel provided key/mask while installing datapath rules, I=
IRC.
>>>>>>> It should be possible to make these dumps a bit more friendly tho=
ugh.
>>>>>>>
>>>>>>> For the offloading not working, see my comment in the v2 patch em=
ail
>>>>>>> I sent (top email of this thread).  In short, it's a problem in u=
ser
>>>>>>> space and it can not be fixed from the kernel side, unless we rev=
ert
>>>>>>> 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 compatibil=
ity
>>>>>> 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 genera=
l
>>>>> 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 ke=
rnel with
>>>> Tom=E2=80=99s =E2=80=9Cnet: openvswitch: IPv6: Add IPv6 extension he=
ader support=E2=80=9D 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=E2=80=99s a pity, Tom do you maybe have time as your patch left O=
VS in this error state?
>
> Looks like everybody is busy, and as the patched kernel is now availabl=
e, let me try to fix this on the OVS side.

I posted a patch on the OVS mailing list, please review/verify.

https://patchwork.ozlabs.org/project/openvswitch/patch/165400753240.20546=
05.12815593709246812392.stgit@ebuild/

//Eelco