netdev - Re: [PATCH net-next v4] openvswitch: enable NSH 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]

Message-ID: <20170821114713.7c6ebb9a@griffin>
Date:   Mon, 21 Aug 2017 11:47:13 +0200
From:   Jiri Benc <jbenc@...hat.com>
To:     "Yang, Yi" <yi.y.yang@...el.com>
Cc:     "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "dev@...nvswitch.org" <dev@...nvswitch.org>,
        "blp@....org" <blp@....org>, "e@...g.me" <e@...g.me>,
        "jan.scheurich@...csson.com" <jan.scheurich@...csson.com>
Subject: Re: [PATCH net-next v4] openvswitch: enable NSH support

On Mon, 21 Aug 2017 17:15:42 +0800, Yang, Yi wrote:
> The issue is it is used union in
> 
> struct nsh_hdr {
>     ovs_be16 ver_flags_ttl_len;
>     uint8_t md_type;
>     uint8_t next_proto;
>     ovs_16aligned_be32 path_hdr;
>     union {
>         struct nsh_md1_ctx md1;
>         struct nsh_md2_tlv md2;
>     };
> };

This should work (modulo the non-kernel type names, of course). Did you
mean to put [] after md2?

> in Linux kernel build, it complained it, I changed it to

What was the error message?

> struct nsh_hdr {
>     ovs_be16 ver_flags_ttl_len;
>     uint8_t md_type;
>     uint8_t next_proto;
>     ovs_16aligned_be32 path_hdr;
>     union {
>         struct nsh_md1_ctx md1;
>         struct nsh_md2_tlv md2[0];
>     };
> };

I wouldn't use this. First, zero length array is a GCC extension. It
would indeed be better not to use that in uAPI. Second, I wouldn't even
use a flexible array member here, see my reply to Jan for the reasons.

Note that I commented on struct nsh_md2_tlv having __u8[] as the last
member which IMHO makes good sense. I'm not entirely sure what C99 says
about flexible array member being part of a struct inside union inside
a struct, though. GCC seems to cope with that just fine but AFAIK it
has some extension over the C standard wrt. flexible array members.

> I don't know how we can support this, is it a must-have thing?

What would happen if you get a GSO packet? Ports of an ovs bridge claim
GSO support, thus they may get a GSO packet. You have to handle it one
way or the other: either software segment the packet before pushing the
header, or implement proper GSO support for NSH.

> But struct nsh_hdr had different struct from struct ovs_key_nsh, we
> have no way to make them completely same, do you mean we should use the
> same name if they are same fields and represent the same thing?

Yes.

Thanks,

 Jiri