Date:   Mon, 7 Mar 2022 23:14:13 +0100
From:   Ilya Maximets <i.maximets@....org>
To:     Jakub Kicinski <kuba@...nel.org>, Roi Dayan <roid@...dia.com>
Cc:     dev@...nvswitch.org, Toms Atteka <cpp.code.lv@...il.com>,
        netdev@...r.kernel.org, linux-kernel@...r.kernel.org,
        davem@...emloft.net, i.maximets@....org
Subject: Re: [ovs-dev] [PATCH net-next v8] net: openvswitch: IPv6: Add IPv6
 extension header support

On 3/7/22 21:26, Jakub Kicinski wrote:
> On Mon, 7 Mar 2022 10:49:31 +0200 Roi Dayan wrote:
>>>> I think there is a missing userspace fix. didnt verify yet.
>>>> but in ovs userspace odp-netlink.h created from datapath/linux/compat/include/linux/openvswitch.h
>>>> and that file is not synced the change here.
>>>> So the new enum OVS_KEY_ATTR_IPV6_EXTHDRS is missing and also struct
>>>> ovs_key_ipv6_exthdrs which is needed in lib/udp-util.c
>>>> in struct ovs_flow_key_attr_lens to add expected len for
>>>> OVS_KEY_ATTR_IPV6_EXTHDR.  
>>>
>>> I guess if this is creating backward compatibility issues, this
>>> patch should be reverted/fixed. As a kmod upgrade should not break
>>> existing deployments. 
>>
>> it looks like it does. we can't work with ovs without reverting this.
>> can we continue with reverting this commit please?
> 
> Sure, can someone ELI5 what the problem is?
> 
> What's "kmod upgrade" in this context a kernel upgrade or loading 
> a newer module in older kernel? 
> 
> How can adding a new nl attr break user space? Does the user space
> actually care about the OVS_KEY_ATTR_TUNNEL_INFO wart?

Hi, Jakub.

The main problem is that userspace uses the modified copy of the uapi header
which looks like this:
  https://github.com/openvswitch/ovs/blob/f77dbc1eb2da2523625cd36922c6fccfcb3f3eb7/datapath/linux/compat/include/linux/openvswitch.h#L357

In short, the userspace view:

  enum ovs_key_attr {
      <common attrs>

  #ifdef __KERNEL__
      /* Only used within kernel data path. */
  #endif

  #ifndef __KERNEL__
      /* Only used within userspace data path. */
  #endif
      __OVS_KEY_ATTR_MAX
};

And the kernel view:

  enum ovs_key_attr {
      <common attrs>

  #ifdef __KERNEL__
      /* Only used within kernel data path. */
  #endif

      __OVS_KEY_ATTR_MAX
  };

This happened before my time, but the commit where userspace made a wrong
turn appears to be this one:
  https://github.com/openvswitch/ovs/commit/beb75a40fdc295bfd6521b0068b4cd12f6de507c
The attribute for userspace only was added to the common enum after the
OVS_KEY_ATTR_TUNNEL_INFO.   I'm not sure how things didn't fall apart when
OVS_KEY_ATTR_NSH was added later (no-one cared that NSH doesn't work, because
OVS didn't support it yet?).

In general, any addition of a new attribute into that enumeration leads to
inevitable clash between userpsace-only attributes and new kernel attributes.

After the kernel update, kernel provides new attributes to the userspace and
userspace tries to parse them as one of the userspace-only attributes and
fails.   In our current case userspace is trying to parse OVS_KEY_ATTR_IPV6_EXTHDR
as userspace-only OVS_KEY_ATTR_PACKET_TYPE, because they have the same value in the
enum, fails and discards the netlink message as malformed.  So, IPv6 is fully
broken, because OVS_KEY_ATTR_IPV6_EXTHDR is supplied now with every IPv6 packet
that goes to userspace.

We need to unify the view of 'enum ovs_key_attr' between userspace and kernel
before we can add any new values to it.

One way to do that should be addition of both userspace-only attributes to the
kernel header (and maybe exposing OVS_KEY_ATTR_TUNNEL_INFO too, just to keep
it flat and avoid any possible problems in the future).  Any other suggestions
are welcome.  But in any case this will require careful testing with existing
OVS userspace to avoid any unexpected issues.

Moving forward, I think, userspace OVS should find a way to not have userpsace-only
attributes, or have them as a separate enumeration.  But I'm not sure how to do
that right now.  Or we'll have to add userspace-only attributes to the kernel
uapi before using them.

Sorry for the mess.

Best regards, Ilya Maximets.

Powered by blists - more mailing lists