| 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
| ||
|
Date: Fri, 24 Apr 2020 09:30:24 -0700
From: Andrii Nakryiko <andrii.nakryiko@...il.com>
To: Quentin Monnet <quentin@...valent.com>
Cc: Andrii Nakryiko <andriin@...com>, bpf <bpf@...r.kernel.org>,
Networking <netdev@...r.kernel.org>,
Alexei Starovoitov <ast@...com>,
Daniel Borkmann <daniel@...earbox.net>,
Kernel Team <kernel-team@...com>
Subject: Re: [PATCH bpf-next 08/10] bpftool: add bpf_link show and pin support
On Fri, Apr 24, 2020 at 3:32 AM Quentin Monnet <quentin@...valent.com> wrote:
>
> 2020-04-23 22:35 UTC-0700 ~ Andrii Nakryiko <andriin@...com>
> > Add `bpftool link show` and `bpftool link pin` commands.
> >
> > Example plain output for `link show` (with showing pinned paths):
> >
> > [vmuser@...hvm bpf]$ sudo ~/local/linux/tools/bpf/bpftool/bpftool -f link
> > 1: tracing prog 12
> > prog_type tracing attach_type fentry
> > pinned /sys/fs/bpf/my_test_link
> > pinned /sys/fs/bpf/my_test_link2
> > 2: tracing prog 13
> > prog_type tracing attach_type fentry
> > 3: tracing prog 14
> > prog_type tracing attach_type fentry
> > 4: tracing prog 15
> > prog_type tracing attach_type fentry
> > 5: tracing prog 16
> > prog_type tracing attach_type fentry
> > 6: tracing prog 17
> > prog_type tracing attach_type fentry
> > 7: raw_tracepoint prog 21
> > tp 'sys_enter'
> > 8: cgroup prog 25
> > cgroup_id 584 attach_type egress
> > 9: cgroup prog 25
> > cgroup_id 599 attach_type egress
> > 10: cgroup prog 25
> > cgroup_id 614 attach_type egress
> > 11: cgroup prog 25
> > cgroup_id 629 attach_type egress
> >
> > Signed-off-by: Andrii Nakryiko <andriin@...com>
> > ---
> > tools/bpf/bpftool/common.c | 2 +
> > tools/bpf/bpftool/link.c | 402 +++++++++++++++++++++++++++++++++++++
> > tools/bpf/bpftool/main.c | 6 +-
> > tools/bpf/bpftool/main.h | 5 +
> > 4 files changed, 414 insertions(+), 1 deletion(-)
> > create mode 100644 tools/bpf/bpftool/link.c
> >
> > diff --git a/tools/bpf/bpftool/common.c b/tools/bpf/bpftool/common.c
> > index f2223dbdfb0a..c47bdc65de8e 100644
> > --- a/tools/bpf/bpftool/common.c
> > +++ b/tools/bpf/bpftool/common.c
> > @@ -262,6 +262,8 @@ int get_fd_type(int fd)
> > return BPF_OBJ_MAP;
> > else if (strstr(buf, "bpf-prog"))
> > return BPF_OBJ_PROG;
> > + else if (strstr(buf, "bpf-link"))
> > + return BPF_OBJ_LINK;
> >
> > return BPF_OBJ_UNKNOWN;
> > }
> > diff --git a/tools/bpf/bpftool/link.c b/tools/bpf/bpftool/link.c
> > new file mode 100644
> > index 000000000000..d5dcf9e46536
> > --- /dev/null
> > +++ b/tools/bpf/bpftool/link.c
>
> [...]
>
> > +
> > +static int link_parse_fd(int *argc, char ***argv)
> > +{
> > + int *fds = NULL;
> > + int nb_fds, fd;
> > +
> > + fds = malloc(sizeof(int));
> > + if (!fds) {
> > + p_err("mem alloc failed");
> > + return -1;
> > + }
> > + nb_fds = link_parse_fds(argc, argv, &fds);
> > + if (nb_fds != 1) {
> > + if (nb_fds > 1) {
> > + p_err("several links match this handle");
>
> Can this ever happen? "bpftool prog show" has this because "name" or
> "tag" can match multiple programs. But "id" and "pinned" for link should
> not, as far as I understand.
No, it shouldn't. I'll keep only link_parse_fd and will always return
singe fd, thanks.
>
> > + while (nb_fds--)
> > + close(fds[nb_fds]);
> > + }
> > + fd = -1;
> > + goto exit_free;
> > + }
> > +
> > + fd = fds[0];
> > +exit_free:
> > + free(fds);
> > + return fd;
> > +}
> > +
> > +static void
> > +show_link_header_json(struct bpf_link_info *info, json_writer_t *wtr)
> > +{
> > + jsonw_uint_field(wtr, "id", info->id);
> > + if (info->type < ARRAY_SIZE(link_type_name))
> > + jsonw_string_field(wtr, "type", link_type_name[info->type]);
> > + else
> > + jsonw_uint_field(wtr, "type", info->type);
> > +
> > + jsonw_uint_field(json_wtr, "prog_id", info->prog_id);
> > +}
> > +
> > +static int get_prog_info(int prog_id, struct bpf_prog_info *info)
> > +{
> > + __u32 len = sizeof(*info);
> > + int err, prog_fd;
> > +
> > + prog_fd = bpf_prog_get_fd_by_id(prog_id);
> > + if (prog_fd < 0)
> > + return prog_fd;
> > +
> > + memset(info, 0, sizeof(*info));
> > + err = bpf_obj_get_info_by_fd(prog_fd, info, &len);
> > + if (err) {
> > + p_err("can't get prog info: %s", strerror(errno));
> > + close(prog_fd);
> > + return err;
>
> Nit: you could "return err;" at the end of the function, and remove the
> "close()" and "return" from this if block.
yep
>
> > + }
> > +
> > + close(prog_fd);
> > + return 0;
> > +}
> > +
>
> [...]
>
> > +
> > +static int do_show_subset(int argc, char **argv)
> > +{
> > + int *fds = NULL;
> > + int nb_fds, i;
> > + int err = -1;
> > +
> > + fds = malloc(sizeof(int));
> > + if (!fds) {
> > + p_err("mem alloc failed");
> > + return -1;
> > + }
> > + nb_fds = link_parse_fds(&argc, &argv, &fds);
> > + if (nb_fds < 1)
> > + goto exit_free;
> > +
> > + if (json_output && nb_fds > 1)
> > + jsonw_start_array(json_wtr); /* root array */
> > + for (i = 0; i < nb_fds; i++) {
> > + err = do_show_link(fds[i]);
> > + if (err) {
> > + for (; i + 1 < nb_fds; i++)
> > + close(fds[i]);
> > + break;
> > + }
> > + }
> > + if (json_output && nb_fds > 1)
> > + jsonw_end_array(json_wtr); /* root array */
> > +
> > +exit_free:
> > + free(fds);
> > + return err;
> > +}
> > +
> > +static int do_show(int argc, char **argv)
> > +{
> > + __u32 id = 0;
> > + int err;
> > + int fd;
> > +
> > + if (show_pinned)
> > + build_pinned_obj_table(&link_table, BPF_OBJ_LINK);
> > +
> > + if (argc == 2)
> > + return do_show_subset(argc, argv);
>
> I understand the "_subset" aspect was taken from prog.c. But it was
> added there (ec2025095cf6) because "bpftool prog show <name|tag>" can
> match several programs, and the array with fds would be reallocated as
> required while parsing names/tags.
>
> I do not think that by restricting the selection on "id" or "pinned",
> you can get more than one link at a time. So we can probably avoid
> juggling with fd arrays for link_parse_fd() / link_parse_fds() and show
> just the single relevant link.
yep, I did quite mechanical conversion of progs code to links, I'll do
another pass with this in mind. Thanks for review!
Powered by blists - more mailing lists