Re: [PATCH bpf-next v3 18/21] tools/bpftool: add bpf_iter support for bptool

From: Yonghong Song <yhs@fb.com>
To: Andrii Nakryiko <andrii.nakryiko@gmail.com>
Cc: Andrii Nakryiko <andriin@fb.com>, bpf <bpf@vger.kernel.org>,
	Martin KaFai Lau <kafai@fb.com>,
	Networking <netdev@vger.kernel.org>,
	Alexei Starovoitov <ast@fb.com>,
	Daniel Borkmann <daniel@iogearbox.net>,
	Kernel Team <kernel-team@fb.com>
Subject: Re: [PATCH bpf-next v3 18/21] tools/bpftool: add bpf_iter support for bptool
Date: Fri, 8 May 2020 22:26:50 -0700	[thread overview]
Message-ID: <1ea0a61f-5400-0bb7-c063-0e2adf18a6bc@fb.com> (raw)
In-Reply-To: <CAEf4Bzb1VJj5gWvL0Jiip8P9KhSfT6seCRH8N7Q49Fw3_jNOGQ@mail.gmail.com>

On 5/8/20 12:51 PM, Andrii Nakryiko wrote:
> On Wed, May 6, 2020 at 10:40 PM Yonghong Song <yhs@fb.com> wrote:
>>
>> 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.
>>
>> This patch also added BPF_LINK_TYPE_ITER to link query.
>>
>> In the future, we may add additional parameters to pin command
>> by parameterizing the bpf iterator. For example, a map_id or pid
>> may be added to let bpf program only traverses a single map or task,
>> similar to kernel seq_file single_open().
>>
>> We may also add introspection command for targets/iterators by
>> leveraging the bpf_iter itself.
>>
>> Signed-off-by: Yonghong Song <yhs@fb.com>
>> ---
>>   .../bpftool/Documentation/bpftool-iter.rst    | 83 ++++++++++++++++++
>>   tools/bpf/bpftool/bash-completion/bpftool     | 13 +++
>>   tools/bpf/bpftool/iter.c                      | 84 +++++++++++++++++++
>>   tools/bpf/bpftool/link.c                      |  1 +
>>   tools/bpf/bpftool/main.c                      |  3 +-
>>   tools/bpf/bpftool/main.h                      |  1 +
>>   6 files changed, 184 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/iter.c b/tools/bpf/bpftool/iter.c
>> new file mode 100644
>> index 000000000000..a8fb1349c103
>> --- /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;
>> +
>> +       if (!REQ_ARGS(2))
>> +               usage();
>> +
>> +       objfile = GET_ARG();
>> +       path = GET_ARG();
>> +
>> +       obj = bpf_object__open(objfile);
>> +       if (IS_ERR_OR_NULL(obj)) {
> 
> nit: can't be NULL

Ack. Will change.

> 
>> +               p_err("can't open objfile %s", objfile);
>> +               return -1;
>> +       }
>> +
>> +       err = bpf_object__load(obj);
>> +       if (err) {
>> +               p_err("can't load objfile %s", objfile);
>> +               goto close_obj;
>> +       }
>> +
>> +       prog = bpf_program__next(NULL, obj);
> 
> check for null and printf error? Crashing is not good.

Make sense. Will change.

> 
>> +       link = bpf_program__attach_iter(prog, NULL);
>> +       if (IS_ERR(link)) {
>> +               err = PTR_ERR(link);
>> +               p_err("attach_iter failed for program %s",
>> +                     bpf_program__name(prog));
>> +               goto close_obj;
>> +       }
>> +
>> +       err = mount_bpffs_for_pin(path);
>> +       if (err)
>> +               goto close_link;
>> +
>> +       err = bpf_link__pin(link, path);
>> +       if (err) {
>> +               p_err("pin_iter failed for program %s to path %s",
>> +                     bpf_program__name(prog), path);
>> +               goto close_link;
>> +       }
>> +
>> +close_link:
>> +       bpf_link__disconnect(link);
> 
> this is wrong, just destroy()

Will change.

> 
>> +       bpf_link__destroy(link);
>> +close_obj:
>> +       bpf_object__close(obj);
>> +       return err;
>> +}
>> +
> 
> [...]
> 