Linaro-mm-sig

linaro-mm-sig@lists.linaro.org

21 participants
3582 discussions

Re: [PATCH v6 4/4] selftests: dmabuf-heaps: add fd-leak-on-EFAULT regression test

by T.J. Mercier

On Fri, Aug 7, 2026 at 3:12 AM Baineng Shou <shoubaineng(a)gmail.com> wrote: > > Add a test case that verifies no file descriptor is leaked when > DMA_HEAP_IOCTL_ALLOC succeeds internally but copy_to_user() fails > to deliver the fd number back to userspace. > > The failure is triggered by placing the ioctl argument in a private > anonymous page and flipping it to PROT_READ (via mprotect) between > the kernel's copy_from_user() and copy_to_user() calls. With the > buggy kernel the ioctl returns -EFAULT but leaves an extra open fd > in the process's fd table; with the fixed kernel the fd count is > unchanged. > > This serves as a regression test for: > "dma-buf: dma-heap: don't publish fd before copy_to_user() succeeds" > > Suggested-by: Sumit Semwal <sumit.semwal(a)linaro.org> > Signed-off-by: Baineng Shou <shoubaineng(a)gmail.com> Reviewed-by: T.J. Mercier <tjmercier(a)google.com>

1 month, 2 weeks

Re: [PATCH v9 2/5] rust: types: implement ForeignOwnable for ARef<T>

by Gary Guo

On Wed Aug 5, 2026 at 3:59 PM BST, Philipp Stanner wrote: > From: Danilo Krummrich <dakr(a)kernel.org> > > Implement ForeignOwnable for ARef<T>, making it possible for C code to > own an ARef<T>. > > Since ARef represents shared ownership, BorrowedMut is &T rather than > &mut T, matching the semantics of the underlying reference-counted type. > > Signed-off-by: Danilo Krummrich <dakr(a)kernel.org> > Reviewed-by: Alice Ryhl <aliceryhl(a)google.com> > Tested-by: Daniel Almeida <daniel.almeida(a)collabora.com> > --- > rust/kernel/sync/aref.rs | 40 ++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 40 insertions(+) > > diff --git a/rust/kernel/sync/aref.rs b/rust/kernel/sync/aref.rs > index b721b2e00b98..540766613659 100644 > --- a/rust/kernel/sync/aref.rs > +++ b/rust/kernel/sync/aref.rs > @@ -24,6 +24,11 @@ > ptr::NonNull, // > }; > > +use crate::{ > + prelude::*, > + types::ForeignOwnable, // > +}; > + > /// Types that are _always_ reference counted. > /// > /// It allows such types to define their own custom ref increment and decrement functions. > @@ -188,6 +193,41 @@ fn eq(&self, other: &ARef<U>) -> bool { > } > impl<T: AlwaysRefCounted + Eq> Eq for ARef<T> {} > > +// SAFETY: `into_foreign` returns a pointer from `NonNull::as_ptr`, so it's non-null. The > +// `ARef` invariant guarantees that `ptr` points to a valid `T`, so it's aligned to `T`. > +unsafe impl<T: AlwaysRefCounted + 'static> ForeignOwnable for ARef<T> { > + const FOREIGN_ALIGN: usize = core::mem::align_of::<T>(); > + > + type Borrowed<'a> = &'a T; > + type BorrowedMut<'a> = &'a T; > + `#[inline]` here and all others. Best, Gary > + fn into_foreign(self) -> *mut c_void { > + ARef::into_raw(self).as_ptr().cast() > + } > + > + unsafe fn from_foreign(ptr: *mut c_void) -> Self { > + // SAFETY: The safety requirements of this function ensure that `ptr` comes from a previous > + // call to `Self::into_foreign`. > + let ptr = unsafe { NonNull::new_unchecked(ptr.cast()) }; > + > + // SAFETY: `ptr` came from `into_foreign`, which consumed an `ARef` without decrementing > + // the refcount, so we can transfer the ownership to the new `ARef`. > + unsafe { ARef::from_raw(ptr) } > + } > + > + unsafe fn borrow<'a>(ptr: *mut c_void) -> &'a T { > + // SAFETY: The safety requirements of this method ensure that the object remains alive and > + // immutable for the duration of 'a. > + unsafe { &*ptr.cast() } > + } > + > + unsafe fn borrow_mut<'a>(ptr: *mut c_void) -> &'a T { > + // SAFETY: The safety requirements for `borrow_mut` are a superset of the safety > + // requirements for `borrow`. > + unsafe { <Self as ForeignOwnable>::borrow(ptr) } > + } > +} > + > impl<T, U> PartialEq<&'_ U> for ARef<T> > where > T: AlwaysRefCounted + PartialEq<U>,

1 month, 2 weeks

Re: [PATCH v4 01/14] dma-buf: introduce initial file I/O infrastructure

by Christian König

