| Message ID | 9138c60f036c68f02c41dae0605ef587a8347f4c.1597915265.git.zhuyifei@google.com |
|---|---|
| State | Changes Requested |
| Delegated to: | BPF Maintainers |
| Headers | show |
| Series | Allow storage of flexible metadata information for eBPF programs | expand |
On 8/20/20 2:42 AM, YiFei Zhu wrote: > From: YiFei Zhu <zhuyifei@google.com> > > Added a flag "--metadata" to `bpftool prog list` to dump the metadata > contents. For some formatting some BTF code is put directly in the > metadata dumping. Sanity checks on the map and the kind of the btf_type > to make sure we are actually dumping what we are expecting. > > A helper jsonw_reset is added to json writer so we can reuse the same > json writer without having extraneous commas. > > Sample output: > > $ bpftool prog --metadata > 6: cgroup_skb name prog tag bcf7977d3b93787c gpl > [...] > btf_id 4 > metadata: > metadata_a = "foo" > metadata_b = 1 > > $ bpftool prog --metadata --json --pretty > [{ > "id": 6, > [...] > "btf_id": 4, > "metadata": { > "metadata_a": "foo", > "metadata_b": 1 > } > } > ] > > Signed-off-by: YiFei Zhu <zhuyifei@google.com> > --- > tools/bpf/bpftool/json_writer.c | 6 ++ > tools/bpf/bpftool/json_writer.h | 3 + > tools/bpf/bpftool/main.c | 10 +++ > tools/bpf/bpftool/main.h | 1 + > tools/bpf/bpftool/prog.c | 135 ++++++++++++++++++++++++++++++++ > 5 files changed, 155 insertions(+) > > diff --git a/tools/bpf/bpftool/json_writer.c b/tools/bpf/bpftool/json_writer.c > index 86501cd3c763..7fea83bedf48 100644 > --- a/tools/bpf/bpftool/json_writer.c > +++ b/tools/bpf/bpftool/json_writer.c > @@ -119,6 +119,12 @@ void jsonw_pretty(json_writer_t *self, bool on) > self->pretty = on; > } > > +void jsonw_reset(json_writer_t *self) > +{ > + assert(self->depth == 0); > + self->sep = '\0'; > +} > + > /* Basic blocks */ > static void jsonw_begin(json_writer_t *self, int c) > { > diff --git a/tools/bpf/bpftool/json_writer.h b/tools/bpf/bpftool/json_writer.h > index 35cf1f00f96c..8ace65cdb92f 100644 > --- a/tools/bpf/bpftool/json_writer.h > +++ b/tools/bpf/bpftool/json_writer.h > @@ -27,6 +27,9 @@ void jsonw_destroy(json_writer_t **self_p); > /* Cause output to have pretty whitespace */ > void jsonw_pretty(json_writer_t *self, bool on); > > +/* Reset separator to create new JSON */ > +void jsonw_reset(json_writer_t *self); > + > /* Add property name */ > void jsonw_name(json_writer_t *self, const char *name); > > diff --git a/tools/bpf/bpftool/main.c b/tools/bpf/bpftool/main.c > index 4a191fcbeb82..a681d568cfa7 100644 > --- a/tools/bpf/bpftool/main.c > +++ b/tools/bpf/bpftool/main.c > @@ -28,6 +28,7 @@ bool show_pinned; > bool block_mount; > bool verifier_logs; > bool relaxed_maps; > +bool dump_metadata; > struct pinned_obj_table prog_table; > struct pinned_obj_table map_table; > struct pinned_obj_table link_table; > @@ -351,6 +352,10 @@ static int do_batch(int argc, char **argv) > return err; > } > > +enum bpftool_longonly_opts { > + OPT_METADATA = 256, > +}; > + > int main(int argc, char **argv) > { > static const struct option options[] = { > @@ -362,6 +367,7 @@ int main(int argc, char **argv) > { "mapcompat", no_argument, NULL, 'm' }, > { "nomount", no_argument, NULL, 'n' }, > { "debug", no_argument, NULL, 'd' }, > + { "metadata", no_argument, NULL, OPT_METADATA }, > { 0 } > }; > int opt, ret; > @@ -371,6 +377,7 @@ int main(int argc, char **argv) > json_output = false; > show_pinned = false; > block_mount = false; > + dump_metadata = false; > bin_name = argv[0]; > > hash_init(prog_table.table); > @@ -412,6 +419,9 @@ int main(int argc, char **argv) > libbpf_set_print(print_all_levels); > verifier_logs = true; > break; > + case OPT_METADATA: > + dump_metadata = true; > + break; > default: > p_err("unrecognized option '%s'", argv[optind - 1]); > if (json_output) > diff --git a/tools/bpf/bpftool/main.h b/tools/bpf/bpftool/main.h > index c46e52137b87..8750758e9150 100644 > --- a/tools/bpf/bpftool/main.h > +++ b/tools/bpf/bpftool/main.h > @@ -90,6 +90,7 @@ extern bool show_pids; > extern bool block_mount; > extern bool verifier_logs; > extern bool relaxed_maps; > +extern bool dump_metadata; > extern struct pinned_obj_table prog_table; > extern struct pinned_obj_table map_table; > extern struct pinned_obj_table link_table; > diff --git a/tools/bpf/bpftool/prog.c b/tools/bpf/bpftool/prog.c > index d393eb8263a6..ee767b8d90fb 100644 > --- a/tools/bpf/bpftool/prog.c > +++ b/tools/bpf/bpftool/prog.c > @@ -151,6 +151,135 @@ static void show_prog_maps(int fd, __u32 num_maps) > } > } > > +static void show_prog_metadata(int fd, __u32 num_maps) > +{ > + struct bpf_prog_info prog_info = {}; > + struct bpf_map_info map_info = {}; > + __u32 prog_info_len = sizeof(prog_info); > + __u32 map_info_len = sizeof(map_info); > + __u32 map_ids[num_maps]; > + void *value = NULL; > + struct btf *btf = NULL; > + const struct btf_type *t_datasec, *t_var; > + struct btf_var_secinfo *vsi; > + int key = 0; > + unsigned int i, vlen; > + int map_fd; > + int err; try to follow reverse christmas tree coding styple? > + > + prog_info.nr_map_ids = num_maps; > + prog_info.map_ids = ptr_to_u64(map_ids); > + > + err = bpf_obj_get_info_by_fd(fd, &prog_info, &prog_info_len); > + if (err || !prog_info.nr_map_ids) > + return; print out something for "err" case and "!prog_info.nr_map_ids" case? The same for some other below returns. > + > + for (i = 0; i < prog_info.nr_map_ids; i++) { > + map_fd = bpf_map_get_fd_by_id(map_ids[i]); > + if (map_fd < 0) > + return; > + > + err = bpf_obj_get_info_by_fd(map_fd, &map_info, &map_info_len); > + if (err) > + goto out_close; > + > + if (map_info.type != BPF_MAP_TYPE_ARRAY) > + goto next_map; > + if (map_info.key_size != sizeof(int)) > + goto next_map; > + if (map_info.max_entries != 1) > + goto next_map; > + if (!map_info.btf_value_type_id) > + goto next_map; > + if (!strstr(map_info.name, ".metadata")) > + goto next_map; > + > + goto found; > + > +next_map: > + close(map_fd); > + } > + > + return; > + > +found: > + value = malloc(map_info.value_size); > + if (!value) > + goto out_close; > + > + if (bpf_map_lookup_elem(map_fd, &key, value)) > + goto out_free; Not sure whether we need formal libbpf API to access metadata or not. This may help other applications too. But we can delay until it is necessary. If we can put metadata in skeleton like <metadata_type> *metadata; and then it will be very easy for users to access it. > + > + err = btf__get_from_id(map_info.btf_id, &btf); > + if (err || !btf) > + goto out_free; > + > + t_datasec = btf__type_by_id(btf, map_info.btf_value_type_id); > + if (BTF_INFO_KIND(t_datasec->info) != BTF_KIND_DATASEC) > + goto out_free; > + > + vlen = BTF_INFO_VLEN(t_datasec->info); > + vsi = (struct btf_var_secinfo *)(t_datasec + 1); > + > + if (json_output) { > + struct btf_dumper d = { > + .btf = btf, > + .jw = json_wtr, > + .is_plain_text = false, > + }; > + > + jsonw_name(json_wtr, "metadata"); > + > + jsonw_start_object(json_wtr); > + for (i = 0; i < vlen; i++) { > + t_var = btf__type_by_id(btf, vsi[i].type); > + > + if (BTF_INFO_KIND(t_var->info) != BTF_KIND_VAR) > + continue; this should not happen. > + > + jsonw_name(json_wtr, btf__name_by_offset(btf, t_var->name_off)); > + err = btf_dumper_type(&d, t_var->type, value + vsi[i].offset); > + if (err) > + break; > + } > + jsonw_end_object(json_wtr); > + } else { > + json_writer_t *btf_wtr = jsonw_new(stdout); > + struct btf_dumper d = { > + .btf = btf, > + .jw = btf_wtr, > + .is_plain_text = true, > + }; > + if (!btf_wtr) > + goto out_free; > + > + printf("\tmetadata:"); > + > + for (i = 0; i < vlen; i++) { > + t_var = btf__type_by_id(btf, vsi[i].type); > + > + if (BTF_INFO_KIND(t_var->info) != BTF_KIND_VAR) > + continue; this should not happen. > + > + printf("\n\t\t%s = ", btf__name_by_offset(btf, t_var->name_off)); > + > + jsonw_reset(btf_wtr); > + err = btf_dumper_type(&d, t_var->type, value + vsi[i].offset); > + if (err) > + break; > + } > + > + jsonw_destroy(&btf_wtr); > + } > + > +out_free: > + btf__free(btf); > + free(value); > + > +out_close: > + close(map_fd); > +} > + > static void print_prog_header_json(struct bpf_prog_info *info) > { > jsonw_uint_field(json_wtr, "id", info->id); > @@ -228,6 +357,9 @@ static void print_prog_json(struct bpf_prog_info *info, int fd) > > emit_obj_refs_json(&refs_table, info->id, json_wtr); > > + if (dump_metadata) > + show_prog_metadata(fd, info->nr_map_ids); > + > jsonw_end_object(json_wtr); > } > > @@ -297,6 +429,9 @@ static void print_prog_plain(struct bpf_prog_info *info, int fd) > emit_obj_refs_plain(&refs_table, info->id, "\n\tpids "); > > printf("\n"); > + > + if (dump_metadata) > + show_prog_metadata(fd, info->nr_map_ids); > } > > static int show_prog(int fd) >
Yonghong Song <yhs@fb.com> writes: > On 8/20/20 2:42 AM, YiFei Zhu wrote: >> From: YiFei Zhu <zhuyifei@google.com> >> >> Added a flag "--metadata" to `bpftool prog list` to dump the metadata >> contents. For some formatting some BTF code is put directly in the >> metadata dumping. Sanity checks on the map and the kind of the btf_type >> to make sure we are actually dumping what we are expecting. >> >> A helper jsonw_reset is added to json writer so we can reuse the same >> json writer without having extraneous commas. >> >> Sample output: >> >> $ bpftool prog --metadata >> 6: cgroup_skb name prog tag bcf7977d3b93787c gpl >> [...] >> btf_id 4 >> metadata: >> metadata_a = "foo" >> metadata_b = 1 >> >> $ bpftool prog --metadata --json --pretty >> [{ >> "id": 6, >> [...] >> "btf_id": 4, >> "metadata": { >> "metadata_a": "foo", >> "metadata_b": 1 >> } >> } >> ] >> >> Signed-off-by: YiFei Zhu <zhuyifei@google.com> >> --- >> tools/bpf/bpftool/json_writer.c | 6 ++ >> tools/bpf/bpftool/json_writer.h | 3 + >> tools/bpf/bpftool/main.c | 10 +++ >> tools/bpf/bpftool/main.h | 1 + >> tools/bpf/bpftool/prog.c | 135 ++++++++++++++++++++++++++++++++ >> 5 files changed, 155 insertions(+) >> >> diff --git a/tools/bpf/bpftool/json_writer.c b/tools/bpf/bpftool/json_writer.c >> index 86501cd3c763..7fea83bedf48 100644 >> --- a/tools/bpf/bpftool/json_writer.c >> +++ b/tools/bpf/bpftool/json_writer.c >> @@ -119,6 +119,12 @@ void jsonw_pretty(json_writer_t *self, bool on) >> self->pretty = on; >> } >> >> +void jsonw_reset(json_writer_t *self) >> +{ >> + assert(self->depth == 0); >> + self->sep = '\0'; >> +} >> + >> /* Basic blocks */ >> static void jsonw_begin(json_writer_t *self, int c) >> { >> diff --git a/tools/bpf/bpftool/json_writer.h b/tools/bpf/bpftool/json_writer.h >> index 35cf1f00f96c..8ace65cdb92f 100644 >> --- a/tools/bpf/bpftool/json_writer.h >> +++ b/tools/bpf/bpftool/json_writer.h >> @@ -27,6 +27,9 @@ void jsonw_destroy(json_writer_t **self_p); >> /* Cause output to have pretty whitespace */ >> void jsonw_pretty(json_writer_t *self, bool on); >> >> +/* Reset separator to create new JSON */ >> +void jsonw_reset(json_writer_t *self); >> + >> /* Add property name */ >> void jsonw_name(json_writer_t *self, const char *name); >> >> diff --git a/tools/bpf/bpftool/main.c b/tools/bpf/bpftool/main.c >> index 4a191fcbeb82..a681d568cfa7 100644 >> --- a/tools/bpf/bpftool/main.c >> +++ b/tools/bpf/bpftool/main.c >> @@ -28,6 +28,7 @@ bool show_pinned; >> bool block_mount; >> bool verifier_logs; >> bool relaxed_maps; >> +bool dump_metadata; >> struct pinned_obj_table prog_table; >> struct pinned_obj_table map_table; >> struct pinned_obj_table link_table; >> @@ -351,6 +352,10 @@ static int do_batch(int argc, char **argv) >> return err; >> } >> >> +enum bpftool_longonly_opts { >> + OPT_METADATA = 256, >> +}; >> + >> int main(int argc, char **argv) >> { >> static const struct option options[] = { >> @@ -362,6 +367,7 @@ int main(int argc, char **argv) >> { "mapcompat", no_argument, NULL, 'm' }, >> { "nomount", no_argument, NULL, 'n' }, >> { "debug", no_argument, NULL, 'd' }, >> + { "metadata", no_argument, NULL, OPT_METADATA }, >> { 0 } >> }; >> int opt, ret; >> @@ -371,6 +377,7 @@ int main(int argc, char **argv) >> json_output = false; >> show_pinned = false; >> block_mount = false; >> + dump_metadata = false; >> bin_name = argv[0]; >> >> hash_init(prog_table.table); >> @@ -412,6 +419,9 @@ int main(int argc, char **argv) >> libbpf_set_print(print_all_levels); >> verifier_logs = true; >> break; >> + case OPT_METADATA: >> + dump_metadata = true; >> + break; >> default: >> p_err("unrecognized option '%s'", argv[optind - 1]); >> if (json_output) >> diff --git a/tools/bpf/bpftool/main.h b/tools/bpf/bpftool/main.h >> index c46e52137b87..8750758e9150 100644 >> --- a/tools/bpf/bpftool/main.h >> +++ b/tools/bpf/bpftool/main.h >> @@ -90,6 +90,7 @@ extern bool show_pids; >> extern bool block_mount; >> extern bool verifier_logs; >> extern bool relaxed_maps; >> +extern bool dump_metadata; >> extern struct pinned_obj_table prog_table; >> extern struct pinned_obj_table map_table; >> extern struct pinned_obj_table link_table; >> diff --git a/tools/bpf/bpftool/prog.c b/tools/bpf/bpftool/prog.c >> index d393eb8263a6..ee767b8d90fb 100644 >> --- a/tools/bpf/bpftool/prog.c >> +++ b/tools/bpf/bpftool/prog.c >> @@ -151,6 +151,135 @@ static void show_prog_maps(int fd, __u32 num_maps) >> } >> } >> >> +static void show_prog_metadata(int fd, __u32 num_maps) >> +{ >> + struct bpf_prog_info prog_info = {}; >> + struct bpf_map_info map_info = {}; >> + __u32 prog_info_len = sizeof(prog_info); >> + __u32 map_info_len = sizeof(map_info); >> + __u32 map_ids[num_maps]; >> + void *value = NULL; >> + struct btf *btf = NULL; >> + const struct btf_type *t_datasec, *t_var; >> + struct btf_var_secinfo *vsi; >> + int key = 0; >> + unsigned int i, vlen; >> + int map_fd; >> + int err; > > try to follow reverse christmas tree coding styple? > >> + >> + prog_info.nr_map_ids = num_maps; >> + prog_info.map_ids = ptr_to_u64(map_ids); >> + >> + err = bpf_obj_get_info_by_fd(fd, &prog_info, &prog_info_len); >> + if (err || !prog_info.nr_map_ids) >> + return; > > print out something for "err" case and "!prog_info.nr_map_ids" case? > The same for some other below returns. > >> + >> + for (i = 0; i < prog_info.nr_map_ids; i++) { >> + map_fd = bpf_map_get_fd_by_id(map_ids[i]); >> + if (map_fd < 0) >> + return; >> + >> + err = bpf_obj_get_info_by_fd(map_fd, &map_info, &map_info_len); >> + if (err) >> + goto out_close; >> + >> + if (map_info.type != BPF_MAP_TYPE_ARRAY) >> + goto next_map; >> + if (map_info.key_size != sizeof(int)) >> + goto next_map; >> + if (map_info.max_entries != 1) >> + goto next_map; >> + if (!map_info.btf_value_type_id) >> + goto next_map; >> + if (!strstr(map_info.name, ".metadata")) >> + goto next_map; >> + >> + goto found; >> + >> +next_map: >> + close(map_fd); >> + } >> + >> + return; >> + >> +found: >> + value = malloc(map_info.value_size); >> + if (!value) >> + goto out_close; >> + >> + if (bpf_map_lookup_elem(map_fd, &key, value)) >> + goto out_free; > > Not sure whether we need formal libbpf API to access metadata or not. > This may help other applications too. But we can delay until it is > necessary. Yeah, please put in a libbpf accessor as well; I would like to use this from libxdp - without a skeleton :) -Toke
On Fri, Aug 21, 2020 at 3:58 AM Toke Høiland-Jørgensen <toke@redhat.com> wrote: > Yonghong Song <yhs@fb.com> writes: > > Not sure whether we need formal libbpf API to access metadata or not. > > This may help other applications too. But we can delay until it is > > necessary. > > Yeah, please put in a libbpf accessor as well; I would like to use this > from libxdp - without a skeleton :) > > -Toke I don't think I have an idea on a good API in libbpf that could be used to get the metadata of an existing program in kernel, that could be reused by bpftool without duplicating all the code. Maybe we can discuss this in a follow up series? YiFei Zhu
YiFei Zhu <zhuyifei@google.com> writes: > On Fri, Aug 21, 2020 at 3:58 AM Toke Høiland-Jørgensen <toke@redhat.com> wrote: >> Yonghong Song <yhs@fb.com> writes: >> > Not sure whether we need formal libbpf API to access metadata or not. >> > This may help other applications too. But we can delay until it is >> > necessary. >> >> Yeah, please put in a libbpf accessor as well; I would like to use this >> from libxdp - without a skeleton :) >> >> -Toke > > I don't think I have an idea on a good API in libbpf that could be > used to get the metadata of an existing program in kernel, that could > be reused by bpftool without duplicating all the code. Maybe we can > discuss this in a follow up series? I think the most important part is getting a reference to the metadata map. So a function that basically does what the top half of what your show_prog_metadata() function does: given a prog fd, walk the map ids, check if any of them looks like a metadata map, and if so return the map fd. Should be pretty straight-forward to reuse between bpftool/libbpf, no? -Toke
On Thu, Aug 20, 2020 at 2:44 AM YiFei Zhu <zhuyifei1999@gmail.com> wrote: > > From: YiFei Zhu <zhuyifei@google.com> > > Added a flag "--metadata" to `bpftool prog list` to dump the metadata > contents. For some formatting some BTF code is put directly in the > metadata dumping. Sanity checks on the map and the kind of the btf_type > to make sure we are actually dumping what we are expecting. > > A helper jsonw_reset is added to json writer so we can reuse the same > json writer without having extraneous commas. > > Sample output: > > $ bpftool prog --metadata > 6: cgroup_skb name prog tag bcf7977d3b93787c gpl > [...] > btf_id 4 > metadata: > metadata_a = "foo" > metadata_b = 1 > > $ bpftool prog --metadata --json --pretty > [{ > "id": 6, > [...] > "btf_id": 4, > "metadata": { > "metadata_a": "foo", > "metadata_b": 1 > } > } > ] > > Signed-off-by: YiFei Zhu <zhuyifei@google.com> > --- > tools/bpf/bpftool/json_writer.c | 6 ++ > tools/bpf/bpftool/json_writer.h | 3 + > tools/bpf/bpftool/main.c | 10 +++ > tools/bpf/bpftool/main.h | 1 + > tools/bpf/bpftool/prog.c | 135 ++++++++++++++++++++++++++++++++ > 5 files changed, 155 insertions(+) > [...] > + for (i = 0; i < prog_info.nr_map_ids; i++) { > + map_fd = bpf_map_get_fd_by_id(map_ids[i]); > + if (map_fd < 0) > + return; > + > + err = bpf_obj_get_info_by_fd(map_fd, &map_info, &map_info_len); > + if (err) > + goto out_close; > + > + if (map_info.type != BPF_MAP_TYPE_ARRAY) > + goto next_map; > + if (map_info.key_size != sizeof(int)) > + goto next_map; > + if (map_info.max_entries != 1) > + goto next_map; > + if (!map_info.btf_value_type_id) > + goto next_map; > + if (!strstr(map_info.name, ".metadata")) This substring check sucks. Let's make libbpf call this map strictly ".metadata". Current convention of "some part of object name" + "." + {rodata,data,bss} is extremely confusing. In practice it's something incomprehensible and "unguessable" like "test_pr.rodata". I think it makes sense to call them just ".data", ".rodata", ".bss", and ".metadata". But that might break existing apps that do lookups based on map name (and might break skeleton as it is today, not sure). But let's at least start with ".metadata", as it's a new map and we can get it right from the start. > + goto next_map; > + > + goto found; > + > +next_map: > + close(map_fd); > + } > + > + return; > + > +found: [...]
On 08/25, Andrii Nakryiko wrote: > On Thu, Aug 20, 2020 at 2:44 AM YiFei Zhu <zhuyifei1999@gmail.com> wrote: > > > > From: YiFei Zhu <zhuyifei@google.com> > > > > Added a flag "--metadata" to `bpftool prog list` to dump the metadata > > contents. For some formatting some BTF code is put directly in the > > metadata dumping. Sanity checks on the map and the kind of the btf_type > > to make sure we are actually dumping what we are expecting. > > > > A helper jsonw_reset is added to json writer so we can reuse the same > > json writer without having extraneous commas. > > > > Sample output: > > > > $ bpftool prog --metadata > > 6: cgroup_skb name prog tag bcf7977d3b93787c gpl > > [...] > > btf_id 4 > > metadata: > > metadata_a = "foo" > > metadata_b = 1 > > > > $ bpftool prog --metadata --json --pretty > > [{ > > "id": 6, > > [...] > > "btf_id": 4, > > "metadata": { > > "metadata_a": "foo", > > "metadata_b": 1 > > } > > } > > ] > > > > Signed-off-by: YiFei Zhu <zhuyifei@google.com> > > --- > > tools/bpf/bpftool/json_writer.c | 6 ++ > > tools/bpf/bpftool/json_writer.h | 3 + > > tools/bpf/bpftool/main.c | 10 +++ > > tools/bpf/bpftool/main.h | 1 + > > tools/bpf/bpftool/prog.c | 135 ++++++++++++++++++++++++++++++++ > > 5 files changed, 155 insertions(+) > > > [...] > > + for (i = 0; i < prog_info.nr_map_ids; i++) { > > + map_fd = bpf_map_get_fd_by_id(map_ids[i]); > > + if (map_fd < 0) > > + return; > > + > > + err = bpf_obj_get_info_by_fd(map_fd, &map_info, > &map_info_len); > > + if (err) > > + goto out_close; > > + > > + if (map_info.type != BPF_MAP_TYPE_ARRAY) > > + goto next_map; > > + if (map_info.key_size != sizeof(int)) > > + goto next_map; > > + if (map_info.max_entries != 1) > > + goto next_map; > > + if (!map_info.btf_value_type_id) > > + goto next_map; > > + if (!strstr(map_info.name, ".metadata")) > This substring check sucks. Let's make libbpf call this map strictly > ".metadata". Current convention of "some part of object name" + "." + > {rodata,data,bss} is extremely confusing. In practice it's something > incomprehensible and "unguessable" like "test_pr.rodata". I think it > makes sense to call them just ".data", ".rodata", ".bss", and > ".metadata". But that might break existing apps that do lookups based > on map name (and might break skeleton as it is today, not sure). But > let's at least start with ".metadata", as it's a new map and we can > get it right from the start. Isn't it bad from the consistency point of view? Even if it's bad, at least it's consistent :-/
On 08/23, Toke H�iland-J�rgensen wrote: > YiFei Zhu <zhuyifei@google.com> writes: > > On Fri, Aug 21, 2020 at 3:58 AM Toke H�iland-J�rgensen > <toke@redhat.com> wrote: > >> Yonghong Song <yhs@fb.com> writes: > >> > Not sure whether we need formal libbpf API to access metadata or not. > >> > This may help other applications too. But we can delay until it is > >> > necessary. > >> > >> Yeah, please put in a libbpf accessor as well; I would like to use this > >> from libxdp - without a skeleton :) > >> > >> -Toke > > > > I don't think I have an idea on a good API in libbpf that could be > > used to get the metadata of an existing program in kernel, that could > > be reused by bpftool without duplicating all the code. Maybe we can > > discuss this in a follow up series? > I think the most important part is getting a reference to the metadata > map. So a function that basically does what the top half of what your > show_prog_metadata() function does: given a prog fd, walk the map ids, > check if any of them looks like a metadata map, and if so return the map > fd. > Should be pretty straight-forward to reuse between bpftool/libbpf, no? Sounds good, I'll be taking over this patch series as YiFei's internship has ended. I'll try to address that.
sdf@google.com writes: > On 08/23, Toke H�iland-J�rgensen wrote: >> YiFei Zhu <zhuyifei@google.com> writes: > >> > On Fri, Aug 21, 2020 at 3:58 AM Toke H�iland-J�rgensen >> <toke@redhat.com> wrote: >> >> Yonghong Song <yhs@fb.com> writes: >> >> > Not sure whether we need formal libbpf API to access metadata or not. >> >> > This may help other applications too. But we can delay until it is >> >> > necessary. >> >> >> >> Yeah, please put in a libbpf accessor as well; I would like to use this >> >> from libxdp - without a skeleton :) >> >> >> >> -Toke >> > >> > I don't think I have an idea on a good API in libbpf that could be >> > used to get the metadata of an existing program in kernel, that could >> > be reused by bpftool without duplicating all the code. Maybe we can >> > discuss this in a follow up series? > >> I think the most important part is getting a reference to the metadata >> map. So a function that basically does what the top half of what your >> show_prog_metadata() function does: given a prog fd, walk the map ids, >> check if any of them looks like a metadata map, and if so return the map >> fd. > >> Should be pretty straight-forward to reuse between bpftool/libbpf, no? > Sounds good, I'll be taking over this patch series as YiFei's internship > has ended. I'll try to address that. Great, thanks! :) -Toke
On Fri, Aug 28, 2020 at 9:59 AM <sdf@google.com> wrote: > > On 08/25, Andrii Nakryiko wrote: > > On Thu, Aug 20, 2020 at 2:44 AM YiFei Zhu <zhuyifei1999@gmail.com> wrote: > > > > > > From: YiFei Zhu <zhuyifei@google.com> > > > > > > Added a flag "--metadata" to `bpftool prog list` to dump the metadata > > > contents. For some formatting some BTF code is put directly in the > > > metadata dumping. Sanity checks on the map and the kind of the btf_type > > > to make sure we are actually dumping what we are expecting. > > > > > > A helper jsonw_reset is added to json writer so we can reuse the same > > > json writer without having extraneous commas. > > > > > > Sample output: > > > > > > $ bpftool prog --metadata > > > 6: cgroup_skb name prog tag bcf7977d3b93787c gpl > > > [...] > > > btf_id 4 > > > metadata: > > > metadata_a = "foo" > > > metadata_b = 1 > > > > > > $ bpftool prog --metadata --json --pretty > > > [{ > > > "id": 6, > > > [...] > > > "btf_id": 4, > > > "metadata": { > > > "metadata_a": "foo", > > > "metadata_b": 1 > > > } > > > } > > > ] > > > > > > Signed-off-by: YiFei Zhu <zhuyifei@google.com> > > > --- > > > tools/bpf/bpftool/json_writer.c | 6 ++ > > > tools/bpf/bpftool/json_writer.h | 3 + > > > tools/bpf/bpftool/main.c | 10 +++ > > > tools/bpf/bpftool/main.h | 1 + > > > tools/bpf/bpftool/prog.c | 135 ++++++++++++++++++++++++++++++++ > > > 5 files changed, 155 insertions(+) > > > > > > [...] > > > > + for (i = 0; i < prog_info.nr_map_ids; i++) { > > > + map_fd = bpf_map_get_fd_by_id(map_ids[i]); > > > + if (map_fd < 0) > > > + return; > > > + > > > + err = bpf_obj_get_info_by_fd(map_fd, &map_info, > > &map_info_len); > > > + if (err) > > > + goto out_close; > > > + > > > + if (map_info.type != BPF_MAP_TYPE_ARRAY) > > > + goto next_map; > > > + if (map_info.key_size != sizeof(int)) > > > + goto next_map; > > > + if (map_info.max_entries != 1) > > > + goto next_map; > > > + if (!map_info.btf_value_type_id) > > > + goto next_map; > > > + if (!strstr(map_info.name, ".metadata")) > > > This substring check sucks. Let's make libbpf call this map strictly > > ".metadata". Current convention of "some part of object name" + "." + > > {rodata,data,bss} is extremely confusing. In practice it's something > > incomprehensible and "unguessable" like "test_pr.rodata". I think it > > makes sense to call them just ".data", ".rodata", ".bss", and > > ".metadata". But that might break existing apps that do lookups based > > on map name (and might break skeleton as it is today, not sure). But > > let's at least start with ".metadata", as it's a new map and we can > > get it right from the start. > Isn't it bad from the consistency point of view? Even if it's bad, > at least it's consistent :-/ Just because we made a mistake once, doesn't mean we need to keep making it. ".metadata" is 9 characters already, which leaves 6 characters for object name prefix, that's not a lot of useful information anyway. As I said, we should probably fix it for other global data maps as well, but we will have to do it gradually. For .metadata we can do a nice and clean ".metadata" immediately, no need to jump through migration and deprecation hoops. Also, how is "test_vml.bss" consistent with "test_v.metadata"?
diff --git a/tools/bpf/bpftool/json_writer.c b/tools/bpf/bpftool/json_writer.c index 86501cd3c763..7fea83bedf48 100644 --- a/tools/bpf/bpftool/json_writer.c +++ b/tools/bpf/bpftool/json_writer.c @@ -119,6 +119,12 @@ void jsonw_pretty(json_writer_t *self, bool on) self->pretty = on; } +void jsonw_reset(json_writer_t *self) +{ + assert(self->depth == 0); + self->sep = '\0'; +} + /* Basic blocks */ static void jsonw_begin(json_writer_t *self, int c) { diff --git a/tools/bpf/bpftool/json_writer.h b/tools/bpf/bpftool/json_writer.h index 35cf1f00f96c..8ace65cdb92f 100644 --- a/tools/bpf/bpftool/json_writer.h +++ b/tools/bpf/bpftool/json_writer.h @@ -27,6 +27,9 @@ void jsonw_destroy(json_writer_t **self_p); /* Cause output to have pretty whitespace */ void jsonw_pretty(json_writer_t *self, bool on); +/* Reset separator to create new JSON */ +void jsonw_reset(json_writer_t *self); + /* Add property name */ void jsonw_name(json_writer_t *self, const char *name); diff --git a/tools/bpf/bpftool/main.c b/tools/bpf/bpftool/main.c index 4a191fcbeb82..a681d568cfa7 100644 --- a/tools/bpf/bpftool/main.c +++ b/tools/bpf/bpftool/main.c @@ -28,6 +28,7 @@ bool show_pinned; bool block_mount; bool verifier_logs; bool relaxed_maps; +bool dump_metadata; struct pinned_obj_table prog_table; struct pinned_obj_table map_table; struct pinned_obj_table link_table; @@ -351,6 +352,10 @@ static int do_batch(int argc, char **argv) return err; } +enum bpftool_longonly_opts { + OPT_METADATA = 256, +}; + int main(int argc, char **argv) { static const struct option options[] = { @@ -362,6 +367,7 @@ int main(int argc, char **argv) { "mapcompat", no_argument, NULL, 'm' }, { "nomount", no_argument, NULL, 'n' }, { "debug", no_argument, NULL, 'd' }, + { "metadata", no_argument, NULL, OPT_METADATA }, { 0 } }; int opt, ret; @@ -371,6 +377,7 @@ int main(int argc, char **argv) json_output = false; show_pinned = false; block_mount = false; + dump_metadata = false; bin_name = argv[0]; hash_init(prog_table.table); @@ -412,6 +419,9 @@ int main(int argc, char **argv) libbpf_set_print(print_all_levels); verifier_logs = true; break; + case OPT_METADATA: + dump_metadata = true; + break; default: p_err("unrecognized option '%s'", argv[optind - 1]); if (json_output) diff --git a/tools/bpf/bpftool/main.h b/tools/bpf/bpftool/main.h index c46e52137b87..8750758e9150 100644 --- a/tools/bpf/bpftool/main.h +++ b/tools/bpf/bpftool/main.h @@ -90,6 +90,7 @@ extern bool show_pids; extern bool block_mount; extern bool verifier_logs; extern bool relaxed_maps; +extern bool dump_metadata; extern struct pinned_obj_table prog_table; extern struct pinned_obj_table map_table; extern struct pinned_obj_table link_table; diff --git a/tools/bpf/bpftool/prog.c b/tools/bpf/bpftool/prog.c index d393eb8263a6..ee767b8d90fb 100644 --- a/tools/bpf/bpftool/prog.c +++ b/tools/bpf/bpftool/prog.c @@ -151,6 +151,135 @@ static void show_prog_maps(int fd, __u32 num_maps) } } +static void show_prog_metadata(int fd, __u32 num_maps) +{ + struct bpf_prog_info prog_info = {}; + struct bpf_map_info map_info = {}; + __u32 prog_info_len = sizeof(prog_info); + __u32 map_info_len = sizeof(map_info); + __u32 map_ids[num_maps]; + void *value = NULL; + struct btf *btf = NULL; + const struct btf_type *t_datasec, *t_var; + struct btf_var_secinfo *vsi; + int key = 0; + unsigned int i, vlen; + int map_fd; + int err; + + prog_info.nr_map_ids = num_maps; + prog_info.map_ids = ptr_to_u64(map_ids); + + err = bpf_obj_get_info_by_fd(fd, &prog_info, &prog_info_len); + if (err || !prog_info.nr_map_ids) + return; + + for (i = 0; i < prog_info.nr_map_ids; i++) { + map_fd = bpf_map_get_fd_by_id(map_ids[i]); + if (map_fd < 0) + return; + + err = bpf_obj_get_info_by_fd(map_fd, &map_info, &map_info_len); + if (err) + goto out_close; + + if (map_info.type != BPF_MAP_TYPE_ARRAY) + goto next_map; + if (map_info.key_size != sizeof(int)) + goto next_map; + if (map_info.max_entries != 1) + goto next_map; + if (!map_info.btf_value_type_id) + goto next_map; + if (!strstr(map_info.name, ".metadata")) + goto next_map; + + goto found; + +next_map: + close(map_fd); + } + + return; + +found: + value = malloc(map_info.value_size); + if (!value) + goto out_close; + + if (bpf_map_lookup_elem(map_fd, &key, value)) + goto out_free; + + err = btf__get_from_id(map_info.btf_id, &btf); + if (err || !btf) + goto out_free; + + t_datasec = btf__type_by_id(btf, map_info.btf_value_type_id); + if (BTF_INFO_KIND(t_datasec->info) != BTF_KIND_DATASEC) + goto out_free; + + vlen = BTF_INFO_VLEN(t_datasec->info); + vsi = (struct btf_var_secinfo *)(t_datasec + 1); + + if (json_output) { + struct btf_dumper d = { + .btf = btf, + .jw = json_wtr, + .is_plain_text = false, + }; + + jsonw_name(json_wtr, "metadata"); + + jsonw_start_object(json_wtr); + for (i = 0; i < vlen; i++) { + t_var = btf__type_by_id(btf, vsi[i].type); + + if (BTF_INFO_KIND(t_var->info) != BTF_KIND_VAR) + continue; + + jsonw_name(json_wtr, btf__name_by_offset(btf, t_var->name_off)); + err = btf_dumper_type(&d, t_var->type, value + vsi[i].offset); + if (err) + break; + } + jsonw_end_object(json_wtr); + } else { + json_writer_t *btf_wtr = jsonw_new(stdout); + struct btf_dumper d = { + .btf = btf, + .jw = btf_wtr, + .is_plain_text = true, + }; + if (!btf_wtr) + goto out_free; + + printf("\tmetadata:"); + + for (i = 0; i < vlen; i++) { + t_var = btf__type_by_id(btf, vsi[i].type); + + if (BTF_INFO_KIND(t_var->info) != BTF_KIND_VAR) + continue; + + printf("\n\t\t%s = ", btf__name_by_offset(btf, t_var->name_off)); + + jsonw_reset(btf_wtr); + err = btf_dumper_type(&d, t_var->type, value + vsi[i].offset); + if (err) + break; + } + + jsonw_destroy(&btf_wtr); + } + +out_free: + btf__free(btf); + free(value); + +out_close: + close(map_fd); +} + static void print_prog_header_json(struct bpf_prog_info *info) { jsonw_uint_field(json_wtr, "id", info->id); @@ -228,6 +357,9 @@ static void print_prog_json(struct bpf_prog_info *info, int fd) emit_obj_refs_json(&refs_table, info->id, json_wtr); + if (dump_metadata) + show_prog_metadata(fd, info->nr_map_ids); + jsonw_end_object(json_wtr); } @@ -297,6 +429,9 @@ static void print_prog_plain(struct bpf_prog_info *info, int fd) emit_obj_refs_plain(&refs_table, info->id, "\n\tpids "); printf("\n"); + + if (dump_metadata) + show_prog_metadata(fd, info->nr_map_ids); } static int show_prog(int fd)