Re: [RFC PATCH bpf-next 05/16] bpf: create file or anonymous dumpers

Yonghong Song
2020-04-10T23:42:06Z

On 4/10/20 3:51 PM, Andrii Nakryiko wrote:
> On Wed, Apr 8, 2020 at 4:26 PM Yonghong Song <yhs@fb.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@fb.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
>> +
>
> [...]
>