linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
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, 17 May 2022 13:10:40 +0200	[thread overview]
Message-ID: <FFBEB52B-FA8C-4989-BDC1-1F3908F024B8@redhat.com> (raw)
In-Reply-To: <87lev783k8.fsf@nvidia.com>



On 12 May 2022, at 12:08, Vlad Buslov wrote:

> 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.  '#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@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=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?

//Eelco


  reply	other threads:[~2022-05-17 11:11 UTC|newest]

Thread overview: 13+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-03-09 22:20 [PATCH net-next v2] net: openvswitch: fix uAPI incompatibility with existing user space Ilya Maximets
2022-03-10  8:24 ` Nicolas Dichtel
2022-03-10 18:44 ` Aaron Conole
2022-03-14 18:33   ` Roi Dayan
2022-03-14 19:40     ` Ilya Maximets
2022-04-07  8:02       ` Vlad Buslov
2022-04-07 10:22         ` Ilya Maximets
2022-05-12 10:19           ` Eelco Chaudron
2022-05-12 10:08             ` Vlad Buslov
2022-05-17 11:10               ` Eelco Chaudron [this message]
2022-05-23 12:54                 ` Eelco Chaudron
2022-05-31 14:39                   ` Eelco Chaudron
2022-03-11  4:30 ` patchwork-bot+netdevbpf

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=FFBEB52B-FA8C-4989-BDC1-1F3908F024B8@redhat.com \
    --to=echaudro@redhat.com \
    --cc=aconole@redhat.com \
    --cc=cpp.code.lv@gmail.com \
    --cc=davem@davemloft.net \
    --cc=dev@openvswitch.org \
    --cc=i.maximets@ovn.org \
    --cc=johannes@sipsolutions.net \
    --cc=kuba@kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=maord@nvidia.com \
    --cc=netdev@vger.kernel.org \
    --cc=pshelar@ovn.org \
    --cc=roid@nvidia.com \
    --cc=vladbu@nvidia.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).