| 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: <82034392-5f65-fd84-8cbd-d2aa85f01ee3@fb.com>
Date: Tue, 28 Apr 2020 10:35:34 -0700
From: Yonghong Song <yhs@...com>
To: Quentin Monnet <quentin@...valent.com>,
Andrii Nakryiko <andriin@...com>, <bpf@...r.kernel.org>,
Martin KaFai Lau <kafai@...com>, <netdev@...r.kernel.org>
CC: Alexei Starovoitov <ast@...com>,
Daniel Borkmann <daniel@...earbox.net>, <kernel-team@...com>
Subject: Re: [PATCH bpf-next v1 16/19] tools/bpftool: add bpf_iter support for
bptool
On 4/28/20 2:27 AM, Quentin Monnet wrote:
> 2020-04-27 13:12 UTC-0700 ~ Yonghong Song <yhs@...com>
>> Currently, only one command is supported
>> bpftool iter pin <bpf_prog.o> <path>
>>
>> It will pin the trace/iter bpf program in
>> the object file <bpf_prog.o> to the <path>
>> where <path> should be on a bpffs mount.
>>
>> For example,
>> $ bpftool iter pin ./bpf_iter_ipv6_route.o \
>> /sys/fs/bpf/my_route
>> User can then do a `cat` to print out the results:
>> $ cat /sys/fs/bpf/my_route
>> fe800000000000000000000000000000 40 00000000000000000000000000000000 ...
>> 00000000000000000000000000000000 00 00000000000000000000000000000000 ...
>> 00000000000000000000000000000001 80 00000000000000000000000000000000 ...
>> fe800000000000008c0162fffebdfd57 80 00000000000000000000000000000000 ...
>> ff000000000000000000000000000000 08 00000000000000000000000000000000 ...
>> 00000000000000000000000000000000 00 00000000000000000000000000000000 ...
>>
>> The implementation for ipv6_route iterator is in one of subsequent
>> patches.
>>
>> Signed-off-by: Yonghong Song <yhs@...com>
>> ---
>> .../bpftool/Documentation/bpftool-iter.rst | 71 ++++++++++++++++
>> tools/bpf/bpftool/bash-completion/bpftool | 13 +++
>> tools/bpf/bpftool/iter.c | 84 +++++++++++++++++++
>> tools/bpf/bpftool/main.c | 3 +-
>> tools/bpf/bpftool/main.h | 1 +
>> 5 files changed, 171 insertions(+), 1 deletion(-)
>> create mode 100644 tools/bpf/bpftool/Documentation/bpftool-iter.rst
>> create mode 100644 tools/bpf/bpftool/iter.c
>>
>> diff --git a/tools/bpf/bpftool/Documentation/bpftool-iter.rst b/tools/bpf/bpftool/Documentation/bpftool-iter.rst
>> new file mode 100644
>> index 000000000000..1997a6bac4a0
>> --- /dev/null
>> +++ b/tools/bpf/bpftool/Documentation/bpftool-iter.rst
>> @@ -0,0 +1,71 @@
>> +============
>> +bpftool-iter
>> +============
>> +-------------------------------------------------------------------------------
>> +tool to create BPF iterators
>> +-------------------------------------------------------------------------------
>> +
>> +:Manual section: 8
>> +
>> +SYNOPSIS
>> +========
>> +
>> + **bpftool** [*OPTIONS*] **iter** *COMMAND*
>> +
>> + *COMMANDS* := { **pin** | **help** }
>> +
>> +STRUCT_OPS COMMANDS
>
> s/STRUCT_OPS/ITER/
Oops. copy-paste error. Will fix.
>
>> +===================
>> +
>> +| **bpftool** **iter pin** *OBJ* *PATH*
>> +| **bpftool** **struct_ops help**
>
> s/struct_ops/iter/
Will fix.
>
>> +|
>> +| *OBJ* := /a/file/of/bpf_iter_target.o
>> +
>> +
>> +DESCRIPTION
>> +===========
>> + **bpftool iter pin** *OBJ* *PATH*
>
> Would be great to have a small blurb on what BPF iterators are and what
> they can do. I'm afraid users reading this man page will have no idea
> whatsoever.
Will add.
>
>> + Create a bpf iterator from *OBJ*, and pin it to
>> + *PATH*. The *PATH* should be located in *bpffs* mount.
>
> Can you keep the note that other pages have about the dot character
> being forbidden in *PATH* basename, please?
Will add.
>
>> +
>> + **bpftool struct_ops help**
>
> s/struct_ops/iter/
Will fix.
>
>> + Print short help message.
>> +
>> +OPTIONS
>> +=======
>> + -h, --help
>> + Print short generic help message (similar to **bpftool help**).
>> +
>> + -V, --version
>> + Print version number (similar to **bpftool version**).
>> +
>> + -d, --debug
>> + Print all logs available, even debug-level information. This
>> + includes logs from libbpf as well as from the verifier, when
>> + attempting to load programs.> +
>> +EXAMPLES
>> +========
>> +**# bpftool iter pin bpf_iter_netlink.o /sys/fs/bpf/my_netlink**
>> +
>> +::
>> +
>> + Create a file-based bpf iterator from bpf_iter_netlink.o and pin it
>> + to /sys/fs/bpf/my_netlink
>> +
>> +
>> +SEE ALSO
>> +========
>> + **bpf**\ (2),
>> + **bpf-helpers**\ (7),
>> + **bpftool**\ (8),
>> + **bpftool-prog**\ (8),
>> + **bpftool-map**\ (8),
>> + **bpftool-cgroup**\ (8),
>> + **bpftool-feature**\ (8),
>> + **bpftool-net**\ (8),
>> + **bpftool-perf**\ (8),
>> + **bpftool-btf**\ (8)
>> + **bpftool-gen**\ (8)
>> + **bpftool-struct_ops**\ (8)
>> diff --git a/tools/bpf/bpftool/bash-completion/bpftool b/tools/bpf/bpftool/bash-completion/bpftool
>> index 45ee99b159e2..17a81695da0f 100644
>> --- a/tools/bpf/bpftool/bash-completion/bpftool
>> +++ b/tools/bpf/bpftool/bash-completion/bpftool
>> @@ -604,6 +604,19 @@ _bpftool()
>> ;;
>> esac
>> ;;
>> + iter)
>> + case $command in
>> + pin)
>> + _filedir
>> + return 0
>> + ;;
>> + *)
>> + [[ $prev == $object ]] && \
>> + COMPREPLY=( $( compgen -W 'help' \
>> + -- "$cur" ) )
>
> You should probably offer "pin" here in addition to "help".
Will add.
>
>> + ;;
>> + esac
>> + ;;
>> map)
>> local MAP_TYPE='id pinned name'
>> case $command in
>> diff --git a/tools/bpf/bpftool/iter.c b/tools/bpf/bpftool/iter.c
>> new file mode 100644
>> index 000000000000..db9fae6be716
>> --- /dev/null
>> +++ b/tools/bpf/bpftool/iter.c
>> @@ -0,0 +1,84 @@
>> +// SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>> +// Copyright (C) 2020 Facebook
>> +
>> +#define _GNU_SOURCE
>> +#include <linux/err.h>
>> +#include <bpf/libbpf.h>
>> +
>> +#include "main.h"
>> +
>> +static int do_pin(int argc, char **argv)
>> +{
>> + const char *objfile, *path;
>> + struct bpf_program *prog;
>> + struct bpf_object *obj;
>> + struct bpf_link *link;
>> + int err;
>
> Nit: initialise err t0 -1 do you don't have to set it three times below?
Double checked cmd_select() handling the return value:
0 : success
non-0 : failure
Looking like I can remove two `err = -1` below.
>
>> +
>> + if (!REQ_ARGS(2))
>> + usage();
>> +
>> + objfile = GET_ARG();
>> + path = GET_ARG();
>> +
>> + obj = bpf_object__open(objfile);
>> + if (IS_ERR_OR_NULL(obj)) {
>> + p_err("can't open objfile %s", objfile);
>> + return -1;
>> + }
>> +
>> + err = bpf_object__load(obj);
>> + if (err < 0) {
>> + err = -1;
>> + p_err("can't load objfile %s", objfile);
>> + goto close_obj;
>> + }
>> +
>> + prog = bpf_program__next(NULL, obj);
>> + link = bpf_program__attach_iter(prog, NULL);
>> + if (IS_ERR(link)) {
>> + err = -1;
>> + p_err("attach_iter failed for program %s",
>> + bpf_program__name(prog));
>> + goto close_obj;
>> + }
>> +
>> + err = bpf_link__pin(link, path);
>
> Try to mount bpffs before that if "-n" is not passed? You could even
> call do_pin_any() from common.c by passing bpf_link__fd().
You probably means do_pin_fd()? That is a good suggestion, will use it
in the next revision.
>
>> + if (err) {
>> + err = -1;
>> + p_err("pin_iter failed for program %s to path %s",
>> + bpf_program__name(prog), path);
>> + goto close_link;
>> + }
>> +
>> + err = 0;
>> +
>> +close_link:
>> + bpf_link__disconnect(link);
>> + bpf_link__destroy(link);
>> +close_obj:
>> + bpf_object__close(obj);
>> + return err;
>> +}
>> +
>> +static int do_help(int argc, char **argv)
>> +{
>> + fprintf(stderr,
>> + "Usage: %s %s pin OBJ PATH\n"
>> + " %s %s help\n"
>> + "\n",
>> + bin_name, argv[-2], bin_name, argv[-2]);
>> +
>> + return 0;
>> +}
>> +
>> +static const struct cmd cmds[] = {
>> + { "help", do_help },
>> + { "pin", do_pin },
>> + { 0 }
>> +};
>> +
>> +int do_iter(int argc, char **argv)
>> +{
>> + return cmd_select(cmds, argc, argv, do_help);
>> +}
>> dif "",
>> bin_name, bin_name, bin_name);
>> @@ -222,6 +222,7 @@ static const struct cmd cmds[] = {
>> { "btf", do_btf },
>> { "gen", do_gen },
>> { "struct_ops", do_struct_ops },
>> + { "iter", do_iter },
>> { "version", do_version },
>> { 0 }
>> };
>> diff --git a/tools/bpf/bpftool/main.h b/tools/bpf/bpftool/main.h
>> index 86f14ce26fd7..2b5d4a616b48 100644
>> --- a/tools/bpf/bpftool/main.h
>> +++ b/tools/bpf/bpftool/main.h
>> @@ -162,6 +162,7 @@ int do_feature(int argc, char **argv);
>> int do_btf(int argc, char **argv);
>> int do_gen(int argc, char **argv);
>> int do_struct_ops(int argc, char **argv);
>> +int do_iter(int argc, char **argv);
>>
>> int parse_u32_arg(int *argc, char ***argv, __u32 *val, const char *what);
>> int prog_parse_fd(int *argc, char ***argv);
>> f --git a/tools/bpf/bpftool/main.c b/tools/bpf/bpftool/main.c
>> index 466c269eabdd..6805b77789cb 100644
>> --- a/tools/bpf/bpftool/main.c
>> +++ b/tools/bpf/bpftool/main.c
>> @@ -58,7 +58,7 @@ static int do_help(int argc, char **argv)
>> " %s batch file FILE\n"
>> " %s version\n"
>> "\n"
>> - " OBJECT := { prog | map | cgroup | perf | net | feature | btf | gen | struct_ops }\n"
>> + " OBJECT := { prog | map | cgroup | perf | net | feature | btf | gen | struct_ops | iter }\n"
>> " " HELP_SPEC_OPTIONS "\n"
>> "",
>> bin_name, bin_name, bin_name);
>> @@ -222,6 +222,7 @@ static const struct cmd cmds[] = {
>> { "btf", do_btf },
>> { "gen", do_gen },
>> { "struct_ops", do_struct_ops },
>> + { "iter", do_iter },
>> { "version", do_version },
>> { 0 }
>> };
>> diff --git a/tools/bpf/bpftool/main.h b/tools/bpf/bpftool/main.h
>> index 86f14ce26fd7..2b5d4a616b48 100644
>> --- a/tools/bpf/bpftool/main.h
>> +++ b/tools/bpf/bpftool/main.h
>> @@ -162,6 +162,7 @@ int do_feature(int argc, char **argv);
>> int do_btf(int argc, char **argv);
>> int do_gen(int argc, char **argv);
>> int do_struct_ops(int argc, char **argv);
>> +int do_iter(int argc, char **argv);
>>
>> int parse_u32_arg(int *argc, char ***argv, __u32 *val, const char *what);
>> int prog_parse_fd(int *argc, char ***argv);
>>
>
> Have you considered simply adapting the more traditional workflow
> "bpftool prog load && bpftool prog attach" so that it supports iterators
> instead of adding a new command? It would:
This is a good question, I should have clarified better in the commit
message.
- prog load && prog attach won't work.
the create_iter is a three stage process:
1. prog load
2. create and attach to a link
3. pin link
In the current implementation, the link merely just has the program.
But in the future, the link will have other parameters like map_id,
tgid/gid, or cgroup_id, or others.
We could say to do:
1. bpftool prog load <pin_path>
2. bpftool iter pin prog file
<maybe more parameters in the future>
But this requires to pin the program itself in the bpffs, which
mostly unneeded for file iterator creator.
So this command `bpftool iter pin ...` is created for ease of use.
>
> - Avoid adding yet another bpftool command with a single subcommand
So far, yes, in the future we may have more. In my RFC patcch, I have
`bpftool iter show ...` for introspection, this is to show all
registered targets and all file iterators prog_id's.
This patch does not have it and I left it for the future work.
I am considering to use bpf iterator to do introspection here...
>
> - Enable to reuse the code from prog load, in particular for map reuse
> (I'm not sure how relevant maps are for iterators, but I wouldn't be
> surprised if someone finds a use case at some point?)
Yes, we do plan to have map element iterators. We can also have
bpf_prog or other iterators. Yes, map element iterator use
implementation should be `bpftool map` code base since it is
a use of bpf_iter infrastructure.
>
> - Avoid users naively trying to run "bpftool prog load && bpftool prog
> attach <prog> iter" and not understanding why it fails
`bpftool prog attach <prog> [map_id]` mostly used to attach a program to
a map, right? In this case, it won't apply, right?
BTW, Thanks for reviewing and catching my mistakes!
>
> Quentin
>
Powered by blists - more mailing lists