lkml.org 
[lkml]   [2022]   [Mar]   [7]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [ovs-dev] [PATCH net-next v8] net: openvswitch: IPv6: Add IPv6 extension header support
    On 3/7/22 23:46, Jakub Kicinski wrote:
    > On Mon, 7 Mar 2022 23:14:13 +0100 Ilya Maximets wrote:
    >> 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.
    >
    > Thanks for the explanation, we can apply a revert if that'd help your
    > CI / ongoing development but sounds like the fix really is in user
    > space. Expecting netlink attribute lists not to grow is not fair.

    I don't think it was intentional, just a careless mistake. Unfortunately,
    all OVS binaries built during the last 5 years rely on that unwanted
    expectation (re-build will also not help as they are using a copy of the
    uAPI header and the clash will be there anyway). If we want to keep them
    working, kernel uAPI has to be carefully updated with current userspace-only
    attributes before we add any new ones. That is not great, but I don't see
    any other option right now that doesn't require code changes in userspace.

    I'd say that we need to revert the current patch and re-introduce it
    later when the uAPI problem is sorted out. This way we will avoid blocking
    the net-next testing and will also avoid problems in case the uAPI changes
    are not ready at the moment of the new kernel release.

    What do you think?

    >
    > Since ovs uses genetlink you should be able to dump the policy from
    > the kernel and at least validate that it doesn't overlap.

    That is interesting. Indeed, this functionality can be used to detect
    problems or to define userspace-only attributes in runtime based on the
    kernel reply. Thanks for the pointer!

    Best regards, Ilya Maximets.

    \
     
     \ /
      Last update: 2022-03-08 01:05    [W:7.320 / U:0.252 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site