| 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: <003701d353b0$449d37e0$cdd7a7a0$@lab.ntt.co.jp>
Date: Thu, 2 Nov 2017 16:57:45 +0900
From: "Prashant Bhole" <bhole_prashant_q7@....ntt.co.jp>
To: "'Quentin Monnet'" <quentin.monnet@...ronome.com>
Cc: <netdev@...r.kernel.org>, "'Alexei Starovoitov'" <ast@...nel.org>,
"'Daniel Borkmann'" <daniel@...earbox.net>,
"'Jakub Kicinski'" <jakub.kicinski@...ronome.com>,
"'David S . Miller'" <davem@...emloft.net>
Subject: RE: [PATCH net-next 2/3] tools: bpftool: show filenames of pinned objects
> From: Quentin Monnet [mailto:quentin.monnet@...ronome.com]
>
> 2017-10-31 16:36 UTC+0900 ~ Prashant Bhole
> <bhole_prashant_q7@....ntt.co.jp>
> > Added support to show filenames of pinned objects.
> >
> > For example:
> >
> > root@...t# ./bpftool prog
> > 3: tracepoint name tracepoint__irq tag f677a7dd722299a3
> > loaded_at Oct 26/11:39 uid 0
> > xlated 160B not jited memlock 4096B map_ids 4
> > pinned /sys/fs/bpf/softirq_prog
> >
> > 4: tracepoint name tracepoint__irq tag ea5dc530d00b92b6
> > loaded_at Oct 26/11:39 uid 0
> > xlated 392B not jited memlock 4096B map_ids 4,6
> >
> > root@...t# ./bpftool --json --pretty prog [{
> > "id": 3,
> > "type": "tracepoint",
> > "name": "tracepoint__irq",
> > "tag": "f677a7dd722299a3",
> > "loaded_at": "Oct 26/11:39",
> > "uid": 0,
> > "bytes_xlated": 160,
> > "jited": false,
> > "bytes_memlock": 4096,
> > "map_ids": [4
> > ]
> > ,
> > "pinned": ["/sys/fs/bpf/softirq_prog"
> > ]
> > },{
> > "id": 4,
> > "type": "tracepoint",
> > "name": "tracepoint__irq",
> > "tag": "ea5dc530d00b92b6",
> > "loaded_at": "Oct 26/11:39",
> > "uid": 0,
> > "bytes_xlated": 392,
> > "jited": false,
> > "bytes_memlock": 4096,
> > "map_ids": [4,6
> > ]
> > ,
> > "pinned": []
> > }
> > ]
> >
> > root@...t# ./bpftool map
> > 4: hash name start flags 0x0
> > key 4B value 16B max_entries 10240 memlock 1003520B
> > pinned /sys/fs/bpf/softirq_map1
> > 5: hash name iptr flags 0x0
> > key 4B value 8B max_entries 10240 memlock 921600B
> >
> > root@...t# ./bpftool --json --pretty map [{
> > "id": 4,
> > "type": "hash",
> > "name": "start",
> > "flags": 0,
> > "bytes_key": 4,
> > "bytes_value": 16,
> > "max_entries": 10240,
> > "bytes_memlock": 1003520,
> > "pinned": ["/sys/fs/bpf/softirq_map1"
> > ]
> > },{
> > "id": 5,
> > "type": "hash",
> > "name": "iptr",
> > "flags": 0,
> > "bytes_key": 4,
> > "bytes_value": 8,
> > "max_entries": 10240,
> > "bytes_memlock": 921600,
> > "pinned": []
> > }
> > ]
> >
> > Signed-off-by: Prashant Bhole <bhole_prashant_q7@....ntt.co.jp>
> > ---
> > tools/bpf/bpftool/common.c | 70
> ++++++++++++++++++++++++++++++++++++++++++++++
> > tools/bpf/bpftool/main.c | 8 ++++++
> > tools/bpf/bpftool/main.h | 15 ++++++++++
> > tools/bpf/bpftool/map.c | 22 ++++++++++++++-
> > tools/bpf/bpftool/prog.c | 23 +++++++++++++++
> > 5 files changed, 137 insertions(+), 1 deletion(-)
> >
> > diff --git a/tools/bpf/bpftool/common.c b/tools/bpf/bpftool/common.c
> > index 4556947..6b6247c 100644
> > --- a/tools/bpf/bpftool/common.c
> > +++ b/tools/bpf/bpftool/common.c
> > @@ -45,6 +45,7 @@
> > #include <sys/mount.h>
> > #include <sys/types.h>
> > #include <sys/vfs.h>
> > +#include <fts.h>
> >
> > #include <bpf.h>
> >
> > @@ -290,3 +291,72 @@ void print_hex_data_json(uint8_t *data, size_t len)
> > jsonw_printf(json_wtr, "\"0x%02hhx\"", data[i]);
> > jsonw_end_array(json_wtr);
> > }
> > +
> > +int build_pinned_obj_table(struct pinned_obj_table *tab, enum
> > +bpf_obj_type type)
>
> Nit: line over 80 characters, please wrap.
>
> > +{
> > + char *path[] = {"/sys/fs/bpf/", NULL};
>
> This will not work for progs/maps pinned elsewhere on the system. Might be
> worth mentioning in the documentation?
>
> > + FTS *ftsp = NULL;
> > + FTSENT *ftse = NULL;
> > + enum bpf_obj_type objtype;
> > + struct bpf_prog_info pinned_info = {};
> > + __u32 len = sizeof(pinned_info);
> > + int fd, err;
> > + struct pinned_obj *obj_node;
>
> Please use reversed-Christmas tree style.
>
> > +
> > + if (!is_bpffs(path[0]))
> > + return -1;
> > +
> > + ftsp = fts_open(path, 0, NULL);
> > + if (!ftsp)
> > + return -1;
> > +
> > + while ((ftse = fts_read(ftsp)) != NULL) {
> > + if (!(ftse->fts_info & FTS_F))
> > + continue;
> > + fd = open_obj_pinned(ftse->fts_path);
> > + if (fd < 0)
> > + continue;
> > +
> > + objtype = get_fd_type(fd);
> > + if (objtype != type) {
> > + close(fd);
> > + continue;
> > + }
> > + memset(&pinned_info, 0, sizeof(pinned_info));
> > + err = bpf_obj_get_info_by_fd(fd, &pinned_info, &len);
> > + if (err) {
> > + close(fd);
> > + continue;
> > + }
> > +
> > + obj_node = malloc(sizeof(*obj_node));
> > + if (!obj_node)
> > + return -1;
>
> Maybe fd and ftsp should be closed before returning?
>
> > +
> > + memset(obj_node, 0, sizeof(*obj_node));
> > + obj_node->id = pinned_info.id;
> > + obj_node->path = strdup(ftse->fts_path);
> > + hash_add(tab->table, &obj_node->hash, obj_node->id);
> > +
> > + close(fd);
> > + }
> > +
> > + fts_close(ftsp);
> > + return 0;
> > +}
> > +
> > +void delete_pinned_obj_table(struct pinned_obj_table *tab) {
> > + struct pinned_obj *obj;
> > + struct hlist_node *tmp;
> > + unsigned int bkt;
> > +
> > + if (hash_empty(tab->table))
> > + return;
> > +
> > + hash_for_each_safe(tab->table, bkt, tmp, obj, hash) {
> > + hash_del(&obj->hash);
> > + free(obj->path);
> > + free(obj);
> > + }
> > +}
> > diff --git a/tools/bpf/bpftool/main.c b/tools/bpf/bpftool/main.c index
> > 78d9afb..6ad53f1 100644
> > --- a/tools/bpf/bpftool/main.c
> > +++ b/tools/bpf/bpftool/main.c
> > @@ -54,6 +54,8 @@
> > json_writer_t *json_wtr;
> > bool pretty_output;
> > bool json_output;
> > +struct pinned_obj_table prog_table;
> > +struct pinned_obj_table map_table;
> >
> > void usage(void)
> > {
> > @@ -272,6 +274,9 @@ int main(int argc, char **argv)
> > json_output = false;
> > bin_name = argv[0];
> >
> > + hash_init(prog_table.table);
> > + hash_init(map_table.table);
> > +
> > while ((opt = getopt_long(argc, argv, "Vhpj",
> > options, NULL)) >= 0) {
> > switch (opt) {
> > @@ -311,5 +316,8 @@ int main(int argc, char **argv)
> > if (json_output)
> > jsonw_destroy(&json_wtr);
> >
> > + delete_pinned_obj_table(&prog_table);
> > + delete_pinned_obj_table(&map_table);
> > +
> > return ret;
> > }
> > diff --git a/tools/bpf/bpftool/main.h b/tools/bpf/bpftool/main.h index
> > 4b56850..3abaa17 100644
> > --- a/tools/bpf/bpftool/main.h
> > +++ b/tools/bpf/bpftool/main.h
> > @@ -42,6 +42,7 @@
> > #include <stdio.h>
> > #include <linux/bpf.h>
> > #include <linux/kernel.h>
> > +#include <linux/hashtable.h>
> >
> > #include "json_writer.h"
> >
> > @@ -70,11 +71,25 @@ enum bpf_obj_type {
> >
> > extern json_writer_t *json_wtr;
> > extern bool json_output;
> > +extern struct pinned_obj_table prog_table; extern struct
> > +pinned_obj_table map_table;
> >
> > bool is_prefix(const char *pfx, const char *str); void
> > fprint_hex(FILE *f, void *arg, unsigned int n, const char *sep); void
> > usage(void) __attribute__((noreturn));
> >
> > +struct pinned_obj_table {
> > + DECLARE_HASHTABLE(table, 16);
> > +};
> > +
> > +struct pinned_obj {
> > + __u32 id;
> > + char *path;
> > + struct hlist_node hash;
> > +};
>
> Nit: add a blank line here?
>
> > +int build_pinned_obj_table(struct pinned_obj_table *table, enum
> > +bpf_obj_type type);
>
> Please wrap this line as well.
>
> > +void delete_pinned_obj_table(struct pinned_obj_table *tab);
> > +
> > struct cmd {
> > const char *cmd;
> > int (*func)(int argc, char **argv);
> > diff --git a/tools/bpf/bpftool/map.c b/tools/bpf/bpftool/map.c index
> > e978ab2..fbe236e 100644
> > --- a/tools/bpf/bpftool/map.c
> > +++ b/tools/bpf/bpftool/map.c
> > @@ -436,6 +436,18 @@ static int show_map_close_json(int fd, struct
> bpf_map_info *info)
> > jsonw_int_field(json_wtr, "bytes_memlock", atoi(memlock));
> > free(memlock);
> >
> > + if (!hash_empty(map_table.table)) {
> > + struct pinned_obj *obj;
> > +
> > + jsonw_name(json_wtr, "pinned");
> > + jsonw_start_array(json_wtr);
> > + hash_for_each_possible(map_table.table, obj, hash, info->id) {
> > + if (obj->id == info->id)
> > + jsonw_string(json_wtr, obj->path);
> > + }
> > + jsonw_end_array(json_wtr);
> > + }
> > +
> > jsonw_end_object(json_wtr);
> >
> > return 0;
> > @@ -466,7 +478,13 @@ static int show_map_close_plain(int fd, struct
> bpf_map_info *info)
> > free(memlock);
> >
> > printf("\n");
> > -
> > + if (!hash_empty(map_table.table)) {
> > + struct pinned_obj *obj;
>
> Nit: add a blank line after declarations.
>
> > + hash_for_each_possible(map_table.table, obj, hash, info->id) {
> > + if (obj->id == info->id)
> > + printf("\tpinned %s\n", obj->path);
> > + }
> > + }
> > return 0;
> > }
> >
> > @@ -478,6 +496,8 @@ static int do_show(int argc, char **argv)
> > int err;
> > int fd;
> >
> > + build_pinned_obj_table(&map_table, BPF_OBJ_MAP);
> > +
> > if (argc == 2) {
> > fd = map_parse_fd_and_info(&argc, &argv, &info, &len);
> > if (fd < 0)
> > diff --git a/tools/bpf/bpftool/prog.c b/tools/bpf/bpftool/prog.c index
> > 250f80f..cd29eae 100644
> > --- a/tools/bpf/bpftool/prog.c
> > +++ b/tools/bpf/bpftool/prog.c
> > @@ -207,6 +207,7 @@ static void show_prog_maps(int fd, u32 num_maps)
> > printf("%u%s", map_ids[i],
> > i == info.nr_map_ids - 1 ? "" : ",");
> > }
> > + printf("\n");
>
> This printf() should not be here, because it would add a new line even if there is
> no pinned path to add, and regardless of plain / JSON output selection. It is
> responsible for the line breaks before the commas you have in your commit log
> example.
>
> > }
> >
> > static void print_prog_json(struct bpf_prog_info *info, int fd) @@
> > -256,6 +257,18 @@ static void print_prog_json(struct bpf_prog_info *info, int
> fd)
> > if (info->nr_map_ids)
> > show_prog_maps(fd, info->nr_map_ids);
> >
> > + if (!hash_empty(prog_table.table)) {
> > + struct pinned_obj *obj;
> > +
> > + jsonw_name(json_wtr, "pinned");
> > + jsonw_start_array(json_wtr);
> > + hash_for_each_possible(prog_table.table, obj, hash, info->id) {
> > + if (obj->id == info->id)
> > + jsonw_string(json_wtr, obj->path);
> > + }
> > + jsonw_end_array(json_wtr);
> > + }
> > +
> > jsonw_end_object(json_wtr);
> > }
> >
> > @@ -300,6 +313,14 @@ static void print_prog_plain(struct bpf_prog_info
> *info, int fd)
> > if (info->nr_map_ids)
> > show_prog_maps(fd, info->nr_map_ids);
> >
> > + if (!hash_empty(prog_table.table)) {
> > + struct pinned_obj *obj;
>
> Nit: add a blank line after declarations.
> Here might be a better place for the printf("\n");?
>
> > + hash_for_each_possible(prog_table.table, obj, hash, info->id) {
> > + if (obj->id == info->id)
> > + printf("\tpinned %s\n", obj->path);
> > + }
> > + }
> > +
> > printf("\n");
> > }
> >
> > @@ -329,6 +350,8 @@ static int do_show(int argc, char **argv)
> > int err;
> > int fd;
> >
> > + build_pinned_obj_table(&prog_table, BPF_OBJ_PROG);
> > +
> > if (argc == 2) {
> > fd = prog_parse_fd(&argc, &argv);
> > if (fd < 0)
>
> Quentin
All requested changes by you and Jakub are done in v2 patch series. Posting soon. Thanks.
Prashant
Powered by blists - more mailing lists