On 8/5/26 12:59, Pavel Begunkov wrote: > On 8/5/26 09:27, Christian König wrote: >>> + >>> + dma_resv_lock(dmabuf->resv, NULL); >>> + ctx->dev_ops->unmap(ctx, map); >>> + dma_resv_unlock(dmabuf->resv); >>> + >>> + dma_fence_put(&fence->base); >> >> You should probably set map->fence to NULL after that. > > The map is freed two lines below, but I can add it as > a defensive measure. In that case it's ok, I've just haven't seen the kfree(map) below. > ... >>> + ret = dma_resv_reserve_fences(dmabuf->resv, 1); >>> + if (WARN_ON_ONCE(ret)) { >>> + struct dma_fence *fence = &map->fence->base; >>> + >>> + dma_fence_get(fence); >>> + percpu_ref_kill(&map->refs); >>> + dma_fence_wait(fence, false); >>> + dma_fence_put(fence); >>> + return; >>> + } >>> + >>> + dma_resv_add_fence(dmabuf->resv, &map->fence->base, >>> + DMA_RESV_USAGE_KERNEL); >> >> That sequence is clearly incorrect! >> >> The fence must be created after dma_resv_reserve_fences(), otherwise you definately have an illegal memory operation here. > > I'm not sure what you mean, can you elaborate? I only cared about > pre-allocating it to avoid allocations here. We add / signal the fence > only once, no reuse. The map is going to be killed here, and if we > create a new map, it'll have its own fence. > > I can move the dma_fence_init() call here if that makes a difference? Yeah that is a good start, but you might need a bit more. Here is a summary of the usual procedure you need to follow when implementing a dma_fence backend: 1. Allocate your operation object, in this case here it's your mapping I think. 2. Prepare your operation, including all memory allocations. 3. Call dma_resv_reserve_fences() to reserve a fence slot. 4. Allocate and init your dma_fence object. After this step no memory allocation is allowed any more until your dma_fence object signals. The only exception is optional logging or crash dumping using GFP_NOWAIT (can fail trivially!) or minimal allocations using GFP_ATOMIC if you absolutely have to. 5. dma_resv_add_fence() to publish the fence. 6. dma_resv_unlock(). Having a dma_fence is certainly nice to have, but the tricky part is that memory allocations using GFP_KERNEL (or GFP_IO, GFP_FS etc...) can cycle back and wait for your dma_fence to signal which essentially can cause a deadlock very deeply inside memory management. Since those deadlocks happen only on memory contention situations they are usually just hard to reproduce but still totally break your neck if you manage to mess this up. So that needs to be super carefully implemented. Regards, Christian.

1 month, 2 weeks

Re: [PATCH v4 01/14] dma-buf: introduce initial file I/O infrastructure

by Christian König

