netdev - Re: [PATCH net-next v2] net: openvswitch: IPv6: Add IPv6 extension header support

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 24 May 2021 11:57:49 -0700
From:   Cpp Code <cpp.code.lv@...il.com>
To:     Ilya Maximets <i.maximets@....org>
Cc:     netdev@...r.kernel.org, "pshelar@....org" <pshelar@....org>,
        "David S. Miller" <davem@...emloft.net>,
        ovs dev <dev@...nvswitch.org>,
        Jakub Kicinski <kuba@...nel.org>, Ben Pfaff <blp@....org>
Subject: Re: [PATCH net-next v2] net: openvswitch: IPv6: Add IPv6 extension
 header support

Yes, these changes only works together with changes in userspace. I
believe in any solution there should be corresponding changes in
userspace. If we would be able to easily run old version of userspace
with these changes in kernel without userspace complaining about
struct size, we could get in to a situation with hard to find bugs.

I don't agree with the solution of a new struct key as semantically
ipv6 extension headers are integral part of every ipv6 packet thus
expected to be in the struct along with label, for example. Correct if
I am missing something.

On Wed, May 19, 2021 at 2:52 AM Ilya Maximets <i.maximets@....org> wrote:
>
> On 5/17/21 5:20 PM, Toms Atteka wrote:
> > IPv6 extension headers carry optional internet layer information
> > and are placed between the fixed header and the upper-layer
> > protocol header.
> >
> > This change adds a new OpenFlow field OFPXMT_OFB_IPV6_EXTHDR and
> > packets can be filtered using ipv6_ext flag.
> >
> > Tested-at: https://github.com/TomCodeLV/ovs/actions/runs/504185214
> > Signed-off-by: Toms Atteka <cpp.code.lv@...il.com>
> > ---
> >  include/uapi/linux/openvswitch.h |   1 +
> >  net/openvswitch/flow.c           | 141 +++++++++++++++++++++++++++++++
> >  net/openvswitch/flow.h           |  14 +++
> >  net/openvswitch/flow_netlink.c   |   5 +-
> >  4 files changed, 160 insertions(+), 1 deletion(-)
> >
> >
> > base-commit: 5d869070569a23aa909c6e7e9d010fc438a492ef
> >
> > diff --git a/include/uapi/linux/openvswitch.h b/include/uapi/linux/openvswitch.h
> > index 8d16744edc31..a19812b6631a 100644
> > --- a/include/uapi/linux/openvswitch.h
> > +++ b/include/uapi/linux/openvswitch.h
> > @@ -420,6 +420,7 @@ struct ovs_key_ipv6 {
> >       __u8   ipv6_tclass;
> >       __u8   ipv6_hlimit;
> >       __u8   ipv6_frag;       /* One of OVS_FRAG_TYPE_*. */
> > +     __u16  ipv6_exthdr;
> >  };
>
> Wouldn't this break existing userspace?  Curent OVS expects netlink
> message with attribute size equal to the old version of 'struct ovs_key_ipv6'
> and it will discard OVS_KEY_ATTR_IPV6 as malformed.
>
> This should likely be a completely new structure and a completely new
> OVS_KEY_ATTR.
>
> Best regards, Ilya Maximets.