linux-kernel - Re: [PATCH bpf-next 2/3] bpf: btf: add btf json print functionality

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: <20180621160719.2cfb4b58@cakuba.netronome.com>
Date:   Thu, 21 Jun 2018 16:07:19 -0700
From:   Jakub Kicinski <jakub.kicinski@...ronome.com>
To:     Martin KaFai Lau <kafai@...com>
Cc:     Okash Khawaja <osk@...com>, Daniel Borkmann <daniel@...earbox.net>,
        "Alexei Starovoitov" <ast@...nel.org>, Yonghong Song <yhs@...com>,
        Quentin Monnet <quentin.monnet@...ronome.com>,
        "David S. Miller" <davem@...emloft.net>, <netdev@...r.kernel.org>,
        <kernel-team@...com>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH bpf-next 2/3] bpf: btf: add btf json print functionality

On Thu, 21 Jun 2018 15:51:17 -0700, Martin KaFai Lau wrote:
> On Thu, Jun 21, 2018 at 02:59:35PM -0700, Jakub Kicinski wrote:
> > On Wed, 20 Jun 2018 13:30:53 -0700, Okash Khawaja wrote:  
> > > $ sudo bpftool map dump -p id 14
> > > [{
> > >         "key": 0
> > >     },{
> > >         "value": {
> > >             "m": 1,
> > >             "n": 2,
> > >             "o": "c",
> > >             "p": [15,16,17,18,15,16,17,18
> > >             ],
> > >             "q": [[25,26,27,28,25,26,27,28
> > >                 ],[35,36,37,38,35,36,37,38
> > >                 ],[45,46,47,48,45,46,47,48
> > >                 ],[55,56,57,58,55,56,57,58
> > >                 ]
> > >             ],
> > >             "r": 1,
> > >             "s": 0x7ffff6f70568,
> > >             "t": {
> > >                 "x": 5,
> > >                 "y": 10
> > >             },
> > >             "u": 100,
> > >             "v": 20,
> > >             "w1": 0x7,
> > >             "w2": 0x3
> > >         }
> > >     }
> > > ]  
> > 
> > I don't think this format is okay, JSON output is an API you shouldn't
> > break.  You can change the non-JSON output whatever way you like, but
> > JSON must remain backwards compatible.
> > 
> > The dump today has object per entry, e.g.:
> > 
> > {
> >         "key":["0x00","0x00","0x00","0x00",
> >         ],
> >         "value": ["0x02","0x00","0x00","0x00","0x00","0x00","0x00","0x00"
> >         ]
> > }
> > 
> > This format must remain, you may only augment it with new fields.  E.g.:
> > 
> > {
> >         "key":["0x00","0x00","0x00","0x00",
> >         ],
> > 	"key_struct":{
> > 		"index":0
> > 	},
> >         "value": ["0x02","0x00","0x00","0x00","0x00","0x00","0x00","0x00"
> >         ],
> > 	"value_struct":{
> > 		"src_ip":2,
> > 		"dst_ip:0
> > 	}
> > }  
> I am not sure how useful to have both "key|value" and "(key|value)_struct"
> while most people would prefer "key_struct"/"value_struct" if it is
> available.

Agreed, it's not that useful, especially with the string-hex debacle :(
It's just about the backwards compat.

> How about introducing a new option, like "-b", to print the
> map with BTF (if available) such that it won't break the existing
> one (-j or -p) while the "-b" output can keep using the "key"
> and "value".
> 
> The existing json can be kept as is.

That was my knee jerk reaction too, but on reflection it doesn't sound
that great.  We expect people with new-enough bpftool to use btf, so it
should be available in the default output, without hiding it behind a
switch.  We could add a switch to hide the old output, but that doesn't
give us back the names...  What about Key and Value or k and v?  Or
key_fields and value_fields?

> > The name XYZ_struct may not be the best, perhaps you can come up with a
> > better one?  
> > 
> > Does that make sense?  Am I missing what you're doing here?
> > 
> > One process note - please make sure you run checkpatch.pl --strict on
> > bpftool patches before posting.
> > 
> > Thanks for working on this!