On 7/28/26 23:29, Pavel Begunkov wrote: > The goal is to be able to natively use dma-buf in the read-write / IO > path. This patch adds basic building blocks serving as a glue and API > between drivers and upper layer subsystems providing the uAPI. Later > patches implement it for NVMe raw block devices and expose it to the > user space via io_uring. > > There are two main objects. struct dma_buf_io_ctx and struct > dma_buf_io_map. The ctx is used during initial registration and serves > as an interface between the upper layer user like io_uring and to the > importer subsystem / driver. The map represents the actual dma map > established for the target device[s] with dma_buf_map_attachment() and > stored in a device specific format. The context is created via a new > file operation ->init_dma_buf_io_ctx. > > The ctx-map separation exists to support map invalidation (see > dma_buf_io_invalidate_mappings()). A ctx can create > multiple maps during its lifetime, but there can only be no more than > one (active) map attached to it. Invalidation drops the active map > if present, and the next map will only be attempted to be created > once there is a new request that wants to use the dma-buf IO ctx. > > The primary task of the dma_buf_io_map object is to count requests > using it and to wait for their completion when we want to destroy the > DMA map. Invalidation is delayed by inserting a dma fence, which is > signaled when all flight requests are completed. > > [un]mapping and any work with dma addresses is delegated to the > importer driver via an ops table stored in the ctx, see struct > dma_buf_io_ops. Only the target driver / subsystem knows about devices > it wants to use the dma-buf with, especially in case of multi-device > filesystems or stacking in the future. > > Signed-off-by: Pavel Begunkov <asml.silence(a)gmail.com> > --- > drivers/dma-buf/Makefile | 2 +- > drivers/dma-buf/dma-buf-io.c | 275 +++++++++++++++++++++++++++++++++++ > include/linux/dma-buf-io.h | 91 ++++++++++++ > include/linux/fs.h | 2 + > 4 files changed, 369 insertions(+), 1 deletion(-) > create mode 100644 drivers/dma-buf/dma-buf-io.c > create mode 100644 include/linux/dma-buf-io.h > > diff --git a/drivers/dma-buf/Makefile b/drivers/dma-buf/Makefile > index b25d7550bacf..523731b0f83e 100644 > --- a/drivers/dma-buf/Makefile > +++ b/drivers/dma-buf/Makefile > @@ -1,6 +1,6 @@ > # SPDX-License-Identifier: GPL-2.0-only > obj-y := dma-buf.o dma-fence.o dma-fence-array.o dma-fence-chain.o \ > - dma-fence-unwrap.o dma-resv.o dma-buf-mapping.o > + dma-fence-unwrap.o dma-resv.o dma-buf-mapping.o dma-buf-io.o > obj-$(CONFIG_DMABUF_HEAPS) += dma-heap.o > obj-$(CONFIG_DMABUF_HEAPS) += heaps/ > obj-$(CONFIG_SYNC_FILE) += sync_file.o > diff --git a/drivers/dma-buf/dma-buf-io.c b/drivers/dma-buf/dma-buf-io.c > new file mode 100644 > index 000000000000..5f545a1bcf32 > --- /dev/null > +++ b/drivers/dma-buf/dma-buf-io.c > @@ -0,0 +1,275 @@ > +/* SPDX-License-Identifier: GPL-2.0 */ > +/* > + * Common infrastructure for supporing dma-buf in the I/O path. > + * > + * Copyright (C) 2026 Pavel Begunkov <asml.silence(a)gmail.com> > + */ > +#include <linux/dma-buf-io.h> > +#include <linux/dma-resv.h> > + > +struct dma_buf_io_fence { > + struct dma_fence base; > + spinlock_t lock; > +}; Upstream has change to allow embedding the spinlock into the dma_fence, so this structure here is most likely not necessary any more. > + > +static const char *dma_buf_io_fence_drv_name(struct dma_fence *fence) > +{ > + /* default fence release kfree's the base pointer */ > + BUILD_BUG_ON(offsetof(struct dma_buf_io_fence, base)); > + > + return "dma-buf-io-ctx"; > +} > + > +static const char *dma_buf_io_fence_timeline_name(struct dma_fence *fence) > +{ > + return "dma-buf-io-ctx"; > +} > + > +const struct dma_fence_ops dma_buf_io_fence_ops = { > + .get_driver_name = dma_buf_io_fence_drv_name, > + .get_timeline_name = dma_buf_io_fence_timeline_name, > +}; > + > +static void dma_buf_io_ctx_destroy_work(struct work_struct *work) > +{ > + struct dma_buf_io_ctx *ctx = container_of(work, struct dma_buf_io_ctx, > + release_work); > + > + if (WARN_ON_ONCE(refcount_read(&ctx->refs))) > + return; > + > + ctx->dev_ops->release(ctx); > + dma_buf_put(ctx->dmabuf); > + kfree(ctx); > +} > + > +static void dma_buf_io_map_release_work(struct work_struct *work) > +{ > + struct dma_buf_io_map *map = container_of(work, struct dma_buf_io_map, > + release_work); > + struct dma_buf_io_fence *fence = map->fence; > + struct dma_buf_io_ctx *ctx = map->ctx; > + struct dma_buf *dmabuf = ctx->dmabuf; > + > + /* the release path must wait for fences */ > + if (WARN_ON_ONCE(refcount_read(&ctx->refs) == 0)) > + return; Stuff like that is usually illegal. And why are you using refcount directly instead of kref? > + > + /* Prevent from destoying the ctx while unmapping */ > + refcount_inc(&ctx->refs); > + > + /* > + * There are no more requests using the map, we can signal the fence. > + * It should be done before taking the resv lock as someone could be > + * waiting for the fence while holding the lock. > + */ > + dma_fence_signal(&fence->base); Signaling fences has a whole bunch of very strict rules associated with it. E.g. you can't alocate memory for example. Are you sure you actually need and want a dma_fence here? > + > + dma_resv_lock(dmabuf->resv, NULL); > + ctx->dev_ops->unmap(ctx, map); > + dma_resv_unlock(dmabuf->resv); > + > + dma_fence_put(&fence->base); You should probably set map->fence to NULL after that. > + percpu_ref_exit(&map->refs); > + kfree(map); > + > + if (refcount_dec_and_test(&ctx->refs)) { > + /* > + * Destruction needs to wait for I/O and dma fences. Defer it to > + * simplify locking. > + */ > + INIT_WORK(&ctx->release_work, dma_buf_io_ctx_destroy_work); > + queue_work(system_wq, &ctx->release_work); > + } > +} > + > +static void dma_buf_io_map_refs_release(struct percpu_ref *ref) > +{ > + struct dma_buf_io_map *map = container_of(ref, struct dma_buf_io_map, refs); > + > + /* might sleep, use a worker */ > + INIT_WORK(&map->release_work, dma_buf_io_map_release_work); > + queue_work(system_wq, &map->release_work); > +} > + > +int dma_buf_io_init_map(struct dma_buf_io_ctx *ctx, struct dma_buf_io_map *map) > +{ > + struct dma_buf_io_fence *fence = NULL; > + int ret; > + > + fence = kzalloc(sizeof(*fence), GFP_KERNEL); > + if (!fence) > + return -ENOMEM; > + > + ret = percpu_ref_init(&map->refs, dma_buf_io_map_refs_release, 0, GFP_KERNEL); > + if (ret) { > + kfree(fence); > + return ret; > + } > + > + spin_lock_init(&fence->lock); > + dma_fence_init(&fence->base, &dma_buf_io_fence_ops, &fence->lock, > + ctx->fence_ctx, atomic_inc_return(&ctx->fence_seq)); > + map->fence = fence; > + map->ctx = ctx; > + return 0; > +} > +EXPORT_SYMBOL_NS_GPL(dma_buf_io_init_map, "DMA_BUF"); > + > +struct dma_buf_io_map *dma_buf_io_create_map(struct dma_buf_io_ctx *ctx) > +{ > + struct dma_buf *dmabuf = ctx->dmabuf; > + struct dma_buf_io_map *map; > + long ret; > + > +retry: > + /* > + * ->dmabuf_map() will be calling dma_buf_map_attachment(), for which > + * we'll need to wait for fences. Do a bit nicer and try to wait > + * without the resv lock first. > + */ Clear NAK to that. Always wait while holding the resv lock if you can! It is absolutely not beneficial to do this outside of the lock and usually just hides problems instead and prevent fixing them. > + ret = dma_resv_wait_timeout(dmabuf->resv, DMA_RESV_USAGE_KERNEL, > + true, MAX_SCHEDULE_TIMEOUT); > + if (!ret) > + ret = -EAGAIN; > + if (ret < 0) > + return ERR_PTR(ret); > + > + ret = dma_resv_lock_interruptible(dmabuf->resv, NULL); > + if (ret) > + return ERR_PTR(ret); > + > + map = dma_buf_io_get_map(ctx); > + if (map) { > + ret = 0; > + goto out; > + } > + > + if (dma_resv_wait_timeout(dmabuf->resv, DMA_RESV_USAGE_KERNEL, > + true, 0) < 0) { > + dma_resv_unlock(dmabuf->resv); > + goto retry; > + } > + > + map = ctx->dev_ops->map(ctx); > + if (IS_ERR(map)) { > + ret = PTR_ERR(map); > + goto out; > + } > + > + percpu_ref_get(&map->refs); > + rcu_assign_pointer(ctx->map, map); > +out: > + dma_resv_unlock(dmabuf->resv); > + if (ret < 0) > + return ERR_PTR(ret); > + return map; > +} > + > +static void dma_buf_io_drop_map(struct dma_buf_io_ctx *ctx) > +{ > + struct dma_buf *dmabuf = ctx->dmabuf; > + struct dma_buf_io_map *map; > + int ret; > + > + dma_resv_assert_held(dmabuf->resv); > + > + map = rcu_dereference_protected(ctx->map, > + dma_resv_held(dmabuf->resv)); > + if (!map) > + return; > + rcu_assign_pointer(ctx->map, NULL); > + > + ret = dma_resv_reserve_fences(dmabuf->resv, 1); > + if (WARN_ON_ONCE(ret)) { > + struct dma_fence *fence = &map->fence->base; > + > + dma_fence_get(fence); > + percpu_ref_kill(&map->refs); > + dma_fence_wait(fence, false); > + dma_fence_put(fence); > + return; > + } > + > + dma_resv_add_fence(dmabuf->resv, &map->fence->base, > + DMA_RESV_USAGE_KERNEL); That sequence is clearly incorrect! The fence must be created after dma_resv_reserve_fences(), otherwise you definately have an illegal memory operation here. Regards, Christian. > + /* > + * Delay destruction until all inflight requests using the map arefence-> > + * gone. It'll also signal the fence then. > + */ > + percpu_ref_kill(&map->refs); > +} > + > +void dma_buf_io_invalidate_mappings(struct dma_buf_io_ctx *ctx) > +{ > + dma_buf_io_drop_map(ctx); > +} > +EXPORT_SYMBOL_NS_GPL(dma_buf_io_invalidate_mappings, "DMA_BUF"); > + > +static void dma_buf_io_ctx_release_work(struct work_struct *work) > +{ > + struct dma_buf_io_ctx *ctx = container_of(work, struct dma_buf_io_ctx, > + release_work); > + struct dma_buf *dmabuf = ctx->dmabuf; > + long ret; > + > + dma_resv_lock(dmabuf->resv, NULL); > + /* Remove the last map, there should be no new ones going forward. */ > + dma_buf_io_drop_map(ctx); > + dma_resv_unlock(dmabuf->resv); > + > + /* Wait until all maps are destroyed. */ > + ret = dma_resv_wait_timeout(dmabuf->resv, DMA_RESV_USAGE_KERNEL, > + false, MAX_SCHEDULE_TIMEOUT); > + > + if (WARN_ON_ONCE(ret <= 0)) > + return; > + if (WARN_ON_ONCE(rcu_dereference_protected(ctx->map, true))) > + return; > + > + if (refcount_dec_and_test(&ctx->refs)) > + dma_buf_io_ctx_destroy_work(&ctx->release_work); > +} > + > +void dma_buf_io_ctx_release(struct dma_buf_io_ctx *ctx) > +{ > + /* > + * Destruction needs to wait for I/O and dma fences. Defer it to > + * simplify locking. > + */ > + INIT_WORK(&ctx->release_work, dma_buf_io_ctx_release_work); > + queue_work(system_wq, &ctx->release_work); > +} > + > +int dma_buf_io_ctx_create(struct file *file, > + struct dma_buf_io_ctx *ctx, > + struct dma_buf *dmabuf, > + enum dma_data_direction dir) > +{ > + int ret; > + > + if (!file->f_op->init_dma_buf_io_ctx) > + return -EOPNOTSUPP; > + > + memset(ctx, 0, sizeof(*ctx)); > + ctx->fence_ctx = dma_fence_context_alloc(1); > + ctx->dir = dir; > + ctx->dmabuf = dmabuf; > + refcount_set(&ctx->refs, 1); > + get_dma_buf(dmabuf); > + > + ret = file->f_op->init_dma_buf_io_ctx(file, ctx); > + if (ret) { > + memset(ctx, 0, sizeof(*ctx)); > + dma_buf_put(dmabuf); > + return ret; > + } > + > + if (WARN_ON_ONCE(!ctx->dev_ops || > + !ctx->dev_ops->map || > + !ctx->dev_ops->unmap || > + !ctx->dev_ops->release)) > + return -EINVAL; > + > + return ret; > +} > diff --git a/include/linux/dma-buf-io.h b/include/linux/dma-buf-io.h > new file mode 100644 > index 000000000000..bcffc6202556 > --- /dev/null > +++ b/include/linux/dma-buf-io.h > @@ -0,0 +1,91 @@ > +/* SPDX-License-Identifier: GPL-2.0 */ > +#ifndef __DMA_BUF_IO_H__ > +#define __DMA_BUF_IO_H__ > + > +#include <linux/dma-buf.h> > + > +struct dma_buf_io_fence; > +struct dma_buf_io_ctx; > +struct dma_buf_io_map; > + > +struct dma_buf_io_ops { > + /* > + * Create a new map for the given ctx. Called with the reservation > + * lock held. > + */ > + struct dma_buf_io_map *(*map)(struct dma_buf_io_ctx *ctx); > + > + /* > + * Clean up device specific parts of the @map. Called with the > + * reservation lock held. > + */ > + void (*unmap)(struct dma_buf_io_ctx *ctx, struct dma_buf_io_map *map); > + > + /* > + * The user tries to destroy the ctx. Release all device specific > + * parts of the token. > + */ > + void (*release)(struct dma_buf_io_ctx *); > +}; > + > +struct dma_buf_io_map { > + /* > + * Counts attached requests and other users. Device specific unmapping > + * is deferred until all refs are dropped. > + */ > + struct percpu_ref refs; > + > + struct work_struct release_work; > + struct dma_buf_io_fence *fence; > + struct dma_buf_io_ctx *ctx; > +}; > + > +struct dma_buf_io_ctx { > + struct dma_buf_io_map __rcu *map; > + struct dma_buf *dmabuf; > + enum dma_data_direction dir; > + > + atomic_t fence_seq; > + u64 fence_ctx; > + struct work_struct release_work; > + refcount_t refs; > + > + void *dev_priv; > + const struct dma_buf_io_ops *dev_ops; > +}; > + > +int dma_buf_io_ctx_create(struct file *file, > + struct dma_buf_io_ctx *ctx, > + struct dma_buf *dmabuf, > + enum dma_data_direction dir); > +void dma_buf_io_ctx_release(struct dma_buf_io_ctx *ctx); > + > +struct dma_buf_io_map *dma_buf_io_create_map(struct dma_buf_io_ctx *ctx); > + > +static inline struct dma_buf_io_map * > +dma_buf_io_get_map(struct dma_buf_io_ctx *ctx) > +{ > + struct dma_buf_io_map *map; > + > + guard(rcu)(); > + > + map = rcu_dereference(ctx->map); > + if (unlikely(!map || !percpu_ref_tryget_live_rcu(&map->refs))) > + return NULL; > + > + return map; > +} > + > +static inline void dma_buf_io_map_drop(struct dma_buf_io_map *map) > +{ > + percpu_ref_put(&map->refs); > +} > + > +/* > + * Device API > + */ > + > +void dma_buf_io_invalidate_mappings(struct dma_buf_io_ctx *ctx); > +int dma_buf_io_init_map(struct dma_buf_io_ctx *ctx, struct dma_buf_io_map *map); > + > +#endif > diff --git a/include/linux/fs.h b/include/linux/fs.h > index d10897b3a1e3..9fe349a279c7 100644 > --- a/include/linux/fs.h > +++ b/include/linux/fs.h > @@ -1914,6 +1914,7 @@ struct dir_context { > #define COPY_FILE_SPLICE (1 << 0) > > struct io_uring_cmd; > +struct dma_buf_io_ctx; > struct offset_ctx; > > typedef unsigned int __bitwise fop_flags_t; > @@ -1962,6 +1963,7 @@ struct file_operations { > int (*uring_cmd_iopoll)(struct io_uring_cmd *, struct io_comp_batch *, > unsigned int poll_flags); > int (*mmap_prepare)(struct vm_area_desc *); > + int (*init_dma_buf_io_ctx)(struct file *, struct dma_buf_io_ctx *); > } __randomize_layout; > > /* Supports async buffered reads */

