Re: [RFC PATCH bpf-next 04/16] bpf: allow loading of a dumper program

Yonghong Song
2020-04-10T23:28:49Z

On 4/10/20 3:36 PM, Andrii Nakryiko wrote:
> On Wed, Apr 8, 2020 at 4:25 PM Yonghong Song <yhs@fb.com> wrote:
>>
>> A dumper bpf program is a tracing program with attach type
>> BPF_TRACE_DUMP. During bpf program load, the load attribute
>> attach_prog_fd
>> carries the target directory fd. The program will be
>> verified against btf_id of the target_proto.
>>
>> If the program is loaded successfully, the dump target, as
>> represented as a relative path to /sys/kernel/bpfdump,
>> will be remembered in prog->aux->dump_target, which will
>> be used later to create dumpers.
>>
>> Signed-off-by: Yonghong Song <yhs@fb.com>
>> ---
>> include/linux/bpf.h | 2 ++
>> include/uapi/linux/bpf.h | 1 +
>> kernel/bpf/dump.c | 40 ++++++++++++++++++++++++++++++++++
>> kernel/bpf/syscall.c | 8 ++++++-
>> kernel/bpf/verifier.c | 15 +++++++++++++
>> tools/include/uapi/linux/bpf.h | 1 +
>> 6 files changed, 66 insertions(+), 1 deletion(-)
>>
>
> [...]
>
>>
>> +int bpf_dump_set_target_info(u32 target_fd, struct bpf_prog *prog)
>> +{
>> + struct bpfdump_target_info *tinfo;
>> + const char *target_proto;
>> + struct file *target_file;
>> + struct fd tfd;
>> + int err = 0, btf_id;
>> +
>> + if (!btf_vmlinux)
>> + return -EINVAL;
>> +
>> + tfd = fdget(target_fd);
>> + target_file = tfd.file;
>> + if (!target_file)
>> + return -EBADF;
>
> fdput is missing (or rather err = -BADF; goto done; ?)

No need to do fdput if tfd.file is NULL.

>
>
>> +
>> + if (target_file->f_inode->i_op != &bpf_dir_iops) {
>> + err = -EINVAL;
>> + goto done;
>> + }
>> +
>> + tinfo = target_file->f_inode->i_private;
>> + target_proto = tinfo->target_proto;
>> + btf_id = btf_find_by_name_kind(btf_vmlinux, target_proto,
>> + BTF_KIND_FUNC);
>> +
>> + if (btf_id > 0) {
>> + prog->aux->dump_target = tinfo->target;
>> + prog->aux->attach_btf_id = btf_id;
>> + }
>> +
>> + err = min(btf_id, 0);
>
> this min trick looks too clever... why not more straightforward and composable:
>
> if (btf_id < 0) {
> err = btf_id;
> goto done;
> }
>
> prog->aux->dump_target = tinfo->target;
> prog->aux->attach_btf_id = btf_id;
>
> ?

this can be done.

>
>> +done:
>> + fdput(tfd);
>> + return err;
>> +}
>> +
>> int bpf_dump_reg_target(const char *target,
>> const char *target_proto,
>> const struct seq_operations *seq_ops,
>> diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
>> index 64783da34202..41005dee8957 100644
>> --- a/kernel/bpf/syscall.c
>> +++ b/kernel/bpf/syscall.c
>> @@ -2060,7 +2060,12 @@ static int bpf_prog_load(union bpf_attr *attr, union bpf_attr __user *uattr)
>>
>> prog->expected_attach_type = attr->expected_attach_type;
>> prog->aux->attach_btf_id = attr->attach_btf_id;
>> - if (attr->attach_prog_fd) {
>> + if (type == BPF_PROG_TYPE_TRACING &&
>> + attr->expected_attach_type == BPF_TRACE_DUMP) {
>> + err = bpf_dump_set_target_info(attr->attach_prog_fd, prog);
>
> looking at bpf_attr, it's not clear why attach_prog_fd and
> prog_ifindex were not combined into a single union field... this
> probably got missed? But in this case I'd say let's create a
>
> union {
> __u32 attach_prog_fd;
> __u32 attach_target_fd; (similar to terminology for BPF_PROG_ATTACH)
> };
>
> instead of reusing not-exactly-matching field names?

I thought about this, but thinking to avoid uapi change (although
compatible). Maybe we should. Let me think about this.

>
>> + if (err)
>> + goto free_prog_nouncharge;
>> + } else if (attr->attach_prog_fd) {
>> struct bpf_prog *tgt_prog;
>>
>> tgt_prog = bpf_prog_get(attr->attach_prog_fd);
>> @@ -2145,6 +2150,7 @@ static int bpf_prog_load(union bpf_attr *attr, union bpf_attr __user *uattr)
>> err = bpf_prog_new_fd(prog);
>> if (err < 0)
>> bpf_prog_put(prog);
>> +
>> return err;
>>
>
> [...]
>