Message ID | 20190320173332.18105-3-alban@kinvolk.io |
---|---|
State | Changes Requested |
Delegated to: | BPF Maintainers |
Headers | show |
Series | [bpf-next,v1,1/7] tools: bpftool: fix infinite loop | expand |
Hi Alban, thanks for those patches! 2019-03-20 18:33 UTC+0100 ~ Alban Crequy <alban.crequy@gmail.com> > From: Alban Crequy <alban@kinvolk.io> > > Before this patch, there was no way to fill attr.inner_map_fd, necessary > for array_of_maps or hash_of_maps. > > This patch adds keyword 'innermap' to pass the innermap, either as an id > or as a pinned map. > > Example of commands: > > $ sudo bpftool map create /sys/fs/bpf/innermap type hash \ > key 8 value 8 entries 64 name innermap flags 1 > $ sudo bpftool map create /sys/fs/bpf/outermap type hash_of_maps \ > innermap pinned /sys/fs/bpf/innermap key 64 value 4 \ > entries 64 name myoutermap flags 1 > $ sudo bpftool map show pinned /sys/fs/bpf/outermap > 47: hash_of_maps name myoutermap flags 0x1 > key 64B value 4B max_entries 64 memlock 12288B > > Documentation and bash completion updated as well. > > Signed-off-by: Alban Crequy <alban@kinvolk.io> > > --- > > Previous version of this patch was sent while bpf-next was closed. > https://marc.info/?l=linux-kernel&m=155180393501258&w=2 > > Since then, the following changes were done: > - error management when calling map_parse_fd (review from Jakub) > - fix documentation and bash completion (review from Quentin) > --- > .../bpf/bpftool/Documentation/bpftool-map.rst | 9 ++- > tools/bpf/bpftool/bash-completion/bpftool | 9 +++ > tools/bpf/bpftool/map.c | 75 +++++++++++++++++-- > 3 files changed, 83 insertions(+), 10 deletions(-) > > diff --git a/tools/bpf/bpftool/Documentation/bpftool-map.rst b/tools/bpf/bpftool/Documentation/bpftool-map.rst > index 5c984ffc9f01..b685641bfd74 100644 > --- a/tools/bpf/bpftool/Documentation/bpftool-map.rst > +++ b/tools/bpf/bpftool/Documentation/bpftool-map.rst > @@ -23,7 +23,7 @@ MAP COMMANDS > > | **bpftool** **map { show | list }** [*MAP*] > | **bpftool** **map create** *FILE* **type** *TYPE* **key** *KEY_SIZE* **value** *VALUE_SIZE* \ > -| **entries** *MAX_ENTRIES* **name** *NAME* [**flags** *FLAGS*] [**dev** *NAME*] > +| **entries** *MAX_ENTRIES* **name** *NAME* [**flags** *FLAGS*] [**dev** *NAME*] [**innermap** MAP] > | **bpftool** **map dump** *MAP* > | **bpftool** **map update** *MAP* [**key** *DATA*] [**value** *VALUE*] [*UPDATE_FLAGS*] > | **bpftool** **map lookup** *MAP* [**key** *DATA*] > @@ -60,10 +60,15 @@ DESCRIPTION > Output will start with map ID followed by map type and > zero or more named attributes (depending on kernel version). > > - **bpftool map create** *FILE* **type** *TYPE* **key** *KEY_SIZE* **value** *VALUE_SIZE* **entries** *MAX_ENTRIES* **name** *NAME* [**flags** *FLAGS*] [**dev** *NAME*] > + **bpftool map create** *FILE* **type** *TYPE* **key** *KEY_SIZE* **value** *VALUE_SIZE* **entries** *MAX_ENTRIES* **name** *NAME* [**flags** *FLAGS*] [**dev** *NAME*] [**innermap** MAP] > Create a new map with given parameters and pin it to *bpffs* > as *FILE*. > > + To create a map of type **array_of_maps** or > + **hash_of_maps**, the additional parameter **innermap** needs > + to reference an existing map with the type and size of the > + maps in the values. > + > **bpftool map dump** *MAP* > Dump all entries in a given *MAP*. > > diff --git a/tools/bpf/bpftool/bash-completion/bpftool b/tools/bpf/bpftool/bash-completion/bpftool > index b803827d01e8..9e37de8bb227 100644 > --- a/tools/bpf/bpftool/bash-completion/bpftool > +++ b/tools/bpf/bpftool/bash-completion/bpftool > @@ -461,6 +461,14 @@ _bpftool() > _sysfs_get_netdevs > return 0 > ;; > + innermap) > + COMPREPLY+=( $( compgen -W "id pinned" -- "$cur" ) ) > + return 0 > + ;; > + id) > + _bpftool_get_map_ids > + return 0 > + ;; > *) > _bpftool_once_attr 'type' > _bpftool_once_attr 'key' > @@ -469,6 +477,7 @@ _bpftool() > _bpftool_once_attr 'name' > _bpftool_once_attr 'flags' > _bpftool_once_attr 'dev' > + _bpftool_once_attr 'innermap' > return 0 > ;; > esac > diff --git a/tools/bpf/bpftool/map.c b/tools/bpf/bpftool/map.c > index 18f9bc3aed4f..a576f2a019be 100644 > --- a/tools/bpf/bpftool/map.c > +++ b/tools/bpf/bpftool/map.c > @@ -1100,6 +1100,7 @@ static int do_create(int argc, char **argv) > struct bpf_create_map_attr attr = { NULL, }; > const char *pinfile; > int err, fd; > + int inner_map_fd = -1; Nit: reverse-Christmas tree for declaring the variables. > > if (!REQ_ARGS(7)) > return -1; > @@ -1108,48 +1109,75 @@ static int do_create(int argc, char **argv) > while (argc) { > if (is_prefix(*argv, "type")) { > NEXT_ARG(); > - if (!REQ_ARGS(1)) > + if (!REQ_ARGS(1)) { > + if (inner_map_fd != -1) > + close(inner_map_fd); > return -1; > + } > > if (attr.map_type) { > p_err("map type already specified"); > + if (inner_map_fd != -1) > + close(inner_map_fd); > return -1; > } > > attr.map_type = map_type_from_str(*argv); > if ((int)attr.map_type < 0) { > p_err("unrecognized map type: %s", *argv); > + if (inner_map_fd != -1) > + close(inner_map_fd); > return -1; > } > NEXT_ARG(); > } else if (is_prefix(*argv, "name")) { > NEXT_ARG(); > - if (!REQ_ARGS(1)) > + if (!REQ_ARGS(1)) { > + if (inner_map_fd != -1) > + close(inner_map_fd); > return -1; > + } > attr.name = GET_ARG(); > } else if (is_prefix(*argv, "key")) { > if (parse_u32_arg(&argc, &argv, &attr.key_size, > - "key size")) > + "key size")) { > + if (inner_map_fd != -1) > + close(inner_map_fd); > return -1; > + } > } else if (is_prefix(*argv, "value")) { > if (parse_u32_arg(&argc, &argv, &attr.value_size, > - "value size")) > + "value size")) { > + if (inner_map_fd != -1) > + close(inner_map_fd); > return -1; > + } > } else if (is_prefix(*argv, "entries")) { > if (parse_u32_arg(&argc, &argv, &attr.max_entries, > - "max entries")) > + "max entries")) { > + if (inner_map_fd != -1) > + close(inner_map_fd); > return -1; > + } > } else if (is_prefix(*argv, "flags")) { > if (parse_u32_arg(&argc, &argv, &attr.map_flags, > - "flags")) > + "flags")) { > + if (inner_map_fd != -1) > + close(inner_map_fd); > return -1; > + } > } else if (is_prefix(*argv, "dev")) { > NEXT_ARG(); > - if (!REQ_ARGS(1)) > + if (!REQ_ARGS(1)) { > + if (inner_map_fd != -1) > + close(inner_map_fd); > return -1; > + } > > if (attr.map_ifindex) { > p_err("offload device already specified"); > + if (inner_map_fd != -1) > + close(inner_map_fd); > return -1; > } > > @@ -1157,28 +1185,59 @@ static int do_create(int argc, char **argv) > if (!attr.map_ifindex) { > p_err("unrecognized netdevice '%s': %s", > *argv, strerror(errno)); > + if (inner_map_fd != -1) > + close(inner_map_fd); > return -1; > } > NEXT_ARG(); > + } else if (is_prefix(*argv, "innermap")) { > + NEXT_ARG(); > + if (!REQ_ARGS(1)) { > + if (inner_map_fd != -1) > + close(inner_map_fd); > + return -1; > + } > + > + if (inner_map_fd != -1) { > + close(inner_map_fd); > + p_err("innermap already specified"); > + return -1; > + } > + > + inner_map_fd = map_parse_fd(&argc, &argv); > + if (inner_map_fd < 0) > + return -1; > } else { > p_err("unknown arg %s", *argv); > + if (inner_map_fd != -1) > + close(inner_map_fd); > return -1; > } > } > > if (!attr.name) { > p_err("map name not specified"); > + if (inner_map_fd != -1) > + close(inner_map_fd); > return -1; > } > > set_max_rlimit(); > > + if (inner_map_fd != -1) > + attr.inner_map_fd = inner_map_fd; > + > fd = bpf_create_map_xattr(&attr); > if (fd < 0) { > p_err("map create failed: %s", strerror(errno)); > + if (inner_map_fd != -1) > + close(inner_map_fd); > return -1; You might want to move all those if (inner_map_fd != -1) close(inner_map_fd); return -1; at the end of the function under a label, and jump there with a goto for the 15 or so occurrences of the pattern? > } > > + if (inner_map_fd != -1) > + close(inner_map_fd); > + > err = do_pin_fd(fd, pinfile); > close(fd); > if (err) > @@ -1243,7 +1302,7 @@ static int do_help(int argc, char **argv) > "Usage: %s %s { show | list } [MAP]\n" > " %s %s create FILE type TYPE key KEY_SIZE value VALUE_SIZE \\\n" > " entries MAX_ENTRIES name NAME [flags FLAGS] \\\n" > - " [dev NAME]\n" > + " [dev NAME] [innermap MAP]\n" > " %s %s dump MAP\n" > " %s %s update MAP [key DATA] [value VALUE] [UPDATE_FLAGS]\n" > " %s %s lookup MAP [key DATA]\n" >
diff --git a/tools/bpf/bpftool/Documentation/bpftool-map.rst b/tools/bpf/bpftool/Documentation/bpftool-map.rst index 5c984ffc9f01..b685641bfd74 100644 --- a/tools/bpf/bpftool/Documentation/bpftool-map.rst +++ b/tools/bpf/bpftool/Documentation/bpftool-map.rst @@ -23,7 +23,7 @@ MAP COMMANDS | **bpftool** **map { show | list }** [*MAP*] | **bpftool** **map create** *FILE* **type** *TYPE* **key** *KEY_SIZE* **value** *VALUE_SIZE* \ -| **entries** *MAX_ENTRIES* **name** *NAME* [**flags** *FLAGS*] [**dev** *NAME*] +| **entries** *MAX_ENTRIES* **name** *NAME* [**flags** *FLAGS*] [**dev** *NAME*] [**innermap** MAP] | **bpftool** **map dump** *MAP* | **bpftool** **map update** *MAP* [**key** *DATA*] [**value** *VALUE*] [*UPDATE_FLAGS*] | **bpftool** **map lookup** *MAP* [**key** *DATA*] @@ -60,10 +60,15 @@ DESCRIPTION Output will start with map ID followed by map type and zero or more named attributes (depending on kernel version). - **bpftool map create** *FILE* **type** *TYPE* **key** *KEY_SIZE* **value** *VALUE_SIZE* **entries** *MAX_ENTRIES* **name** *NAME* [**flags** *FLAGS*] [**dev** *NAME*] + **bpftool map create** *FILE* **type** *TYPE* **key** *KEY_SIZE* **value** *VALUE_SIZE* **entries** *MAX_ENTRIES* **name** *NAME* [**flags** *FLAGS*] [**dev** *NAME*] [**innermap** MAP] Create a new map with given parameters and pin it to *bpffs* as *FILE*. + To create a map of type **array_of_maps** or + **hash_of_maps**, the additional parameter **innermap** needs + to reference an existing map with the type and size of the + maps in the values. + **bpftool map dump** *MAP* Dump all entries in a given *MAP*. diff --git a/tools/bpf/bpftool/bash-completion/bpftool b/tools/bpf/bpftool/bash-completion/bpftool index b803827d01e8..9e37de8bb227 100644 --- a/tools/bpf/bpftool/bash-completion/bpftool +++ b/tools/bpf/bpftool/bash-completion/bpftool @@ -461,6 +461,14 @@ _bpftool() _sysfs_get_netdevs return 0 ;; + innermap) + COMPREPLY+=( $( compgen -W "id pinned" -- "$cur" ) ) + return 0 + ;; + id) + _bpftool_get_map_ids + return 0 + ;; *) _bpftool_once_attr 'type' _bpftool_once_attr 'key' @@ -469,6 +477,7 @@ _bpftool() _bpftool_once_attr 'name' _bpftool_once_attr 'flags' _bpftool_once_attr 'dev' + _bpftool_once_attr 'innermap' return 0 ;; esac diff --git a/tools/bpf/bpftool/map.c b/tools/bpf/bpftool/map.c index 18f9bc3aed4f..a576f2a019be 100644 --- a/tools/bpf/bpftool/map.c +++ b/tools/bpf/bpftool/map.c @@ -1100,6 +1100,7 @@ static int do_create(int argc, char **argv) struct bpf_create_map_attr attr = { NULL, }; const char *pinfile; int err, fd; + int inner_map_fd = -1; if (!REQ_ARGS(7)) return -1; @@ -1108,48 +1109,75 @@ static int do_create(int argc, char **argv) while (argc) { if (is_prefix(*argv, "type")) { NEXT_ARG(); - if (!REQ_ARGS(1)) + if (!REQ_ARGS(1)) { + if (inner_map_fd != -1) + close(inner_map_fd); return -1; + } if (attr.map_type) { p_err("map type already specified"); + if (inner_map_fd != -1) + close(inner_map_fd); return -1; } attr.map_type = map_type_from_str(*argv); if ((int)attr.map_type < 0) { p_err("unrecognized map type: %s", *argv); + if (inner_map_fd != -1) + close(inner_map_fd); return -1; } NEXT_ARG(); } else if (is_prefix(*argv, "name")) { NEXT_ARG(); - if (!REQ_ARGS(1)) + if (!REQ_ARGS(1)) { + if (inner_map_fd != -1) + close(inner_map_fd); return -1; + } attr.name = GET_ARG(); } else if (is_prefix(*argv, "key")) { if (parse_u32_arg(&argc, &argv, &attr.key_size, - "key size")) + "key size")) { + if (inner_map_fd != -1) + close(inner_map_fd); return -1; + } } else if (is_prefix(*argv, "value")) { if (parse_u32_arg(&argc, &argv, &attr.value_size, - "value size")) + "value size")) { + if (inner_map_fd != -1) + close(inner_map_fd); return -1; + } } else if (is_prefix(*argv, "entries")) { if (parse_u32_arg(&argc, &argv, &attr.max_entries, - "max entries")) + "max entries")) { + if (inner_map_fd != -1) + close(inner_map_fd); return -1; + } } else if (is_prefix(*argv, "flags")) { if (parse_u32_arg(&argc, &argv, &attr.map_flags, - "flags")) + "flags")) { + if (inner_map_fd != -1) + close(inner_map_fd); return -1; + } } else if (is_prefix(*argv, "dev")) { NEXT_ARG(); - if (!REQ_ARGS(1)) + if (!REQ_ARGS(1)) { + if (inner_map_fd != -1) + close(inner_map_fd); return -1; + } if (attr.map_ifindex) { p_err("offload device already specified"); + if (inner_map_fd != -1) + close(inner_map_fd); return -1; } @@ -1157,28 +1185,59 @@ static int do_create(int argc, char **argv) if (!attr.map_ifindex) { p_err("unrecognized netdevice '%s': %s", *argv, strerror(errno)); + if (inner_map_fd != -1) + close(inner_map_fd); return -1; } NEXT_ARG(); + } else if (is_prefix(*argv, "innermap")) { + NEXT_ARG(); + if (!REQ_ARGS(1)) { + if (inner_map_fd != -1) + close(inner_map_fd); + return -1; + } + + if (inner_map_fd != -1) { + close(inner_map_fd); + p_err("innermap already specified"); + return -1; + } + + inner_map_fd = map_parse_fd(&argc, &argv); + if (inner_map_fd < 0) + return -1; } else { p_err("unknown arg %s", *argv); + if (inner_map_fd != -1) + close(inner_map_fd); return -1; } } if (!attr.name) { p_err("map name not specified"); + if (inner_map_fd != -1) + close(inner_map_fd); return -1; } set_max_rlimit(); + if (inner_map_fd != -1) + attr.inner_map_fd = inner_map_fd; + fd = bpf_create_map_xattr(&attr); if (fd < 0) { p_err("map create failed: %s", strerror(errno)); + if (inner_map_fd != -1) + close(inner_map_fd); return -1; } + if (inner_map_fd != -1) + close(inner_map_fd); + err = do_pin_fd(fd, pinfile); close(fd); if (err) @@ -1243,7 +1302,7 @@ static int do_help(int argc, char **argv) "Usage: %s %s { show | list } [MAP]\n" " %s %s create FILE type TYPE key KEY_SIZE value VALUE_SIZE \\\n" " entries MAX_ENTRIES name NAME [flags FLAGS] \\\n" - " [dev NAME]\n" + " [dev NAME] [innermap MAP]\n" " %s %s dump MAP\n" " %s %s update MAP [key DATA] [value VALUE] [UPDATE_FLAGS]\n" " %s %s lookup MAP [key DATA]\n"