1 month, 2 weeks

Re: [PATCH v9 3/5] rust: sync: Add abstraction for rcu_barrier()

by Gary Guo

On Wed Aug 5, 2026 at 3:59 PM BST, Philipp Stanner wrote: > rcu_barrier() is a frequently used C function which is always safe to be > called. > > Add a safe abstraction for rcu_barrier(). > > Signed-off-by: Philipp Stanner <phasta(a)kernel.org> > Tested-by: Daniel Almeida <daniel.almeida(a)collabora.com> Acked-by: Gary Guo <gary(a)garyguo.net> > --- > rust/kernel/sync/rcu.rs | 20 ++++++++++++++++++++ > 1 file changed, 20 insertions(+) > > diff --git a/rust/kernel/sync/rcu.rs b/rust/kernel/sync/rcu.rs > index a32bef6e490b..7031ca5d2473 100644 > --- a/rust/kernel/sync/rcu.rs > +++ b/rust/kernel/sync/rcu.rs > @@ -50,3 +50,23 @@ fn drop(&mut self) { > pub fn read_lock() -> Guard { > Guard::new() > } > + > +/// Wait until all in-flight call_rcu() callbacks complete. This misses some `` quoting but these can be applied on fixup. Best, Gary > +/// > +/// Note that this primitive does not necessarily wait for an RCU grace period > +/// to complete. For example, if there are no RCU callbacks queued anywhere > +/// in the system, then rcu_barrier() is within its rights to return > +/// immediately, without waiting for anything, much less an RCU grace period. > +/// In fact, rcu_barrier() will normally not result in any RCU grace periods > +/// beyond those that were already destined to be executed. > +/// > +/// In kernels built with CONFIG_RCU_LAZY=y, this function also hurries all > +/// pending lazy RCU callbacks. > +/// > +/// Note that this is one of the RCU primitives which must not be called in > +/// atomic context. > +#[inline] > +pub fn rcu_barrier() { > + // SAFETY: `rcu_barrier()` is always safe to be called. It just might wait for a grace period. > + unsafe { bindings::rcu_barrier() }; > +}

1 month, 2 weeks

Re: [PATCH v9 2/5] rust: types: implement ForeignOwnable for ARef<T>

by Gary Guo

On Wed Aug 5, 2026 at 3:59 PM BST, Philipp Stanner wrote: > From: Danilo Krummrich <dakr(a)kernel.org> > > Implement ForeignOwnable for ARef<T>, making it possible for C code to > own an ARef<T>. > > Since ARef represents shared ownership, BorrowedMut is &T rather than > &mut T, matching the semantics of the underlying reference-counted type. > > Signed-off-by: Danilo Krummrich <dakr(a)kernel.org> > Reviewed-by: Alice Ryhl <aliceryhl(a)google.com> > Tested-by: Daniel Almeida <daniel.almeida(a)collabora.com> > --- > rust/kernel/sync/aref.rs | 40 ++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 40 insertions(+) > > diff --git a/rust/kernel/sync/aref.rs b/rust/kernel/sync/aref.rs > index b721b2e00b98..540766613659 100644 > --- a/rust/kernel/sync/aref.rs > +++ b/rust/kernel/sync/aref.rs > @@ -24,6 +24,11 @@ > ptr::NonNull, // > }; > > +use crate::{ > + prelude::*, > + types::ForeignOwnable, // > +}; > + > /// Types that are _always_ reference counted. > /// > /// It allows such types to define their own custom ref increment and decrement functions. > @@ -188,6 +193,41 @@ fn eq(&self, other: &ARef<U>) -> bool { > } > impl<T: AlwaysRefCounted + Eq> Eq for ARef<T> {} > > +// SAFETY: `into_foreign` returns a pointer from `NonNull::as_ptr`, so it's non-null. The > +// `ARef` invariant guarantees that `ptr` points to a valid `T`, so it's aligned to `T`. > +unsafe impl<T: AlwaysRefCounted + 'static> ForeignOwnable for ARef<T> { This doesn't need to be static, if you can add `where Self: 'a` on `Borrowed` and `BorrowedMut` instead. Best, Gary > + const FOREIGN_ALIGN: usize = core::mem::align_of::<T>(); > + > + type Borrowed<'a> = &'a T; > + type BorrowedMut<'a> = &'a T; > + > + fn into_foreign(self) -> *mut c_void { > + ARef::into_raw(self).as_ptr().cast() > + } > + > + unsafe fn from_foreign(ptr: *mut c_void) -> Self { > + // SAFETY: The safety requirements of this function ensure that `ptr` comes from a previous > + // call to `Self::into_foreign`. > + let ptr = unsafe { NonNull::new_unchecked(ptr.cast()) }; > + > + // SAFETY: `ptr` came from `into_foreign`, which consumed an `ARef` without decrementing > + // the refcount, so we can transfer the ownership to the new `ARef`. > + unsafe { ARef::from_raw(ptr) } > + } > + > + unsafe fn borrow<'a>(ptr: *mut c_void) -> &'a T { > + // SAFETY: The safety requirements of this method ensure that the object remains alive and > + // immutable for the duration of 'a. > + unsafe { &*ptr.cast() } > + } > + > + unsafe fn borrow_mut<'a>(ptr: *mut c_void) -> &'a T { > + // SAFETY: The safety requirements for `borrow_mut` are a superset of the safety > + // requirements for `borrow`. > + unsafe { <Self as ForeignOwnable>::borrow(ptr) } > + } > +} > + > impl<T, U> PartialEq<&'_ U> for ARef<T> > where > T: AlwaysRefCounted + PartialEq<U>,

1 month, 2 weeks

Re: [RFC PATCH 10/30] vfio/pci: Export vfio dma-buf specific info for importers

by Jason Gunthorpe

On Mon, Jul 27, 2026 at 11:53:55AM -0700, Ackerley Tng wrote: > Hope you also had a chance to look at [1] that David Woodhouse is > working on :) > > [1] https://lore.kernel.org/all/1dc299af6b795a40c8887b3d84f915024f6446a9.camel@… Heh, that's awfully DMABUF like :) So, if that happens then sure we can probably create a VFIO exporter for it as well along side the DMABUF exporter Matt is working on and that should handle the shared/private steps intel needs Jason

