| 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: <05d9c82d-8cba-db77-02af-265e4d200946@isovalent.com>
Date: Tue, 28 Apr 2020 10:27:23 +0100
From: Quentin Monnet <quentin@...valent.com>
To: Yonghong Song <yhs@...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
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/
> +===================
> +
> +| **bpftool** **iter pin** *OBJ* *PATH*
> +| **bpftool** **struct_ops help**
s/struct_ops/iter/
> +|
> +| *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.
> + 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?
> +
> + **bpftool struct_ops help**
s/struct_ops/iter/
> + 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".
> + ;;
> + 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?
> +
> + 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().
> + 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:
- Avoid adding yet another bpftool command with a single subcommand
- 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?)
- Avoid users naively trying to run "bpftool prog load && bpftool prog
attach <prog> iter" and not understanding why it fails
Quentin
Powered by blists - more mailing lists