From: Nicolas Dichtel <nicolas.dichtel@6wind.com>
To: Cpp Code <cpp.code.lv@gmail.com>
Cc: Jakub Kicinski <kuba@kernel.org>,
netdev@vger.kernel.org, pshelar@ovn.org,
"David S. Miller" <davem@davemloft.net>,
ovs dev <dev@openvswitch.org>,
linux-kernel@vger.kernel.org
Subject: Re: [PATCH net-next v6] net: openvswitch: IPv6: Add IPv6 extension header support
Date: Tue, 5 Oct 2021 08:41:03 +0200 [thread overview]
Message-ID: <a4894aef-b82a-8224-611d-07be229f5ebe@6wind.com> (raw)
In-Reply-To: <CAASuNyW81zpSu+FGSDuUrOsyqJj7SokZtvX081BbeXi0ARBaYg@mail.gmail.com>
Le 01/10/2021 à 22:42, Cpp Code a écrit :
> On Fri, Oct 1, 2021 at 12:21 AM Nicolas Dichtel
> <nicolas.dichtel@6wind.com> wrote:
>>
>> Le 30/09/2021 à 18:11, Cpp Code a écrit :
>>> On Wed, Sep 29, 2021 at 6:19 AM Jakub Kicinski <kuba@kernel.org> wrote:
>>>>
>>>> On Wed, 29 Sep 2021 08:19:05 +0200 Nicolas Dichtel wrote:
>>>>>> /* Insert a kernel only KEY_ATTR */
>>>>>> #define OVS_KEY_ATTR_TUNNEL_INFO __OVS_KEY_ATTR_MAX
>>>>>> #undef OVS_KEY_ATTR_MAX
>>>>>> #define OVS_KEY_ATTR_MAX __OVS_KEY_ATTR_MAX
>>>>> Following the other thread [1], this will break if a new app runs over an old
>>>>> kernel.
>>>>
>>>> Good point.
>>>>
>>>>> Why not simply expose this attribute to userspace and throw an error if a
>>>>> userspace app uses it?
>>>>
>>>> Does it matter if it's exposed or not? Either way the parsing policy
>>>> for attrs coming from user space should have a reject for the value.
>>>> (I say that not having looked at the code, so maybe I shouldn't...)
>>>
>>> To remove some confusion, there are some architectural nuances if we
>>> want to extend code without large refactor.
>>> The ovs_key_attr is defined only in kernel side. Userspace side is
>>> generated from this file. As well the code can be built without kernel
>>> modules.
>>> The code inside OVS repository and net-next is not identical, but I
>>> try to keep some consistency.
>> I didn't get why OVS_KEY_ATTR_TUNNEL_INFO cannot be exposed to userspace.
>
> OVS_KEY_ATTR_TUNNEL_INFO is compressed version of OVS_KEY_ATTR_TUNNEL
> and for clarity purposes its not exposed to userspace as it will never
> use it.
> I would say it's a coding style as it would not brake anything if exposed.
In fact, it's the best way to keep the compatibility in the long term.
You can define it like this:
OVS_KEY_ATTR_TUNNEL_INFO, /* struct ip_tunnel_info, reserved for kernel use */
>
>>
>>>
>>> JFYI This is the file responsible for generating userspace part:
>>> https://github.com/openvswitch/ovs/blob/master/build-aux/extract-odp-netlink-h
>>> This is the how corresponding file for ovs_key_attr looks inside OVS:
>>> https://github.com/openvswitch/ovs/blob/master/datapath/linux/compat/include/linux/openvswitch.h
>>> one can see there are more values than in net-next version.
>> There are still some '#ifdef __KERNEL__'. The standard 'make headers_install'
>> filters them. Why not using this standard mechanism?
>
> Could you elaborate on this, I don't quite understand the idea!? Which
> ifdef you are referring, the one along OVS_KEY_ATTR_TUNNEL_INFO or
> some other?
My understanding is that this file is used for the userland third party, thus,
theoretically, there should be no '#ifdef __KERNEL__'. uapi headers generated
with 'make headers_install' are filtered to remove them.
>
>>
>> In this file, there are two attributes (OVS_KEY_ATTR_PACKET_TYPE and
>> OVS_KEY_ATTR_ND_EXTENSIONS) that doesn't exist in the kernel.
>> This will also breaks if an old app runs over a new kernel. I don't see how it
>> is possible to keep the compat between {old|new} {kernel|app}.
>
> Looks like this most likely is a bug while working on multiple
> versions of code. Need to do add more padding.
As said above, just define the same uapi for everybody and the problem is gone
forever.
Regards,
Nicolas
next prev parent reply other threads:[~2021-10-05 6:41 UTC|newest]
Thread overview: 11+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-09-28 19:47 [PATCH net-next v6] net: openvswitch: IPv6: Add IPv6 extension header support Toms Atteka
2021-09-29 0:48 ` Jakub Kicinski
2021-09-29 6:19 ` Nicolas Dichtel
2021-09-29 13:19 ` Jakub Kicinski
2021-09-30 16:11 ` Cpp Code
2021-10-01 7:21 ` Nicolas Dichtel
2021-10-01 20:42 ` Cpp Code
2021-10-05 6:41 ` Nicolas Dichtel [this message]
2021-10-14 21:12 ` Cpp Code
2021-10-15 13:52 ` [ovs-dev] " Ilya Maximets
2021-09-30 16:12 ` Cpp Code
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=a4894aef-b82a-8224-611d-07be229f5ebe@6wind.com \
--to=nicolas.dichtel@6wind.com \
--cc=cpp.code.lv@gmail.com \
--cc=davem@davemloft.net \
--cc=dev@openvswitch.org \
--cc=kuba@kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=netdev@vger.kernel.org \
--cc=pshelar@ovn.org \
/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).