On Wed, Apr 8, 2020 at 4:25 PM Yonghong Song <yhs@xxxxxx> 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@xxxxxx>
> ---
> 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; ?)
> +
> + 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;
?
> +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?
> + 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;
>
[...]
