| 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
| ||
|
Message-ID: <20180622012052.htkvholi674x6i4f@kafai-mbp.dhcp.thefacebook.com>
Date: Thu, 21 Jun 2018 18:20:52 -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 05:25:23PM -0700, Jakub Kicinski wrote:
> On Thu, 21 Jun 2018 16:58:15 -0700, Martin KaFai Lau wrote:
> > 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
> > > > > },
Got a few questions.
When we support hashtab later, the key could be int
but reusing the name as "index" is weird.
The key could also be a struct (e.g. a struct to describe ip:port).
Can you suggest how the "key_struct" will look like?
> > > > > "value": ["0x02","0x00","0x00","0x00","0x00","0x00","0x00","0x00"
> > > > > ],
> > > > > "value_struct":{
> > > > > "src_ip":2,
If for the same map the user changes the "src_ip" to an array of int[4]
later (e.g. to support ipv6), it will become "src_ip": [1, 2, 3, 4].
Is it breaking backward compat?
i.e.
struct five_tuples {
- int src_ip;
+ int src_ip[4];
/* ... */
};
> > > > > "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"?
>
> SGTM! Or even maybe as a "formatted" object?:
>
> {
> "key":["0x00","0x00","0x00","0x00",
> ],
> "value": ["0x02","0x00","0x00","0x00","0x00","0x00","0x00","0x00"
> ],
> "formatted":{
> "key":{
> "index":0
> },
> "value":{
> "src_ip":2,
> "dst_ip:0
> }
> }
hmm... that is an extra indentation (keep in mind that the "value" could
already have a few nested structs which itself consumes a few indentations)
but I guess adding another one may be ok-ish.
> }
>
> > > > > 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!
>
Powered by blists - more mailing lists