Date:   Wed, 29 Apr 2020 13:40:24 -0700
From:   Andrii Nakryiko <andrii.nakryiko@...il.com>
To:     Yonghong Song <yhs@...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: [PATCH bpf-next v1 08/19] bpf: create file bpf iterator

On Mon, Apr 27, 2020 at 1:18 PM Yonghong Song <yhs@...com> wrote:
>
> A new obj type BPF_TYPE_ITER is added to bpffs.
> To produce a file bpf iterator, the fd must be
> corresponding to a link_fd assocciated with a
> trace/iter program. When the pinned file is
> opened, a seq_file will be generated.
>
> Signed-off-by: Yonghong Song <yhs@...com>
> ---
>  include/linux/bpf.h   |  3 +++
>  kernel/bpf/bpf_iter.c | 48 ++++++++++++++++++++++++++++++++++++++++++-
>  kernel/bpf/inode.c    | 28 +++++++++++++++++++++++++
>  kernel/bpf/syscall.c  |  2 +-
>  4 files changed, 79 insertions(+), 2 deletions(-)
>
> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> index 0f0cafc65a04..601b3299b7e4 100644
> --- a/include/linux/bpf.h
> +++ b/include/linux/bpf.h
> @@ -1021,6 +1021,8 @@ static inline void bpf_enable_instrumentation(void)
>
>  extern const struct file_operations bpf_map_fops;
>  extern const struct file_operations bpf_prog_fops;
> +extern const struct file_operations bpf_link_fops;
> +extern const struct file_operations bpffs_iter_fops;
>
>  #define BPF_PROG_TYPE(_id, _name, prog_ctx_type, kern_ctx_type) \
>         extern const struct bpf_prog_ops _name ## _prog_ops; \
> @@ -1136,6 +1138,7 @@ int bpf_iter_link_attach(const union bpf_attr *attr, struct bpf_prog *prog);
>  int bpf_iter_link_replace(struct bpf_link *link, struct bpf_prog *old_prog,
>                           struct bpf_prog *new_prog);
>  int bpf_iter_new_fd(struct bpf_link *link);
> +void *bpf_iter_get_from_fd(u32 ufd);
>
>  int bpf_percpu_hash_copy(struct bpf_map *map, void *key, void *value);
>  int bpf_percpu_array_copy(struct bpf_map *map, void *key, void *value);
> diff --git a/kernel/bpf/bpf_iter.c b/kernel/bpf/bpf_iter.c
> index 1f4e778d1814..f5e933236996 100644
> --- a/kernel/bpf/bpf_iter.c
> +++ b/kernel/bpf/bpf_iter.c
> @@ -123,7 +123,8 @@ struct bpf_prog *bpf_iter_get_prog(struct seq_file *seq, u32 priv_data_size,
>  {
>         struct extra_priv_data *extra_data;
>
> -       if (seq->file->f_op != &anon_bpf_iter_fops)
> +       if (seq->file->f_op != &anon_bpf_iter_fops &&
> +           seq->file->f_op != &bpffs_iter_fops)

Do we really need anon_bpf_iter_fops and bpffs_iter_fops? Seems like
the only difference is bpffs_iter_open. Could it be implemented as
part of anon_bpf_iter_ops as well? Seems like open() is never called
for anon_inode_file, so it should work for both?

>                 return NULL;
>
>         extra_data = get_extra_priv_dptr(seq->private, priv_data_size);
> @@ -310,3 +311,48 @@ int bpf_iter_new_fd(struct bpf_link *link)
>         put_unused_fd(fd);
>         return err;
>  }
> +
> +static int bpffs_iter_open(struct inode *inode, struct file *file)
> +{
> +       struct bpf_iter_link *link = inode->i_private;
> +
> +       return prepare_seq_file(file, link);
> +}
> +
> +static int bpffs_iter_release(struct inode *inode, struct file *file)
> +{
> +       return anon_iter_release(inode, file);
> +}
> +
> +const struct file_operations bpffs_iter_fops = {
> +       .open           = bpffs_iter_open,
> +       .read           = seq_read,
> +       .release        = bpffs_iter_release,
> +};
> +
> +void *bpf_iter_get_from_fd(u32 ufd)

return struct bpf_iter_link * here, given this is specific constructor
for bpf_iter_link?

> +{
> +       struct bpf_link *link;
> +       struct bpf_prog *prog;
> +       struct fd f;
> +
> +       f = fdget(ufd);
> +       if (!f.file)
> +               return ERR_PTR(-EBADF);
> +       if (f.file->f_op != &bpf_link_fops) {
> +               link = ERR_PTR(-EINVAL);
> +               goto out;
> +       }
> +
> +       link = f.file->private_data;
> +       prog = link->prog;
> +       if (prog->expected_attach_type != BPF_TRACE_ITER) {
> +               link = ERR_PTR(-EINVAL);
> +               goto out;
> +       }
> +
> +       bpf_link_inc(link);
> +out:
> +       fdput(f);
> +       return link;
> +}
> diff --git a/kernel/bpf/inode.c b/kernel/bpf/inode.c
> index 95087d9f4ed3..de4493983a37 100644
> --- a/kernel/bpf/inode.c
> +++ b/kernel/bpf/inode.c
> @@ -26,6 +26,7 @@ enum bpf_type {
>         BPF_TYPE_PROG,
>         BPF_TYPE_MAP,
>         BPF_TYPE_LINK,
> +       BPF_TYPE_ITER,

Adding ITER as an alternative type of pinned object to BPF_TYPE_LINK
seems undesirable. We can allow opening bpf_iter's seq_file by doing
the same trick as is done for bpf_maps, supporting seq_show (see
bpf_mkmap() and bpf_map_support_seq_show()). Do you think we can do
the same here? If we later see that more kinds of links would want to
allow direct open() to create a file with some output from BPF
program, we can generalize this as part of bpf_link infrastructure.
For now having a custom check similar to bpf_map's seems sufficient.

What do you think?

>  };
>
>  static void *bpf_any_get(void *raw, enum bpf_type type)
> @@ -38,6 +39,7 @@ static void *bpf_any_get(void *raw, enum bpf_type type)
>                 bpf_map_inc_with_uref(raw);
>                 break;
>         case BPF_TYPE_LINK:
> +       case BPF_TYPE_ITER:
>                 bpf_link_inc(raw);
>                 break;
>         default:
> @@ -58,6 +60,7 @@ static void bpf_any_put(void *raw, enum bpf_type type)
>                 bpf_map_put_with_uref(raw);
>                 break;
>         case BPF_TYPE_LINK:
> +       case BPF_TYPE_ITER:
>                 bpf_link_put(raw);
>                 break;
>         default:
> @@ -82,6 +85,15 @@ static void *bpf_fd_probe_obj(u32 ufd, enum bpf_type *type)
>                 return raw;
>         }
>

[...]

Powered by blists - more mailing lists