* [PATCH bpf-next 0/2] tools: bpftool: support creating and dumping outer maps
@ 2020-09-04 16:13 Quentin Monnet
2020-09-04 16:13 ` [PATCH bpf-next 1/2] tools: bpftool: dump outer maps content Quentin Monnet
2020-09-04 16:13 ` [PATCH bpf-next 2/2] tools: bpftool: add "inner_map" to "bpftool map create" outer maps Quentin Monnet
0 siblings, 2 replies; 6+ messages in thread
From: Quentin Monnet @ 2020-09-04 16:13 UTC (permalink / raw)
To: Alexei Starovoitov, Daniel Borkmann
Cc: bpf, netdev, Martynas Pumputis, Quentin Monnet
This series makes bpftool able to create and dump the content for outer
maps (maps of types array-of-maps and hash-of-maps). The modifications are
rather succinct: dumping works if we remove the related restriction in
bpftool's code, and creation is just a matter of passing the relevant
inner_map_fd, which we do through a new command-line keyword.
Quentin Monnet (2):
tools: bpftool: dump outer maps content
tools: bpftool: add "inner_map" to "bpftool map create" outer maps
.../bpf/bpftool/Documentation/bpftool-map.rst | 10 +++-
tools/bpf/bpftool/bash-completion/bpftool | 22 +++++++-
tools/bpf/bpftool/map.c | 52 ++++++++++++-------
3 files changed, 62 insertions(+), 22 deletions(-)
--
2.25.1
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH bpf-next 1/2] tools: bpftool: dump outer maps content
2020-09-04 16:13 [PATCH bpf-next 0/2] tools: bpftool: support creating and dumping outer maps Quentin Monnet
@ 2020-09-04 16:13 ` Quentin Monnet
2020-09-04 22:03 ` Andrii Nakryiko
2020-09-04 16:13 ` [PATCH bpf-next 2/2] tools: bpftool: add "inner_map" to "bpftool map create" outer maps Quentin Monnet
1 sibling, 1 reply; 6+ messages in thread
From: Quentin Monnet @ 2020-09-04 16:13 UTC (permalink / raw)
To: Alexei Starovoitov, Daniel Borkmann
Cc: bpf, netdev, Martynas Pumputis, Quentin Monnet
Although user space can lookup and dump the content of an outer map
(hash-of-maps or array-of-maps), bpftool does not allow to do so.
It seems that the only reason for that is historical. Lookups for outer
maps was added in commit 14dc6f04f49d ("bpf: Add syscall lookup support
for fd array and htab"), and although the relevant code in bpftool had
not been merged yet, I suspect it had already been written with the
assumption that user space could not read outer maps.
Let's remove the restriction, dump for outer maps works with no further
change.
Reported-by: Martynas Pumputis <m@lambda.lt>
Signed-off-by: Quentin Monnet <quentin@isovalent.com>
---
tools/bpf/bpftool/map.c | 4 ----
1 file changed, 4 deletions(-)
diff --git a/tools/bpf/bpftool/map.c b/tools/bpf/bpftool/map.c
index bc0071228f88..cb3a75eb5531 100644
--- a/tools/bpf/bpftool/map.c
+++ b/tools/bpf/bpftool/map.c
@@ -739,10 +739,6 @@ static int dump_map_elem(int fd, void *key, void *value,
/* lookup error handling */
lookup_errno = errno;
- if (map_is_map_of_maps(map_info->type) ||
- map_is_map_of_progs(map_info->type))
- return 0;
-
if (json_output) {
jsonw_start_object(json_wtr);
jsonw_name(json_wtr, "key");
--
2.25.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH bpf-next 2/2] tools: bpftool: add "inner_map" to "bpftool map create" outer maps
2020-09-04 16:13 [PATCH bpf-next 0/2] tools: bpftool: support creating and dumping outer maps Quentin Monnet
2020-09-04 16:13 ` [PATCH bpf-next 1/2] tools: bpftool: dump outer maps content Quentin Monnet
@ 2020-09-04 16:13 ` Quentin Monnet
2020-09-04 21:55 ` Andrii Nakryiko
1 sibling, 1 reply; 6+ messages in thread
From: Quentin Monnet @ 2020-09-04 16:13 UTC (permalink / raw)
To: Alexei Starovoitov, Daniel Borkmann
Cc: bpf, netdev, Martynas Pumputis, Quentin Monnet
There is no support for creating maps of types array-of-map or
hash-of-map in bpftool. This is because the kernel needs an inner_map_fd
to collect metadata on the inner maps to be supported by the new map,
but bpftool does not provide a way to pass this file descriptor.
Add a new optional "inner_map" keyword that can be used to pass a
reference to a map, retrieve a fd to that map, and pass it as the
inner_map_fd.
Add related documentation and bash completion. Note that we can
reference the inner map by its name, meaning we can have several times
the keyword "name" with different meanings (mandatory outer map name,
and possibly a name to use to find the inner_map_fd). The bash
completion will offer it just once, and will not suggest "name" on the
following command:
# bpftool map create /sys/fs/bpf/my_outer_map type hash_of_maps \
inner_map name my_inner_map [TAB]
Fixing that specific case seems too convoluted. Completion will work as
expected, however, if the outer map name comes first and the "inner_map
name ..." is passed second.
Signed-off-by: Quentin Monnet <quentin@isovalent.com>
---
.../bpf/bpftool/Documentation/bpftool-map.rst | 10 +++-
tools/bpf/bpftool/bash-completion/bpftool | 22 ++++++++-
tools/bpf/bpftool/map.c | 48 +++++++++++++------
3 files changed, 62 insertions(+), 18 deletions(-)
diff --git a/tools/bpf/bpftool/Documentation/bpftool-map.rst b/tools/bpf/bpftool/Documentation/bpftool-map.rst
index 083db6c2fc67..ca9d62d7e0bd 100644
--- a/tools/bpf/bpftool/Documentation/bpftool-map.rst
+++ b/tools/bpf/bpftool/Documentation/bpftool-map.rst
@@ -23,7 +23,8 @@ 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*] [**inner_map** *MAP*] \
+| [**dev** *NAME*]
| **bpftool** **map dump** *MAP*
| **bpftool** **map update** *MAP* [**key** *DATA*] [**value** *VALUE*] [*UPDATE_FLAGS*]
| **bpftool** **map lookup** *MAP* [**key** *DATA*]
@@ -67,7 +68,7 @@ DESCRIPTION
maps. On such kernels bpftool will automatically emit this
information as well.
- **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*] [**inner_map** *MAP*] [**dev** *NAME*]
Create a new map with given parameters and pin it to *bpffs*
as *FILE*.
@@ -75,6 +76,11 @@ DESCRIPTION
desired flags, e.g. 1024 for **BPF_F_MMAPABLE** (see bpf.h
UAPI header for existing flags).
+ To create maps of type array-of-maps or hash-of-maps, the
+ **inner_map** keyword must be used to pass an inner map. The
+ kernel needs it to collect metadata related to the inner maps
+ that the new map will work with.
+
Keyword **dev** expects a network interface name, and is used
to request hardware offload for the map.
diff --git a/tools/bpf/bpftool/bash-completion/bpftool b/tools/bpf/bpftool/bash-completion/bpftool
index 7b68e3c0a5fb..3f1da30c4da6 100644
--- a/tools/bpf/bpftool/bash-completion/bpftool
+++ b/tools/bpf/bpftool/bash-completion/bpftool
@@ -709,9 +709,26 @@ _bpftool()
"$cur" ) )
return 0
;;
- key|value|flags|name|entries)
+ key|value|flags|entries)
return 0
;;
+ inner_map)
+ COMPREPLY=( $( compgen -W "$MAP_TYPE" -- "$cur" ) )
+ return 0
+ ;;
+ id)
+ _bpftool_get_map_ids
+ ;;
+ name)
+ case $pprev in
+ inner_map)
+ _bpftool_get_map_names
+ ;;
+ *)
+ return 0
+ ;;
+ esac
+ ;;
*)
_bpftool_once_attr 'type'
_bpftool_once_attr 'key'
@@ -719,6 +736,9 @@ _bpftool()
_bpftool_once_attr 'entries'
_bpftool_once_attr 'name'
_bpftool_once_attr 'flags'
+ if _bpftool_search_list 'array_of_maps' 'hash_of_maps'; then
+ _bpftool_once_attr 'inner_map'
+ fi
_bpftool_once_attr 'dev'
return 0
;;
diff --git a/tools/bpf/bpftool/map.c b/tools/bpf/bpftool/map.c
index cb3a75eb5531..3cfdf71a8e13 100644
--- a/tools/bpf/bpftool/map.c
+++ b/tools/bpf/bpftool/map.c
@@ -1243,7 +1243,7 @@ static int do_create(int argc, char **argv)
{
struct bpf_create_map_attr attr = { NULL, };
const char *pinfile;
- int err, fd;
+ int err = -1, fd;
if (!REQ_ARGS(7))
return -1;
@@ -1258,13 +1258,13 @@ static int do_create(int argc, char **argv)
if (attr.map_type) {
p_err("map type already specified");
- return -1;
+ goto exit;
}
attr.map_type = map_type_from_str(*argv);
if ((int)attr.map_type < 0) {
p_err("unrecognized map type: %s", *argv);
- return -1;
+ goto exit;
}
NEXT_ARG();
} else if (is_prefix(*argv, "name")) {
@@ -1273,43 +1273,56 @@ static int do_create(int argc, char **argv)
} else if (is_prefix(*argv, "key")) {
if (parse_u32_arg(&argc, &argv, &attr.key_size,
"key size"))
- return -1;
+ goto exit;
} else if (is_prefix(*argv, "value")) {
if (parse_u32_arg(&argc, &argv, &attr.value_size,
"value size"))
- return -1;
+ goto exit;
} else if (is_prefix(*argv, "entries")) {
if (parse_u32_arg(&argc, &argv, &attr.max_entries,
"max entries"))
- return -1;
+ goto exit;
} else if (is_prefix(*argv, "flags")) {
if (parse_u32_arg(&argc, &argv, &attr.map_flags,
"flags"))
- return -1;
+ goto exit;
} else if (is_prefix(*argv, "dev")) {
NEXT_ARG();
if (attr.map_ifindex) {
p_err("offload device already specified");
- return -1;
+ goto exit;
}
attr.map_ifindex = if_nametoindex(*argv);
if (!attr.map_ifindex) {
p_err("unrecognized netdevice '%s': %s",
*argv, strerror(errno));
- return -1;
+ goto exit;
}
NEXT_ARG();
+ } else if (is_prefix(*argv, "inner_map")) {
+ struct bpf_map_info info = {};
+ __u32 len = sizeof(info);
+ int inner_map_fd;
+
+ NEXT_ARG();
+ if (!REQ_ARGS(2))
+ usage();
+ inner_map_fd = map_parse_fd_and_info(&argc, &argv,
+ &info, &len);
+ if (inner_map_fd < 0)
+ return -1;
+ attr.inner_map_fd = inner_map_fd;
} else {
p_err("unknown arg %s", *argv);
- return -1;
+ goto exit;
}
}
if (!attr.name) {
p_err("map name not specified");
- return -1;
+ goto exit;
}
set_max_rlimit();
@@ -1317,17 +1330,22 @@ static int do_create(int argc, char **argv)
fd = bpf_create_map_xattr(&attr);
if (fd < 0) {
p_err("map create failed: %s", strerror(errno));
- return -1;
+ goto exit;
}
err = do_pin_fd(fd, pinfile);
close(fd);
if (err)
- return err;
+ goto exit;
if (json_output)
jsonw_null(json_wtr);
- return 0;
+
+exit:
+ if (attr.inner_map_fd > 0)
+ close(attr.inner_map_fd);
+
+ return err;
}
static int do_pop_dequeue(int argc, char **argv)
@@ -1413,7 +1431,7 @@ static int do_help(int argc, char **argv)
"Usage: %1$s %2$s { show | list } [MAP]\n"
" %1$s %2$s create FILE type TYPE key KEY_SIZE value VALUE_SIZE \\\n"
" entries MAX_ENTRIES name NAME [flags FLAGS] \\\n"
- " [dev NAME]\n"
+ " [inner_map MAP] [dev NAME]\n"
" %1$s %2$s dump MAP\n"
" %1$s %2$s update MAP [key DATA] [value VALUE] [UPDATE_FLAGS]\n"
" %1$s %2$s lookup MAP [key DATA]\n"
--
2.25.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH bpf-next 2/2] tools: bpftool: add "inner_map" to "bpftool map create" outer maps
2020-09-04 16:13 ` [PATCH bpf-next 2/2] tools: bpftool: add "inner_map" to "bpftool map create" outer maps Quentin Monnet
@ 2020-09-04 21:55 ` Andrii Nakryiko
0 siblings, 0 replies; 6+ messages in thread
From: Andrii Nakryiko @ 2020-09-04 21:55 UTC (permalink / raw)
To: Quentin Monnet
Cc: Alexei Starovoitov, Daniel Borkmann, bpf, Networking, Martynas Pumputis
On Fri, Sep 4, 2020 at 9:16 AM Quentin Monnet <quentin@isovalent.com> wrote:
>
> There is no support for creating maps of types array-of-map or
> hash-of-map in bpftool. This is because the kernel needs an inner_map_fd
> to collect metadata on the inner maps to be supported by the new map,
> but bpftool does not provide a way to pass this file descriptor.
>
> Add a new optional "inner_map" keyword that can be used to pass a
> reference to a map, retrieve a fd to that map, and pass it as the
> inner_map_fd.
>
> Add related documentation and bash completion. Note that we can
> reference the inner map by its name, meaning we can have several times
> the keyword "name" with different meanings (mandatory outer map name,
> and possibly a name to use to find the inner_map_fd). The bash
> completion will offer it just once, and will not suggest "name" on the
> following command:
>
> # bpftool map create /sys/fs/bpf/my_outer_map type hash_of_maps \
> inner_map name my_inner_map [TAB]
>
> Fixing that specific case seems too convoluted. Completion will work as
> expected, however, if the outer map name comes first and the "inner_map
> name ..." is passed second.
>
> Signed-off-by: Quentin Monnet <quentin@isovalent.com>
> ---
Acked-by: Andrii Nakryiko <andriin@fb.com>
> .../bpf/bpftool/Documentation/bpftool-map.rst | 10 +++-
> tools/bpf/bpftool/bash-completion/bpftool | 22 ++++++++-
> tools/bpf/bpftool/map.c | 48 +++++++++++++------
> 3 files changed, 62 insertions(+), 18 deletions(-)
>
[...]
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH bpf-next 1/2] tools: bpftool: dump outer maps content
2020-09-04 16:13 ` [PATCH bpf-next 1/2] tools: bpftool: dump outer maps content Quentin Monnet
@ 2020-09-04 22:03 ` Andrii Nakryiko
2020-09-07 14:49 ` Quentin Monnet
0 siblings, 1 reply; 6+ messages in thread
From: Andrii Nakryiko @ 2020-09-04 22:03 UTC (permalink / raw)
To: Quentin Monnet
Cc: Alexei Starovoitov, Daniel Borkmann, bpf, Networking, Martynas Pumputis
On Fri, Sep 4, 2020 at 9:14 AM Quentin Monnet <quentin@isovalent.com> wrote:
>
> Although user space can lookup and dump the content of an outer map
> (hash-of-maps or array-of-maps), bpftool does not allow to do so.
>
> It seems that the only reason for that is historical. Lookups for outer
> maps was added in commit 14dc6f04f49d ("bpf: Add syscall lookup support
> for fd array and htab"), and although the relevant code in bpftool had
> not been merged yet, I suspect it had already been written with the
> assumption that user space could not read outer maps.
>
> Let's remove the restriction, dump for outer maps works with no further
> change.
>
> Reported-by: Martynas Pumputis <m@lambda.lt>
> Signed-off-by: Quentin Monnet <quentin@isovalent.com>
> ---
> tools/bpf/bpftool/map.c | 4 ----
> 1 file changed, 4 deletions(-)
>
> diff --git a/tools/bpf/bpftool/map.c b/tools/bpf/bpftool/map.c
> index bc0071228f88..cb3a75eb5531 100644
> --- a/tools/bpf/bpftool/map.c
> +++ b/tools/bpf/bpftool/map.c
> @@ -739,10 +739,6 @@ static int dump_map_elem(int fd, void *key, void *value,
> /* lookup error handling */
> lookup_errno = errno;
>
> - if (map_is_map_of_maps(map_info->type) ||
> - map_is_map_of_progs(map_info->type))
> - return 0;
> -
this code path handles the error case when lookup fails, or am I
misreading it? It's fine to remove this restriction, but the commit
message is completely misleading. That whole dump_map_elem() code is a
bit confusing. E.g., what's the purpose of num_elems there?..
Also, can you please update the commit message with how the output
looks like for map-of-maps with your change?
> if (json_output) {
> jsonw_start_object(json_wtr);
> jsonw_name(json_wtr, "key");
> --
> 2.25.1
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH bpf-next 1/2] tools: bpftool: dump outer maps content
2020-09-04 22:03 ` Andrii Nakryiko
@ 2020-09-07 14:49 ` Quentin Monnet
0 siblings, 0 replies; 6+ messages in thread
From: Quentin Monnet @ 2020-09-07 14:49 UTC (permalink / raw)
To: Andrii Nakryiko
Cc: Alexei Starovoitov, Daniel Borkmann, bpf, Networking, Martynas Pumputis
On 04/09/2020 23:03, Andrii Nakryiko wrote:
> On Fri, Sep 4, 2020 at 9:14 AM Quentin Monnet <quentin@isovalent.com> wrote:
>>
>> Although user space can lookup and dump the content of an outer map
>> (hash-of-maps or array-of-maps), bpftool does not allow to do so.
>>
>> It seems that the only reason for that is historical. Lookups for outer
>> maps was added in commit 14dc6f04f49d ("bpf: Add syscall lookup support
>> for fd array and htab"), and although the relevant code in bpftool had
>> not been merged yet, I suspect it had already been written with the
>> assumption that user space could not read outer maps.
>>
>> Let's remove the restriction, dump for outer maps works with no further
>> change.
>>
>> Reported-by: Martynas Pumputis <m@lambda.lt>
>> Signed-off-by: Quentin Monnet <quentin@isovalent.com>
>> ---
>> tools/bpf/bpftool/map.c | 4 ----
>> 1 file changed, 4 deletions(-)
>>
>> diff --git a/tools/bpf/bpftool/map.c b/tools/bpf/bpftool/map.c
>> index bc0071228f88..cb3a75eb5531 100644
>> --- a/tools/bpf/bpftool/map.c
>> +++ b/tools/bpf/bpftool/map.c
>> @@ -739,10 +739,6 @@ static int dump_map_elem(int fd, void *key, void *value,
>> /* lookup error handling */
>> lookup_errno = errno;
>>
>> - if (map_is_map_of_maps(map_info->type) ||
>> - map_is_map_of_progs(map_info->type))
>> - return 0;
>> -
>
> this code path handles the error case when lookup fails, or am I
> misreading it? It's fine to remove this restriction, but the commit
> message is completely misleading. That whole dump_map_elem() code is a
> bit confusing. E.g., what's the purpose of num_elems there?..
>
> Also, can you please update the commit message with how the output
> looks like for map-of-maps with your change?
Ok, the function _is_ confusing and _I_ totally got confused. Dumping
outer maps is already supported and this patch is not needed (and as you
mentioned, it does not do what I expected and wrote). Sorry about that.
I'll send a clean-up for that function instead in v2, to avoid confusion
in the future. Thanks for the review!
Quentin
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-09-07 15:59 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-09-04 16:13 [PATCH bpf-next 0/2] tools: bpftool: support creating and dumping outer maps Quentin Monnet
2020-09-04 16:13 ` [PATCH bpf-next 1/2] tools: bpftool: dump outer maps content Quentin Monnet
2020-09-04 22:03 ` Andrii Nakryiko
2020-09-07 14:49 ` Quentin Monnet
2020-09-04 16:13 ` [PATCH bpf-next 2/2] tools: bpftool: add "inner_map" to "bpftool map create" outer maps Quentin Monnet
2020-09-04 21:55 ` Andrii Nakryiko
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).