Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20211014211828.291213-1-cpp.code.lv@gmail.com> <1d0a5e90-b878-61a1-99af-35702b72f2d9@ovn.org>
In-Reply-To: <1d0a5e90-b878-61a1-99af-35702b72f2d9@ovn.org>
From:   Cpp Code <cpp.code.lv@gmail.com>
Date:   Tue, 19 Oct 2021 12:30:22 -0700
Message-ID: <CAASuNyXe6OYPe0Utd4xG1-ZTHZet3sJDi2sxXJ-7RWQ-GaEW2w@mail.gmail.com>
Subject: Re: [ovs-dev] [PATCH net-next v7] net: openvswitch: IPv6: Add IPv6
 extension header support
To:     Ilya Maximets <i.maximets@ovn.org>
Cc:     netdev@vger.kernel.org, pshelar@ovn.org,
        "David S. Miller" <davem@davemloft.net>,
        Jakub Kicinski <kuba@kernel.org>,
        ovs dev <dev@openvswitch.org>, linux-kernel@vger.kernel.org,
        Nicolas Dichtel <nicolas.dichtel@6wind.com>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Fri, Oct 15, 2021 at 6:56 AM Ilya Maximets <i.maximets@ovn.org> wrote:
>
> On 10/14/21 23:18, Toms Atteka wrote:
> > This change adds a new OpenFlow field OFPXMT_OFB_IPV6_EXTHDR and
> > packets can be filtered using ipv6_ext flag.
> >
> > Signed-off-by: Toms Atteka <cpp.code.lv@gmail.com>
> > ---
> >  include/uapi/linux/openvswitch.h |  16 +++-
> >  net/openvswitch/flow.c           | 140 +++++++++++++++++++++++++++++++
> >  net/openvswitch/flow.h           |  14 ++++
> >  net/openvswitch/flow_netlink.c   |  24 +++++-
> >  4 files changed, 192 insertions(+), 2 deletions(-)
> >
> > diff --git a/include/uapi/linux/openvswitch.h b/include/uapi/linux/openvswitch.h
> > index a87b44cd5590..763adf3dce23 100644
> > --- a/include/uapi/linux/openvswitch.h
> > +++ b/include/uapi/linux/openvswitch.h
> > @@ -344,8 +344,17 @@ enum ovs_key_attr {
> >       OVS_KEY_ATTR_NSH,       /* Nested set of ovs_nsh_key_* */
> >
> >  #ifdef __KERNEL__
> > -     OVS_KEY_ATTR_TUNNEL_INFO,  /* struct ip_tunnel_info */
> > +     OVS_KEY_ATTR_TUNNEL_INFO,/* struct ip_tunnel_info */
> > +     __OVS_KEY_ATTR_PADDING_1,/* Padding to match field count with ovs */
> >  #endif
> > +
> > +#ifndef __KERNEL__
> > +     __OVS_KEY_ATTR_PADDING_2,/* Padding to match field count with ovs */
> > +     __OVS_KEY_ATTR_PADDING_3,/* Padding to match field count with ovs */
> > +#endif
> > +
> > +     OVS_KEY_ATTR_IPV6_EXTHDRS,  /* struct ovs_key_ipv6_exthdr */
> > +
> >       __OVS_KEY_ATTR_MAX
> >  };
>
> Not a full review, but, I think, that we should not add paddings, and
> define OVS_KEY_ATTR_IPV6_EXTHDRS before the OVS_KEY_ATTR_TUNNEL_INFO
> instead.  See my comments for v6:
>   https://lore.kernel.org/netdev/8c4ee3e8-0400-ee6e-b12c-327806f26dae@ovn.org/T/#u
>
> Best regards, Ilya Maximets.

Maybe I am still missing the idea, but I think it's not possible.

If we consider userspace code:
OVS_KEY_ATTR_NSH,       /* Nested set of ovs_nsh_key_* */

#ifdef __KERNEL__
/* Only used within kernel data path. */
OVS_KEY_ATTR_TUNNEL_INFO,  /* struct ovs_tunnel_info */
#endif

#ifndef __KERNEL__
/* Only used within userspace data path. */
OVS_KEY_ATTR_PACKET_TYPE,  /* be32 packet type */
OVS_KEY_ATTR_ND_EXTENSIONS, /* struct ovs_key_nd_extensions */
#endif

__OVS_KEY_ATTR_MAX
};

adding OVS_KEY_ATTR_IPV6_EXTHDRS before OVS_KEY_ATTR_PACKET_TYPE will
conflict on kernel update.

And we will see following warning:
00001|odp_util(handler1)|WARN|attribute packet_type has length 2 but
should have length 4

Best,
Tom