| 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: <419c2909-349a-2495-e2dd-1cd647e21d4a@fb.com>
Date: Thu, 30 Apr 2020 11:02:54 -0700
From: Yonghong Song <yhs@...com>
To: Andrii Nakryiko <andrii.nakryiko@...il.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 4/29/20 1:40 PM, Andrii Nakryiko wrote:
> 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?
Yes, open() will not be used for anon_bpf_iter. I used two
file_operations just for this reason. But I guess, I can
just use one. It won't hurt.
>
>> 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?
Sounds good. Will use the mechanism similar to bpf_map.
>
>> };
>>
>> 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