| 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: <CABdmKX3ZjeZmT=Fj_TYfpXouM6AGigcQPH7ygf3puFQip0DQ_g@mail.gmail.com>
Date: Wed, 7 May 2025 10:36:44 -0700
From: "T.J. Mercier" <tjmercier@...gle.com>
To: Christian König <christian.koenig@....com>
Cc: sumit.semwal@...aro.org, ast@...nel.org, daniel@...earbox.net,
andrii@...nel.org, martin.lau@...ux.dev, skhan@...uxfoundation.org,
alexei.starovoitov@...il.com, song@...nel.org, linux-kernel@...r.kernel.org,
linux-media@...r.kernel.org, dri-devel@...ts.freedesktop.org,
linaro-mm-sig@...ts.linaro.org, bpf@...r.kernel.org,
linux-kselftest@...r.kernel.org, android-mm@...gle.com, simona@...ll.ch,
eddyz87@...il.com, yonghong.song@...ux.dev, john.fastabend@...il.com,
kpsingh@...nel.org, sdf@...ichev.me, jolsa@...nel.org, mykolal@...com
Subject: Re: [PATCH bpf-next v3 2/5] bpf: Add dmabuf iterator
On Wed, May 7, 2025 at 1:15 AM Christian König <christian.koenig@....com> wrote:
>
> On 5/7/25 02:10, T.J. Mercier wrote:
> > The dmabuf iterator traverses the list of all DMA buffers.
> >
> > DMA buffers are refcounted through their associated struct file. A
> > reference is taken on each buffer as the list is iterated to ensure each
> > buffer persists for the duration of the bpf program execution without
> > holding the list mutex.
> >
> > Signed-off-by: T.J. Mercier <tjmercier@...gle.com>
> > ---
> > drivers/dma-buf/dma-buf.c | 64 ++++++++++++++++++++++++
> > include/linux/dma-buf.h | 3 ++
> > kernel/bpf/Makefile | 3 ++
> > kernel/bpf/dmabuf_iter.c | 102 ++++++++++++++++++++++++++++++++++++++
> > 4 files changed, 172 insertions(+)
> > create mode 100644 kernel/bpf/dmabuf_iter.c
> >
> > diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c
> > index 8d151784e302..9fee2788924e 100644
> > --- a/drivers/dma-buf/dma-buf.c
> > +++ b/drivers/dma-buf/dma-buf.c
> > @@ -19,7 +19,9 @@
> > #include <linux/anon_inodes.h>
> > #include <linux/export.h>
> > #include <linux/debugfs.h>
> > +#include <linux/list.h>
> > #include <linux/module.h>
> > +#include <linux/mutex.h>
> > #include <linux/seq_file.h>
> > #include <linux/sync_file.h>
> > #include <linux/poll.h>
> > @@ -55,6 +57,68 @@ static void __dma_buf_list_del(struct dma_buf *dmabuf)
> > mutex_unlock(&dmabuf_list_mutex);
> > }
> >
> > +/**
> > + * get_first_dmabuf - begin iteration through global list of DMA-bufs
>
> As far as I can see that looks really good.
>
> The only thing I'm questioning a little bit is that the name get_first_dmabuf() just doesn't sound so well to me.
>
> I'm a fan of keeping the object you work with first in the naming and it should somehow express that this iters over the global list of all buffers. Maybe something like dmabuf_get_first_globally or dmabuf_get_first_instance.
>
> Feel free to add my rb if any of those suggestions are used, but I'm completely open other ideas as well.
>
> Regards,
> Christian.
>
Yeah you're right. "first" is actually a little misleading too, since
the most recently exported buffer will be at the list head, not the
oldest buffer. But buffer age or ordering doesn't really matter here
as long as we get through all of them.
So I'm thinking dma_buf_iter_begin() and dma_buf_iter_next() would be
better names. Similar to seq_start / seq_next or bpf's iter_<type>_new
/ iter_<type>_next.
> > + *
> > + * Returns the first buffer in the global list of DMA-bufs that's not in the
> > + * process of being destroyed. Increments that buffer's reference count to
> > + * prevent buffer destruction. Callers must release the reference, either by
> > + * continuing iteration with get_next_dmabuf(), or with dma_buf_put().
> > + *
> > + * Returns NULL If no active buffers are present.
> > + */
> > +struct dma_buf *get_first_dmabuf(void)
> > +{
> > + struct dma_buf *ret = NULL, *dmabuf;
> > +
> > + /*
> > + * The list mutex does not protect a dmabuf's refcount, so it can be
> > + * zeroed while we are iterating. We cannot call get_dma_buf() since the
> > + * caller may not already own a reference to the buffer.
> > + */
> > + mutex_lock(&dmabuf_list_mutex);
> > + list_for_each_entry(dmabuf, &dmabuf_list, list_node) {
> > + if (file_ref_get(&dmabuf->file->f_ref)) {
> > + ret = dmabuf;
> > + break;
> > + }
> > + }
> > + mutex_unlock(&dmabuf_list_mutex);
> > + return ret;
> > +}
> > +
> > +/**
> > + * get_next_dmabuf - continue iteration through global list of DMA-bufs
> > + * @dmabuf: [in] pointer to dma_buf
> > + *
> > + * Decrements the reference count on the provided buffer. Returns the next
> > + * buffer from the remainder of the global list of DMA-bufs with its reference
> > + * count incremented. Callers must release the reference, either by continuing
> > + * iteration with get_next_dmabuf(), or with dma_buf_put().
> > + *
> > + * Returns NULL If no additional active buffers are present.
> > + */
> > +struct dma_buf *get_next_dmabuf(struct dma_buf *dmabuf)
> > +{
> > + struct dma_buf *ret = NULL;
> > +
> > + /*
> > + * The list mutex does not protect a dmabuf's refcount, so it can be
> > + * zeroed while we are iterating. We cannot call get_dma_buf() since the
> > + * caller may not already own a reference to the buffer.
> > + */
> > + mutex_lock(&dmabuf_list_mutex);
> > + dma_buf_put(dmabuf);
> > + list_for_each_entry_continue(dmabuf, &dmabuf_list, list_node) {
> > + if (file_ref_get(&dmabuf->file->f_ref)) {
> > + ret = dmabuf;
> > + break;
> > + }
> > + }
> > + mutex_unlock(&dmabuf_list_mutex);
> > + return ret;
> > +}
> > +
> > static char *dmabuffs_dname(struct dentry *dentry, char *buffer, int buflen)
> > {
> > struct dma_buf *dmabuf;
> > diff --git a/include/linux/dma-buf.h b/include/linux/dma-buf.h
> > index 8ff4add71f88..1820f6db6e58 100644
> > --- a/include/linux/dma-buf.h
> > +++ b/include/linux/dma-buf.h
> > @@ -568,6 +568,9 @@ static inline void get_dma_buf(struct dma_buf *dmabuf)
> > get_file(dmabuf->file);
> > }
> >
> > +struct dma_buf *get_first_dmabuf(void);
> > +struct dma_buf *get_next_dmabuf(struct dma_buf *dmbuf);
> > +
> > /**
> > * dma_buf_is_dynamic - check if a DMA-buf uses dynamic mappings.
> > * @dmabuf: the DMA-buf to check
> > diff --git a/kernel/bpf/Makefile b/kernel/bpf/Makefile
> > index 70502f038b92..3a335c50e6e3 100644
> > --- a/kernel/bpf/Makefile
> > +++ b/kernel/bpf/Makefile
> > @@ -53,6 +53,9 @@ obj-$(CONFIG_BPF_SYSCALL) += relo_core.o
> > obj-$(CONFIG_BPF_SYSCALL) += btf_iter.o
> > obj-$(CONFIG_BPF_SYSCALL) += btf_relocate.o
> > obj-$(CONFIG_BPF_SYSCALL) += kmem_cache_iter.o
> > +ifeq ($(CONFIG_DMA_SHARED_BUFFER),y)
> > +obj-$(CONFIG_BPF_SYSCALL) += dmabuf_iter.o
> > +endif
> >
> > CFLAGS_REMOVE_percpu_freelist.o = $(CC_FLAGS_FTRACE)
> > CFLAGS_REMOVE_bpf_lru_list.o = $(CC_FLAGS_FTRACE)
> > diff --git a/kernel/bpf/dmabuf_iter.c b/kernel/bpf/dmabuf_iter.c
> > new file mode 100644
> > index 000000000000..80bca8239c6d
> > --- /dev/null
> > +++ b/kernel/bpf/dmabuf_iter.c
> > @@ -0,0 +1,102 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/* Copyright (c) 2025 Google LLC */
> > +#include <linux/bpf.h>
> > +#include <linux/btf_ids.h>
> > +#include <linux/dma-buf.h>
> > +#include <linux/kernel.h>
> > +#include <linux/seq_file.h>
> > +
> > +BTF_ID_LIST_SINGLE(bpf_dmabuf_btf_id, struct, dma_buf)
> > +DEFINE_BPF_ITER_FUNC(dmabuf, struct bpf_iter_meta *meta, struct dma_buf *dmabuf)
> > +
> > +static void *dmabuf_iter_seq_start(struct seq_file *seq, loff_t *pos)
> > +{
> > + if (*pos)
> > + return NULL;
> > +
> > + return get_first_dmabuf();
> > +}
> > +
> > +static void *dmabuf_iter_seq_next(struct seq_file *seq, void *v, loff_t *pos)
> > +{
> > + struct dma_buf *dmabuf = v;
> > +
> > + ++*pos;
> > +
> > + return get_next_dmabuf(dmabuf);
> > +}
> > +
> > +struct bpf_iter__dmabuf {
> > + __bpf_md_ptr(struct bpf_iter_meta *, meta);
> > + __bpf_md_ptr(struct dma_buf *, dmabuf);
> > +};
> > +
> > +static int __dmabuf_seq_show(struct seq_file *seq, void *v, bool in_stop)
> > +{
> > + struct bpf_iter_meta meta = {
> > + .seq = seq,
> > + };
> > + struct bpf_iter__dmabuf ctx = {
> > + .meta = &meta,
> > + .dmabuf = v,
> > + };
> > + struct bpf_prog *prog = bpf_iter_get_info(&meta, in_stop);
> > +
> > + if (prog)
> > + return bpf_iter_run_prog(prog, &ctx);
> > +
> > + return 0;
> > +}
> > +
> > +static int dmabuf_iter_seq_show(struct seq_file *seq, void *v)
> > +{
> > + return __dmabuf_seq_show(seq, v, false);
> > +}
> > +
> > +static void dmabuf_iter_seq_stop(struct seq_file *seq, void *v)
> > +{
> > + struct dma_buf *dmabuf = v;
> > +
> > + if (dmabuf)
> > + dma_buf_put(dmabuf);
> > +}
> > +
> > +static const struct seq_operations dmabuf_iter_seq_ops = {
> > + .start = dmabuf_iter_seq_start,
> > + .next = dmabuf_iter_seq_next,
> > + .stop = dmabuf_iter_seq_stop,
> > + .show = dmabuf_iter_seq_show,
> > +};
> > +
> > +static void bpf_iter_dmabuf_show_fdinfo(const struct bpf_iter_aux_info *aux,
> > + struct seq_file *seq)
> > +{
> > + seq_puts(seq, "dmabuf iter\n");
> > +}
> > +
> > +static const struct bpf_iter_seq_info dmabuf_iter_seq_info = {
> > + .seq_ops = &dmabuf_iter_seq_ops,
> > + .init_seq_private = NULL,
> > + .fini_seq_private = NULL,
> > + .seq_priv_size = 0,
> > +};
> > +
> > +static struct bpf_iter_reg bpf_dmabuf_reg_info = {
> > + .target = "dmabuf",
> > + .feature = BPF_ITER_RESCHED,
> > + .show_fdinfo = bpf_iter_dmabuf_show_fdinfo,
> > + .ctx_arg_info_size = 1,
> > + .ctx_arg_info = {
> > + { offsetof(struct bpf_iter__dmabuf, dmabuf),
> > + PTR_TO_BTF_ID_OR_NULL },
> > + },
> > + .seq_info = &dmabuf_iter_seq_info,
> > +};
> > +
> > +static int __init dmabuf_iter_init(void)
> > +{
> > + bpf_dmabuf_reg_info.ctx_arg_info[0].btf_id = bpf_dmabuf_btf_id[0];
> > + return bpf_iter_reg_target(&bpf_dmabuf_reg_info);
> > +}
> > +
> > +late_initcall(dmabuf_iter_init);
>
Powered by blists - more mailing lists