Message-ID: <ed14f5c8-dacc-369f-07d0-f5ee2877e8ea@fb.com>
Date:   Fri, 10 Apr 2020 16:41:46 -0700
From:   Yonghong Song <yhs@...com>
To:     Andrii Nakryiko <andrii.nakryiko@...il.com>
CC:     Andrii Nakryiko <andriin@...com>, bpf <bpf@...r.kernel.org>,
        Martin KaFai Lau <kafai@...com>,
        Networking <netdev@...r.kernel.org>,
        Alexei Starovoitov <ast@...com>,
        Daniel Borkmann <daniel@...earbox.net>,
        Kernel Team <kernel-team@...com>
Subject: Re: [RFC PATCH bpf-next 05/16] bpf: create file or anonymous dumpers



On 4/10/20 3:51 PM, Andrii Nakryiko wrote:
> On Wed, Apr 8, 2020 at 4:26 PM Yonghong Song <yhs@...com> wrote:
>>
>> Given a loaded dumper bpf program, which already
>> knows which target it should bind to, there
>> two ways to create a dumper:
>>    - a file based dumper under hierarchy of
>>      /sys/kernel/bpfdump/ which uses can
>>      "cat" to print out the output.
>>    - an anonymous dumper which user application
>>      can "read" the dumping output.
>>
>> For file based dumper, BPF_OBJ_PIN syscall interface
>> is used. For anonymous dumper, BPF_PROG_ATTACH
>> syscall interface is used.
>>
>> To facilitate target seq_ops->show() to get the
>> bpf program easily, dumper creation increased
>> the target-provided seq_file private data size
>> so bpf program pointer is also stored in seq_file
>> private data.
>>
>> Further, a seq_num which represents how many
>> bpf_dump_get_prog() has been called is also
>> available to the target seq_ops->show().
>> Such information can be used to e.g., print
>> banner before printing out actual data.
>>
>> Note the seq_num does not represent the num
>> of unique kernel objects the bpf program has
>> seen. But it should be a good approximate.
>>
>> A target feature BPF_DUMP_SEQ_NET_PRIVATE
>> is implemented specifically useful for
>> net based dumpers. It sets net namespace
>> as the current process net namespace.
>> This avoids changing existing net seq_ops
>> in order to retrieve net namespace from
>> the seq_file pointer.
>>
>> For open dumper files, anonymous or not, the
>> fdinfo will show the target and prog_id associated
>> with that file descriptor. For dumper file itself,
>> a kernel interface will be provided to retrieve the
>> prog_id in one of the later patches.
>>
>> Signed-off-by: Yonghong Song <yhs@...com>
>> ---
>>   include/linux/bpf.h            |   5 +
>>   include/uapi/linux/bpf.h       |   6 +-
>>   kernel/bpf/dump.c              | 338 ++++++++++++++++++++++++++++++++-
>>   kernel/bpf/syscall.c           |  11 +-
>>   tools/include/uapi/linux/bpf.h |   6 +-
>>   5 files changed, 362 insertions(+), 4 deletions(-)
>>
> 
> [...]
> 
>>
>> +struct dumper_inode_info {
>> +       struct bpfdump_target_info *tinfo;
>> +       struct bpf_prog *prog;
>> +};
>> +
>> +struct dumper_info {
>> +       struct list_head list;
>> +       /* file to identify an anon dumper,
>> +        * dentry to identify a file dumper.
>> +        */
>> +       union {
>> +               struct file *file;
>> +               struct dentry *dentry;
>> +       };
>> +       struct bpfdump_target_info *tinfo;
>> +       struct bpf_prog *prog;
>> +};
> 
> This is essentially a bpf_link. Why not do it as a bpf_link from the
> get go? Instead of having all this duplication for anonymous and

This is a good question. Maybe part of bpf-link can be used and
I have to implement others. I will check.

> pinned dumpers, it would always be a bpf_link-based dumper, but for
> those pinned bpf_link itself is going to be pinned. You also get a
> benefit of being able to list all dumpers through existing bpf_link
> API (also see my RFC patches with bpf_link_prime/bpf_link_settle,
> which makes using bpf_link safe and simple).

Agree. Alternative is to use BPF_OBJ_GET_INFO_BY_FD to query individual
dumper as directory tree walk can be easily done at user space.


> 
> [...]
> 
>> +
>> +static void anon_dumper_show_fdinfo(struct seq_file *m, struct file *filp)
>> +{
>> +       struct dumper_info *dinfo;
>> +
>> +       mutex_lock(&anon_dumpers.dumper_mutex);
>> +       list_for_each_entry(dinfo, &anon_dumpers.dumpers, list) {
> 
> this (and few other places where you search in a loop) would also be
> simplified, because struct file* would point to bpf_dumper_link, which
> then would have a pointer to bpf_prog, dentry (if pinned), etc. No
> searching at all.

This is a reason for this. the same as bpflink, bpfdump already has
the full information about file, inode, etc.
The file private_data actually points to seq_file. The seq_file private 
data is used in the target. That is exactly why we try to have this 
mapping to keep track. bpf_link won't help here.

> 
>> +               if (dinfo->file == filp) {
>> +                       seq_printf(m, "target:\t%s\n"
>> +                                     "prog_id:\t%u\n",
>> +                                  dinfo->tinfo->target,
>> +                                  dinfo->prog->aux->id);
>> +                       break;
>> +               }
>> +       }
>> +       mutex_unlock(&anon_dumpers.dumper_mutex);
>> +}
>> +
>> +#endif
>> +
> 
> [...]
> 

Powered by blists - more mailing lists