| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAEf4BzY1gor=j9kh2JxZAQc4SoyaRoVGA_7UK9z_Nb0FpCudkQ@mail.gmail.com>
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