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

[Andrii Nakryiko <andrii.nakryiko@gmail.com>]

On Fri, Apr 10, 2020 at 4:28 PM Yonghong Song <yhs@fb.com> wrote:
>
>
>
> 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.

ah, right :)

>
> >
> >
> >> +
> >> +       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.

This is creating a new alias for the same field, so should be fine
from UAPI perspective.

>
> >
> >> +               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;
> >>
> >
> > [...]
> >