Re: [PATCH net-next v2] net: openvswitch: fix uAPI incompatibility with existing user space

[Vlad Buslov]

On Mon 14 Mar 2022 at 20:40, Ilya Maximets <i.maximets@xxxxxxx> wrote:
> On 3/14/22 19:33, Roi Dayan wrote:
>> 
>> 
>> On 2022-03-10 8:44 PM, Aaron Conole wrote:
>>> Ilya Maximets <i.maximets@xxxxxxx> 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@xxxxxxxxxx
>>>> Link: https://github.com/openvswitch/ovs/commit/beb75a40fdc295bfd6521b0068b4cd12f6de507c
>>>> Reported-by: Roi Dayan <roid@xxxxxxxxxx>
>>>> Signed-off-by: Ilya Maximets <i.maximets@xxxxxxx>
>>>> ---
>>>
>>> Acked-by: Aaron Conole <aconole@xxxxxxxxxx>
>>>
>> 
>> 
>> 
>> 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. Do you plan to merge that code into OvS or you
require some help from our side?

Regards,
Vlad

[...]