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: <20170821111854.42dece9f@griffin>
Date:   Mon, 21 Aug 2017 11:18:54 +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 16:39:01 +0800, Yang, Yi wrote:
> Anyway, we need to keep the code in userspace consistent with the one in
> kernel as possible as, otherwise it will be a burden for developer, I
> know userspace has different coding standard from kernel, this will make
> developer painful if we have two sets of code although they have same
> functionality.

I'm sorry, I don't get this. What's wrong with having __u8[] as the
last member of the struct? That's C99. It's 18 years old standard.
We're using that throughout our uAPI. Why that should be a problem for
any user space program?

> > MPLS supports GSO and needs this for segmentation. I don't see anything
> > GSO related in this patch.
> > 
> > How do you plan to address GSO, anyway?
> 
> No plan to do that, I'm not an expert on this, we can remove it if
> you're very sure it is necessary.

Without GSO, I don't see any use for inner_protocol.

However, don't you need to software segment the packet if it's GSO
before pushing the NSH header?

And wouldn't it be better to implement GSO for NSH, anyway?

> To make sure we make agreement, please confirm if this one is ok?
> 
> struct nsh_hdr {
>     ovs_be16 ver_flags_ttl_len;
>     uint8_t mdtype;
>     uint8_t np;
>     ovs_16aligned_be32 path_hdr;
>     union {
>         struct nsh_md1_ctx md1;
>         struct nsh_md2_tlv md2;
>     };
> };
> 
> Or it will be better if you can provide your preferred version here.

I don't really care that much about the names if it's clear what they
mean. I was merely commenting on the inconsistency which looked weird.
Whether it's md_type or mdtype, I don't have a preference (does not
mean others won't, though :-)). Just pick one and stick to it, as far
as I'm concerned.

 Jiri