Received-SPF: softfail (google.com: domain of transitioning linux-kernel-owner@vger.kernel.org does not designate 23.128.96.19 as permitted sender) client-ip=23.128.96.19;
Received-SPF: Pass (protection.outlook.com: domain of nvidia.com designates
 12.22.5.238 as permitted sender) receiver=protection.outlook.com;
 client-ip=12.22.5.238; helo=mail.nvidia.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>
User-agent: mu4e 1.6.6; emacs 27.2
From:   Vlad Buslov <vladbu@nvidia.com>
To:     Eelco Chaudron <echaudro@redhat.com>
CC:     Toms Atteka <cpp.code.lv@gmail.com>, 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:   Thu, 12 May 2022 13:08:14 +0300
In-Reply-To: <4778B505-DBF5-4F57-90AF-87F12C1E0311@redhat.com>
Message-ID: <87lev783k8.fsf@nvidia.com>
MIME-Version: 1.0
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: quoted-printable
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 12 May 2022 11:10:21.7256
 (UTC)
X-MS-Exchange-CrossTenant-Network-Message-Id: 3304a42f-b778-43eb-77ba-08da340801a5
X-MS-Exchange-CrossTenant-Id: 43083d15-7273-40c1-b7db-39efd9ccc17a
X-MS-Exchange-CrossTenant-OriginalAttributedTenantConnectingIp: TenantId=43083d15-7273-40c1-b7db-39efd9ccc17a;Ip=[12.22.5.238];Helo=[mail.nvidia.com]
X-MS-Exchange-CrossTenant-AuthSource: CO1NAM11FT029.eop-nam11.prod.protection.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Anonymous
X-MS-Exchange-CrossTenant-FromEntityHeader: HybridOnPrem
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BL3PR12MB6547
X-Spam-Status: No, score=-2.6 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
	DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,
	MAILING_LIST_MULTI,RDNS_NONE,SPF_HELO_NONE,T_SCC_BODY_TEXT_LINE
	autolearn=unavailable autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
	lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org


On Thu 12 May 2022 at 12:19, Eelco Chaudron <echaudro@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.maximets@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.maximets@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.=C2=A0 '#ifndef __KERNEL__' and another attribut=
e was
>>>>>>> added later.
>>>>>>>
>>>>>>> This leads to the inevitable clash between user space and kernel ty=
pes
>>>>>>> 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 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.=C2=A0 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.=C2=A0 Strictly speaking this is not the pr=
oblem
>>>>>>> of the kernel uAPI, but changing it is the only way to avoid breaka=
ge
>>>>>>> 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.=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 overflow on
>>>>>>> OVS_KEY_ATTR_IPV6_EXTHDRS, since it equals 32 now.
>>>>>>>
>>>>>>> =C2=A0 [1] beb75a40fdc2 ("userspace: Switching of L3 packets in L2 =
pipeline")
>>>>>>>
>>>>>>> Fixes: 28a3f0601727 ("net: openvswitch: IPv6: Add IPv6 extension he=
ader support")
>>>>>>> Link: https://lore.kernel.org/netdev/3adf00c7-fe65-3ef4-b6d7-6d8a0c=
ad8a5f@nvidia.com
>>>>>>> Link: https://github.com/openvswitch/ovs/commit/beb75a40fdc295bfd65=
21b0068b4cd12f6de507c
>>>>>>> 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 gives
>>>>> error like this
>>>>>
>>>>> recirc_id(0),in_port(enp8s0f0_1),ct_state(-trk),eth(),eth_type(0x86dd=
),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,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=E2=80=99s =E2=80=9Cnet: openvswitch: IPv6: Add IPv6 extension header =
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.

Regards,
Vlad