netdev - 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]

Date:   Thu, 21 Jun 2018 16:58:15 -0700
From:   Martin KaFai Lau <kafai@...com>
To:     Jakub Kicinski <jakub.kicinski@...ronome.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, Jun 21, 2018 at 04:07:19PM -0700, Jakub Kicinski wrote:
> 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?
I thought the current default output is "plain" ;)
Having said that, yes, the btf is currently printed in json.

Ideally, the default json output should do what most people want:
print btf and btf only (if it is available).
but I don't see a way out without new option if we need to
be backward compat :(

Agree that showing the btf in the existing json output will be useful (e.g.
to hint people that BTF is available).  If btf is showing in old json,
also agree that the names should be the same with the new json.
key_fields and value_fields may hint it has >1 fields though.
May be "formatted_key" and "formatted_value"?

> 
> > > 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!