1 month, 2 weeks

Re: [PATCH v5 2/9] PCI/P2PDMA: Add CONFIG_PCI_P2PDMA_CORE

by Jason Gunthorpe

On Tue, Aug 04, 2026 at 10:19:11AM -0600, Logan Gunthorpe wrote: > There's a vague convention for this already: the term 'p2pmem' is often > used for cases where the driver uses the allocator, etc. (I think I had > this intention when I wrote the code and have since forgotten about > it). I've been calling it the genalloc layer and the core layer. p2pmem would be OK to refer to the genalloc stuff. So if you want to have CONFIG_PCI_P2PDMA and CONFIG_PCI_P2PMEM that seem sOk > code into it's own file, potentially renaming some functions. Then, in > the end, we would probably have a pcim_p2pdma_supported() function and a > pcim_p2pmem_supported() function, the latter being used by existing use > cases. Not quite sure why we need this? Matt, the mlx5 stuff is the same as VFIO, it just uses the "core" layer and does not use the genalloc. So there shouldn't be an issue here, if the genalloc is off then the mlx5 stuff should still work. There shouldn't be a case where CONFIG_PCI_P2PDMA=y and mlx5 is broken? Did some of APIs get mixed into the genalloc family that should not have? Jason

1 month, 2 weeks

Re: [PATCH v6 4/6] rust: xarray: Add support for index reservations

