netdev - Re: [PATCHv3 iproute2-next 3/7] iproute_lwtunnel: add options support for erspan metadata

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:   Sun, 19 Apr 2020 16:39:20 +0800
From:   Xin Long <lucien.xin@...il.com>
To:     Stephen Hemminger <stephen@...workplumber.org>
Cc:     David Ahern <dsahern@...il.com>,
        network dev <netdev@...r.kernel.org>,
        Simon Horman <simon.horman@...ronome.com>
Subject: Re: [PATCHv3 iproute2-next 3/7] iproute_lwtunnel: add options support
 for erspan metadata

On Tue, Feb 18, 2020 at 12:29 PM Xin Long <lucien.xin@...il.com> wrote:
>
> On Tue, Feb 18, 2020 at 5:03 AM Stephen Hemminger
> <stephen@...workplumber.org> wrote:
> >
> > On Mon, 17 Feb 2020 12:53:14 -0700
> > David Ahern <dsahern@...il.com> wrote:
> >
> > > On 2/15/20 11:38 PM, Xin Long wrote:
> > > > On Sun, Feb 16, 2020 at 12:51 AM David Ahern <dsahern@...il.com> wrote:
> > > >>
> > > >> On 2/14/20 9:18 PM, Xin Long wrote:
> > > >>> On Sat, Feb 15, 2020 at 8:21 AM Stephen Hemminger
> > > >>> <stephen@...workplumber.org> wrote:
> > > >>>>
> > > >>>> On Sat, 15 Feb 2020 01:40:27 +0800
> > > >>>> Xin Long <lucien.xin@...il.com> wrote:
> > > >>>>
> > > >>>>> This's not gonna work. as the output will be:
> > > >>>>> {"ver":"0x2","idx":"0","dir":"0x1","hwid":"0x2"}  (string)
> > > >>>>> instead of
> > > >>>>> {"ver":2,"index":0,"dir":1,"hwid":2} (number)
> > > >>>>
> > > >>>> JSON is typeless. Lots of values are already printed in hex
> > > >>> You may mean JSON data itself is typeless.
> > > >>> But JSON objects are typed when parsing JSON data, which includes
> > > >>> string, number, array, boolean. So it matters how to define the
> > > >>> members' 'type' in JSON data.
> > > >>>
> > > >>> For example, in python's 'json' module:
> > > >>>
> > > >>> #!/usr/bin/python2
> > > >>> import json
> > > >>> json_data_1 = '{"ver":"0x2","idx":"0","dir":"0x1","hwid":"0x2"}'
> > > >>> json_data_2 = '{"ver":2,"index":0,"dir":1,"hwid":2}'
> > > >>> parsed_json_1 = (json.loads(json_data_1))
> > > >>> parsed_json_2 = (json.loads(json_data_2))
> > > >>> print type(parsed_json_1["hwid"])
> > > >>> print type(parsed_json_2["hwid"])
> > > >>>
> > > >>> The output is:
> > > >>> <type 'unicode'>
> > > >>> <type 'int'>
> > > >>>
> > > >>> Also, '{"result": true}' is different from '{"result": "true"}' when
> > > >>> loading it in a 3rd-party lib.
> > > >>>
> > > >>> I think the JSON data coming from iproute2 is designed to be used by
> > > >>> a 3rd-party lib to parse, not just to show to users. To keep these
> > > >>> members' original type (numbers) is more appropriate, IMO.
> > > >>>
> > > >>
> > > >> Stephen: why do you think all of the numbers should be in hex?
> > > >>
> > > >> It seems like consistency with existing output should matter more.
> > > >> ip/link_gre.c for instance prints index as an int, version as an int,
> > > >> direction as a string and only hwid in hex.
> > > >>
> > > >> Xin: any reason you did not follow the output of the existingg netdev
> > > >> based solutions?
> > > > Hi David,
> > > >
> > > > Option is expressed as "version:index:dir:hwid", I made all fields
> > > > in this string of hex, just like "class:type:data" in:
> > > >
> > > > commit 0ed5269f9e41f495c8e9020c85f5e1644c1afc57
> > > > Author: Simon Horman <simon.horman@...ronome.com>
> > > > Date:   Tue Jun 26 21:39:37 2018 -0700
> > > >
> > > >     net/sched: add tunnel option support to act_tunnel_key
> > > >
> > > > I'm not sure if it's good to mix multiple types in this string. wdyt?
> > > >
> > > > but for the JSON data, of course, these are all numbers(not string).
> > > >
> > >
> > > I don't understand why Stephen is pushing for hex; it does not make
> > > sense for version, index or direction. I don't have a clear
> > > understanding of hwid to know uint vs hex, so your current JSON prints
> > > seem fine.
> > >
> > > As for the stdout print and hex fields, staring at the tc and lwtunnel
> > > code, it seems like those 2 have a lot of parallels in expressing
> > > options for encoding vs lwtunnel and netdev based code. ie., I think
> > > this latest set is correct.
> > >
> > > Stephen?
> >
> > I just wanted:
> > 1. The parse and print functions should have the same formats.
> > I.e. if you take the output and do a little massaging of the ifindex
> > it should be accepted as an input set of parameters.
> >
> > 2. As much as possible, the JSON and non-JSON output should be similar.
> > If non-JSON prints in hex, then JSON should display hex and vice/versa.
> >
> > Ideally all inputs would be human format (not machine formats like hex).
> > But I guess the mistake was already made with some of the other tunnels.
> I guess we can't 'fix' these in other tunnels in tc.
>
> So I'm thinking we can either use the latest patchset,
> or keep the geneve opts format in lwtunnel consistent
> with the geneve opts in tc only and parse all with
> unint in the new erspan/vxlan tunnels opts.
Hi, Stephen and David A.

This patchset is in "deferred" status for a long time.
What should we do about this one?
should I improve something then repost or the lastest one will be fine.

Thanks.