by Philipp Stanner

On Wed, 2026-08-05 at 10:50 +0200, Andreas Hindborg wrote: > "Philipp Stanner" <phasta(a)kernel.org> writes: > > > One often cannot allocate in the kernel with the desired flags, most > > notably in atomic context. Pre-allocating the memory is the preferred > > solution in such situations. > > > > Add support for xa_reserve() in the Rust abstractions of xarray. Create > > a Reservation object similar to a lock-guard, that can be dropped once > > the reservation is no longer needed or once the index was stored to. > > > > Signed-off-by: Philipp Stanner <phasta(a)kernel.org> > > --- > > Please regard this more as an RFC. > > > > I need pre-allocating in XArray for DmaFence. How exactly we achieve > > this is open for discussion. > > Do you need to actually reserve a key, or do you just need atomic > allocation? I would just have needed a position to store to, but the fence sequence number would be the only reasonable index, so you'd also reserve a key. Anyways, please forget about this for now, we abstained from using the XArray in the current revision (v8) of this patch series. Thx P.

1 month, 2 weeks

Re: [PATCH v8 4/5] rust: Add dma_fence abstractions

by Philipp Stanner

On Tue, 2026-08-04 at 14:54 -0300, Daniel Almeida wrote: > > Hi Phillip :) > > Tested-by: Daniel Almeida <daniel.almeida(a)collabora.com> Hi, thx for the test I address your major feedback below; minor points like renaming I will address in the next revision. > > > On 31 Jul 2026, at 05:04, Philipp Stanner <phasta(a)kernel.org> wrote: > > […] > > An additional issue discovered during the review process of this code is > > that there is (currently) no mechanism in Rust to prevent someone from > > circumventing the DriverFence's FenceContext-reference's lifetime by > > "forgetting" the fence, e.g. with core::mem::forget(). Since this > > apparently can also happen with recfounting cycles, it's quite likely > > that it would enable UAF bugs on the FenceContext. Print warnings if > > this or other misuse of the fence API happens. > > I think cycles are fine, if anything they make things live longer than intended > (possibly leaking them forever), but at no point they lead to UAFs. And for the > mem::forget() point, there seems to be precedent in other patches pointing to > unsafe constructors, where the safety requirement is basically "don't > mem::forget() this". > > In fact, if you mem::forget() in the previous Arc approach, it seems like you leak > the context, while mem::forget() on the current solution does seem to allow UAF > by ending the borrow as you said yourself: > > let ctx = .... // FenceContext in some driver queue structure > let fence = ctx.fence_alloc(...).new_fence() // DriverFence<'_, T> // borrows ctx > mem::forget(fence); // ends the borrow > drop(ctx); // DriverFenceData contains a dangling &FenceContext, and that allocation is managed by C > > versus: > > let ctx: Arc<FenceCtx<...>> = ...; // same as above, assume refcount==1 > let fence = ctx.new_fence_allocation(..).new_fence(); // ctx refcount==2 > mem::forget(fence); // ctx refcount==2 > drop(ctx) //ctx refcount==1 > > I noticed that you added a counter to catch the first case, but that assumes that > signaled fences won’t reach into the context anymore, IIUC? More on that below. Answering on that further down below > > > […] > > SPDX is being used here, > > > + > > +/* > > + * Copyright (C) 2025, 2026 Red Hat Inc.: > > + * Author: Philipp Stanner <pstanner(a)redhat.com> > > + */ > > So perhaps SPDX-CopyrightText should be used here? What does that look like? I think I never saw it anywhere. > > > + > > […] > > > + // happening. > > + // > > + // However, we cannot fully guarantee in Rust that `DriverFence`s will not > > + // be forgotten, e.g., through refcounting. This could circumvent the > > + // lifetime which intends to enforce that all fences disappear before their > > + // context. > > + nr_of_unsignaled_fences: Atomic<u64>, > > Why are we tracking the number of unsignaled fences, specifically? Is it not > possible for the C side (which controls the actual allocation) to reach back > into the Rust side via the callbacks even after the context drops, regardless > of their signaled status? Tracking the number was agreed on as a compromise to move the abstraction forward. It only exists for a warning-print. The signaled status is absolutely decisive for preventing that both the DriverFence::data and FenceContext cannot be reached anymore by both C *and* Rust code consuming a Fence. The reason is that the fence backend ops can reach the DriverFence and the FenceContext. Only signalling the fence decouples them, and then you still have to wait for an RCU grace period to be sure that all accessors are gone. This is what the C backend forces on us, but it also applies for someone doing (future implementations of) Fence::get_driver_name() or Fence::is_signaled(). > > And in any case, even side stepping this signaled vs unsignaled dilemma that > this counter is trying to check, the end result seems to be "sorry, you misused > the API and now a UAF is possible". In this specific sense, it doesn't sound > that much better than what we are trying to move away from in C. > > The more I think about this borrow design, the more I believe a simple refcount > would be way safer. I mean, this field would go away to begin with, IIUC, and > that’s not even considering the point above. Refcounting does not solve the fundamental issue. A dma_fence must always correctly represent the state of the associated job on the GPU. If you forget a fence, you might deadlock. If you signal a fence for a job that is still running on the GPU, you might get unnoticed memory corruptions. Refcounting the fctx was also my solution, but was objected to by various parties (IIRC at least Boris and Danilo) who argue that the life time is the proper solution. If I understood correctly the major objection is that the refcount enables the device resources in FenceContext::data to arbitrarily outlive its device, which is something Danilo is very concerned about. Anyways, as we discussed in the call, and as also my code comment in FenceContext::drop() highlights, this atomic is only there for printing a warning, because the proper solution we actually want is to have the FenceContext signal all forgotten fences when it drops. *Then* you are really completely safe, because then the forgotten fences get decoupled from the fence context. So no UAF, although a driver that forgets fences would still be broken. The reason why (IIRC) Alice proposed doing the counter + warning and a TODO entry is to move forward faster with the fence implementation, which I understood is also in Tyr's interest (regarding this unlikely bug). The reason why it is not solved right now is that you need a data structure in FenceContext that keeps track of unsignaled fences. Entries in that data structure then have to be pre-allocated. And you have to remove entries from the data structure *whenever a fence signals*. The only good data structure we have in Rust currently is XArray, which we cannot use because: * it uses `usize` as an index, but dma_fence seqno is u64, so this could become a problem for Tyr, supporting 32-bit architectures. * We'd need to wait for Andreas' reservation mechanism for XArray. * XArray is not a good data structure for ever-increasing seqnos like the u64 of dma_fence. It quickly performance-degrades because it preserves index order, resulting in like a dozen pointer indirections for large indices. So I had investigated using a List, but the list would force you then to shove another refcounting mechanism (ListArc) on top of the stored fences, and Rust's list implementation is really not trivial to be used. At least not for me. Getting this right would take time. Note that with the JobQueue design we're aiming at right now, DriverFence and FenceContext would all be owned by JobQueue, so the driver couldn't ever forget a DriverFence whithout also forgetting the FenceContext. Hence, for the forseeable future, no one will encounter that bug. So, long story short, this is a solvable TODO, but we need a good data structure for it. Preferably a hash table? Or the list implementation needs to be improved so that it can take an ARef<Fence> instead of a ListArc<Fence>. But someone needs to do that work. Please tell me whether you agree with proceeding like this and make FenceContext::drop() completely waterproof later, or whether you want to suggest to do that now – but then I'd need help with the data structures. > > > > […] > > + /// The `data` you pass here must not perform any operations that are illegal > > + /// in atomic context in its [`Drop`] implementation. > > + pub fn fence_alloc(&self, data: T::FenceDataType) -> Result<DriverFenceAllocation<'_, T>> { > > ^ In the same spirit as the last iteration, can we please rename this? > > We don’t need to shorten methods and types IMHO. We can just say > “new_allocation” or a some other variation without “alloc”. Well, the name IMO needs to reflect that this method creates some fency object. So just "new_allocation" is not good I think because you want to see *what* is being allocated. new_fence_allocation() maybe? > > > […] > > +/// The receiving counterpart of a [`DriverFence`]. > > +/// > > +/// The Rust DMA fence implementation has a dualistic design: [`DriverFence`]s > > +/// are the producer-side, intended to be always owned by only one party. That > > +/// party has the monopoly on signalling the fence. > > +/// > > +/// A [`Fence`] is the counterpart for consumers. Thus, [`Fence`]s are always > > +/// refcounted and can shared with an arbitrary number of parties, including > > +/// userspace. Hereby, a [`Fence`] can only be used for actions such as checking > > +/// the fence's status or for registering callbacks on it. > > +/// > > +/// Once the associated [`DriverFence`] signals, all > > +/// [`FenceCallbackRegistration`]s registered on the [`Fence`] will be executed. > > +/// > > +/// A [`Fence`] can arbitrarily outlive its [`DriverFence`] and the > > +/// [`FenceContext`]. Signalling a [`DriverFence`] decouples it from its > > +/// [`Fence`]s. > > +#[repr(transparent)] > > +pub struct Fence { > > + /// The actual dma_fence passed to C. > > + inner: Opaque<bindings::dma_fence>, > > +} > > + > > +/// Guard helper for locking within this module. > > Is this new? Needs a bit more documentation. It is new, yes. Changelog mentions it. > > OTOH I think we shouldn’t come up with a new guard type. IIRC from > Lyude’s et al previous work, there’s already a way to build a Rust lock > from a C lock (Lock::from_raw()). In fact, I think this whole implementation > can boil down to: > > pub fn lock(&self) -> SpinLockIrqGuard<‘_, ()> { > let ptr = unsafe {bindings::dma_fence_spinlock(…)}; > unsafe { SpinLockIrq::<()>::from_raw(ptr)}.lock() > } > > This has a few advantages: > > a) doesn’t introduce its own guard type, instead reusing something that was > previously tested, > > b) you get the right behavior with the included NotThreadSafe token. > > c) the guard is now borrowed. Your guard is owned, which can easily lead to UB > because the lifetime is detached from the &self that originated it. This little helper is only used internally, and where it is used other code bits have to operate on the rawpointer with other unsafe blocks. At other places we use the fence rawpointer without taking the lock. So I'm not sure whether this will ever be clean. The only reason I added it was that I by now had 3 places where I do unsafe { } lock and unlock. Lyude's code is not yet merged. […] > > > +/// > > +/// let mut fctx = KBox::pin_init(FenceContext::new(0, driver_name, timeline_name, fctx_data), GFP_KERNEL)?; > > Missing rustfmt? I do run rustfmt. It doesn't do anything about this line. P.

1 month, 2 weeks

← Newer
1
...
8
9
10
11
12
13
14
...
359
Older →

Jump to page:

2026

2025

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

2012

2011

Linaro-mm-sig