Add dmabuf read/write via io_uring
Subsystems (9)
v9 cover Add dmabuf read/write via io_uring
The patch set allows to register a dmabuf to an io_uring instance for a specified file and use it with io_uring read / write requests. The infrastructure is not tied to io_uring and there could be more users in the future. A similar idea was attempted some years ago by Keith [1], from where I borrowed a number of changes. Later it was brough back up to life by Tushar and Vishal.
It's an opt-in feature for files, and they need to implement a new file operation to use it. Only NVMe block devices are supported in this series. The user API is built on top of io_uring's "registered buffers", where a dmabuf is registered in a special way, but after it can be used as any other "registered buffer" with IORING_OP_{READ,WRITE}_FIXED requests. It's created via a new file operation and the resulted map is then passed through the I/O stack in a new iterator type. There is some additional infrastructure to glue it together, count requests, manage lifetime and implement invalidation.
Tushar, William, Phil did a lot of testing and experimentation on various devices with previous versions of the patch set, and I've received lots of help from Anuj, Kanchan and Nitesh with investigations, testing, and patching. Earlier benchmarks by Anuj for IOMMU optimisations with udmabuf showed:
STRICT: before = 570 KIOPS, after = 5.01 MIOPS
LAZY: before = 1.93 MIOPS, after = 5.01 MIOPS
PASSTHROUGH: before = 5.01 MIOPS, after = 5.01 MIOPS
# Patch set structure:
- Patches 1-2 introduce internal API and infrastructure mediating io_uring and target subsystem / devices
- Patches 3-5: block layer support + prep patches
- Patches 6-8 implement NVMe support
- Patches 9-13 add io_uring support and uapi.
There are some liburing tests that can serve as an example:
git: https://github.com/isilence/liburing.git rw-dmabuf-tests-v8
url: https://github.com/isilence/liburing/tree/rw-dmabuf-tests-v8
The patches are based on Jens' for-next. Also available as a branch:
git: https://github.com/isilence/linux.git rw-dmabuf-v9
url: https://github.com/isilence/linux/tree/rw-dmabuf-v9
[1] https://lore.kernel.org/io-uring/[email protected]/
v9 - resend
v8: - fix blkdev_buffered_write() returning 0
- fix kvmalloc-kfree mismatch
- fix invalidation hang
- remove cpu dma sync as unnecessary for dev-to-dev transfers
- move nvme dma-buf code into a new file
- tighten dma-buf [re]export around io_uring issue
- simplify dma-buf-io locking/sync as proposed by Matthew Brost
v7: - refcount *_io_ctx
- wait rcu grace period for ctx->map before freeing maps
- increment map counters under dma resv lock
- more comments about bio splitting
- make ->unmap responsible for freeing maps to match ->map
- replace dma_sync_single* with full table sync
- detach dma-buf on nvme reset after cancel nvme_cancel_tagset
v6: - remove fences and wait on invalidation synchronously
- report all map requests are detached earlier, outside of wq
- handle nvme removal
- gate nvme_free_descriptors() on iod->nr_descriptors
- fix error handling in nvme_rq_setup_dmabuf_map()
- fix iov_iter_alignment()
- relax the 1G io_uring registered dma-buf limit
- fix type overflow issue in bio split
- allocate struct dma_buf_io_ctx inside the dma-buf-io code
- harden block checks against dma-buf + buffered io
v5: - Reject dma-buf with buffered IO for raw bdev
- Add lim->max_segments bio splitting
- Add bio_iov_iter_set() helper
- Fix io_uring uapi validation
- Other minor changes
NVMe:
- Rename nvme_pci_sgl_set_data() to nvme_pci_dma_iter_set_sgl(), split into its own prep patch.
- Convert segment walk loops to do-while.
- Drop adjacent segment coalescing logic.
- Drop first_dma/first_len from nvme_pci_dmabuf_sgl_nents()
- Remove entries > NVME_MAX_SEGS bailout; moving to the block layer.
- Factor SGL vs PRP decision making into a helper.
v4: - https://lore.kernel.org/all/[email protected]/
- Add sgl support from Anuj
- Move it under drivers/dma-buf/ and rename
- Fix mis-sized allocations
- Fix io_uring re-import mishandling
- Drop map before io_uring "task work"
- Move blk-mq callback to block_device_operations
- Convert bio flag to REQ_OP*
- Other small changes
v3: https://lore.kernel.org/io-uring/[email protected]/
- Rework io_uring registration
- Move token/map infrastructure code out of blk-mq
- Simplify callbacks: remove a separate blk-mq table, which was mostly just forwarding calls (to nvme).
- Don't skip dma sync depending on request direction
- Fix a couple of hangs
- Rename s/dma/dmabuf/
- Other small changes
v2: - Don't pass raw dma addresses, wrap it into a driver specific object
- Split into two objects: token and map
- Implement move_notify
Anuj Gupta (2):
nvme-pci: rename nvme_pci_sgl_set_data to nvme_pci_dma_iter_set_sgl
nvme-pci: add SGL support for the dmabuf path
Pavel Begunkov (11):
dma-buf: introduce initial file I/O infrastructure
iov_iter: add iterator type for dmabuf maps
block: always adjust bi_offset on bio_advance_iter
block: introduce dma map backed bio type
block: add dma-buf support for raw bdev
nvme-pci: implement dma-buf backed requests io_uring/rsrc: introduce buf registration structure io_uring/rsrc: extend buffer update io_uring/rsrc: add uncloneable regbuf flag io_uring/rsrc: add regbuf import flags io_uring/rsrc: add dmabuf backed registered buffers
block/bio.c | 15 +- block/blk-merge.c | 50 +++++++ block/fops.c | 24 +++- drivers/dma-buf/Makefile | 2 +- drivers/dma-buf/dma-buf-io.c | 183 ++++++++++++++++++++++++ drivers/md/dm-io-rewind.c | 6 +- drivers/nvme/host/Makefile | 1 + drivers/nvme/host/core.c | 12 ++ drivers/nvme/host/nvme.h | 2 + drivers/nvme/host/pci-dmabuf.c | 164 ++++++++++++++++++++++ drivers/nvme/host/pci-dmabuf.h | 73 ++++++++++ drivers/nvme/host/pci.c | 249 ++++++++++++++++++++++++++++++++- include/linux/bio.h | 21 +-- include/linux/blk-mq.h | 7 + include/linux/blk_types.h | 14 +- include/linux/blkdev.h | 2 + include/linux/bvec.h | 3 +- include/linux/dma-buf-io.h | 105 ++++++++++++++ include/linux/fs.h | 2 + include/linux/io_uring_types.h | 5 + include/linux/iov_iter.h | 2 + include/linux/uio.h | 11 ++ include/uapi/linux/io_uring.h | 31 +++- io_uring/Makefile | 1 + io_uring/dma-buf.c | 68 +++++++++ io_uring/dma-buf.h | 67 +++++++++ io_uring/rsrc.c | 172 +++++++++++++++++------ io_uring/rsrc.h | 32 ++++- io_uring/rw.c | 33 +++-- lib/iov_iter.c | 29 +++- 30 files changed, 1301 insertions(+), 85 deletions(-) create mode 100644 drivers/dma-buf/dma-buf-io.c create mode 100644 drivers/nvme/host/pci-dmabuf.c create mode 100644 drivers/nvme/host/pci-dmabuf.h create mode 100644 include/linux/dma-buf-io.h create mode 100644 io_uring/dma-buf.c create mode 100644 io_uring/dma-buf.h
Signature · 1 line
2.54.0
PATCH v9 1/13 dma-buf: introduce initial file I/O infrastructure
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 facilitate 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 any new I/O request will try to create a new one.
The primary task of dma_buf_io_map is to count requests using it and wait for their completion when we want to destroy the DMA map.
[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.
--- drivers/dma-buf/Makefile | 2 +- drivers/dma-buf/dma-buf-io.c | 183 +++++++++++++++++++++++++++++++++++ include/linux/dma-buf-io.h | 105 ++++++++++++++++++++ include/linux/fs.h | 2 + 4 files changed, 291 insertions(+), 1 deletion(-) create mode 100644 drivers/dma-buf/dma-buf-io.c create mode 100644 include/linux/dma-buf-io.h
drivers/dma-buf/Makefile +1 -1
@@ -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
drivers/dma-buf/dma-buf-io.c new file +183 -0
183 lines not on this page · raw message
include/linux/dma-buf-io.h new file +105 -0
105 lines not on this page · raw message
include/linux/fs.h +2 -0
@@ -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 ;struct file_operations {
@@ -1960,6 +1961,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 */
Signature · 1 line
2.54.0
[email protected]
AI reviewRe: [PATCH v9 01/13] dma-buf: introduce initial file I/O infrastructure
Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider: - [High] dma-buf: memory leak in dma_buf_io_ctx_create() on dev_ops validation failure - [High] dma-buf…
Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [High] dma-buf: memory leak in dma_buf_io_ctx_create() on dev_ops validation failure
- [High] dma-buf: GFP_KERNEL allocation while holding dma_resv lock causes reclaim deadlocks
- [High] dma-buf: deadlock when waiting for I/O completion with dma_resv lock held
- [Medium] dma-buf: memory leak of map object in __dma_buf_io_create_map() on min_seg_shift failure
- [Medium] dma-buf: memory leak when ctx->dev_ops validation fails
- [Medium] dma-buf: map and percpu_ref leak when min_seg_shift is zero
--
commit 942b5b5c2d4bd9e70cda186064a1b0a1f705440a
Author: Pavel Begunkov <[email protected]>
dma-buf: introduce initial file I/O infrastructure
This commit introduces the basic building blocks (dma_buf_io_ctx and dma_buf_io_map) to support using dma-buf natively in read-write / IO paths. It provides an API between upper layer subsystems (like io_uring) and importer drivers to facilitate map creation and invalidation.
Quoted 6 lines
drivers/dma-buf/dma-buf-io.c new file +0
-0@@ -0,0 +1,183 @@
[ ... ]
Quoted 5 lines
drivers/dma-buf/dma-buf-io.c new file +5
-0+static void dma_buf_io_kill_map (struct dma_buf_io_ctx *ctx) +{ +struct dma_buf_io_map *map; + +dma_resv_assert_held (ctx->dmabuf->resv);
[ ... ]
drivers/dma-buf/dma-buf-io.c line 42
Quoted 5 lines
drivers/dma-buf/dma-buf-io.c new file +5
-0+rcu_assign_pointer (ctx->map,NULL ); +percpu_ref_kill (&map->refs); +/* make sure the map is not visible via ctx->map */ +synchronize_rcu_expedited (); +wait_for_completion (&map->drained);
[Severity: High]
Does waiting for I/O completion here risk a deadlock with memory reclaim?
Since dma_buf_io_kill_map() is called with the dma_resv lock held (and asserted above), waiting synchronously for unbounded in-flight I/O via wait_for_completion() could block memory shrinkers.
If the active I/O operation itself requires memory allocation, it creates an ABBA deadlock where the I/O needs memory, the shrinker needs the dma_resv lock, and this function holds the lock while waiting for the I/O.
This is especially problematic when called through dma_buf_io_invalidate_mappings() from an exporter's invalidate_mappings callback during memory pressure.
Quoted 5 lines
drivers/dma-buf/dma-buf-io.c new file +5
-0+dma_buf_io_map_release (map); +} + +int dma_buf_io_init_map (struct dma_buf_io_ctx *ctx,struct dma_buf_io_map *map, +struct sg_table *sgt)
drivers/dma-buf/dma-buf-io.c line 57
Quoted 10 lines
drivers/dma-buf/dma-buf-io.c new file +10
-0+{ +unsigned seg_shift = ~0U ; +struct scatterlist *sg; +unsigned long tmp; +int ret; + +for_each_sgtable_dma_sg (sgt, sg, tmp) + seg_shift =min (seg_shift,__ffs (sg_dma_len (sg))); + + ret =percpu_ref_init (&map->refs, dma_buf_io_map_refs_cb,0 ,GFP_KERNEL );
[Severity: High]
Can this GFP_KERNEL allocation trigger a reclaim deadlock?
dma_buf_io_init_map() initializes the percpu_ref using GFP_KERNEL, which permits direct memory reclaim. This function is invoked from the driver's map callback via __dma_buf_io_create_map().
However, the caller dma_buf_io_create_map() acquires the dma_resv lock before executing this sequence:
dma_buf_io_create_map() {
ret = dma_resv_lock_interruptible(ctx->dmabuf->resv, NULL);
...
map = __dma_buf_io_create_map(ctx);
}Because GPU and other dma-buf shrinkers attempt to acquire dma_resv locks to evict buffers, doing a GFP_KERNEL allocation while holding this lock creates an ABBA deadlock vulnerability. Should this use GFP_NOWAIT or be moved outside the lock?
Quoted 2 lines
drivers/dma-buf/dma-buf-io.c new file +2
-0+if (ret) +return ret;
[ ... ]
Quoted 2 lines
drivers/dma-buf/dma-buf-io.c new file +2
-0+static struct dma_buf_io_map *__dma_buf_io_create_map (struct dma_buf_io_ctx *ctx) +{
[ ... ]
drivers/dma-buf/dma-buf-io.c line 87
Quoted 6 lines
drivers/dma-buf/dma-buf-io.c new file +6
-0+ map = ctx->dev_ops->map (ctx); +if (IS_ERR (map)) +return map; + +if (WARN_ON_ONCE (!map->min_seg_shift)) +return ERR_PTR (-EFAULT );
[Severity: Medium]
If map->min_seg_shift is zero, does this leak the map object?
The early return -EFAULT happens after the driver successfully allocates and initializes the map via ctx->dev_ops->map(ctx). Returning here bypasses releasing the map and its percpu_ref initialized in dma_buf_io_init_map(), as well as any driver-specific resources.
Quoted 2 lines
drivers/dma-buf/dma-buf-io.c new file +2
-0+ + ret = dma_resv_wait_timeout(dmabuf->resv, DMA_RESV_USAGE_KERNEL,
[ ... ]
Quoted 5 lines
drivers/dma-buf/dma-buf-io.c new file +5
-0+int dma_buf_io_ctx_create (struct file *file, +struct dma_buf *dmabuf, +enum dma_data_direction dir, +struct dma_buf_io_ctx **out_ctx) +{
[ ... ]
drivers/dma-buf/dma-buf-io.c line 155
Quoted 12 lines
drivers/dma-buf/dma-buf-io.c new file +12
-0+ ret = file->f_op->init_dma_buf_io_ctx (file, ctx); +if (ret) { +kfree (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 ;
[Severity: High]
If the dev_ops validation fails here, do we leak ctx and the dmabuf reference?
This early return abandons the ctx object and dmabuf reference without cleaning them up. It skips freeing ctx, dropping the dmabuf reference acquired earlier by get_dma_buf(dmabuf), and executing any driver-specific cleanup that might have occurred during init_dma_buf_io_ctx().
Quoted 4 lines
drivers/dma-buf/dma-buf-io.c new file +4
-0+ + *out_ctx = ctx; +return 0 ; +}
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1
Re: [PATCH v9 01/13] dma-buf: introduce initial file I/O infrastructure
In reply to Pavel BegunkovQuoted 20 lines · Pavel Begunkov
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_ctxand structdma_buf_io_map. The ctx is used during initial registration and serves as an interface between the upper layer user likeio_uringand to the importer subsystem / driver. The map represents the actual dma map established for the target device[s] withdma_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 facilitate 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 any new I/O request will try to create a new one.
Quoted 9 lines
The primary task of
dma_buf_io_mapis to count requests using it and wait for their completion when we want to destroy the DMA map.[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.
Full disclosure Pavel incorporated some of my ideas wrt to locking cleanup which I shared off the list.
So I can't in fairness give an RB but:
For the series - I can give this and I have pulled to code + have Xe nvme + IGT setup showing this working:
Quoted 345 lines
Signed-off-by Pavel Begunkov <[email protected]>--- drivers/dma-buf/Makefile | 2 +- drivers/dma-buf/dma-buf-io.c | 183 +++++++++++++++++++++++++++++++++++ include/linux/dma-buf-io.h | 105 ++++++++++++++++++++ include/linux/fs.h | 2 + 4 files changed, 291 insertions(+), 1 deletion(-) create mode 100644 drivers/dma-buf/dma-buf-io.c create mode 100644 include/linux/dma-buf-io.hdrivers/dma-buf/Makefile +1
-1@@ -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.odrivers/dma-buf/dma-buf-io.c new file +183
-0@@ -0,0 +1,183 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +/* +* Common infrastructure for supporing dma-buf in the I/O path. +* +* Copyright (C) 2026 Pavel Begunkov <[email protected]> +*/ +#include <linux/dma-buf-io.h> +#include <linux/dma-resv.h> + +static void dma_buf_io_map_release (struct dma_buf_io_map *map) +{ +struct dma_buf_io_ctx *ctx = map->ctx; + +dma_resv_assert_held (ctx->dmabuf->resv); + +percpu_ref_exit (&map->refs); + ctx->dev_ops->unmap (ctx, map); +} + +static void dma_buf_io_map_refs_cb (struct percpu_ref *ref) +{ + struct dma_buf_io_map *map = container_of(ref, struct dma_buf_io_map, refs); + +complete (&map->drained); +} + +static void dma_buf_io_kill_map (struct dma_buf_io_ctx *ctx) +{ +struct dma_buf_io_map *map; + +dma_resv_assert_held (ctx->dmabuf->resv); + + map =rcu_dereference_protected (ctx->map, +dma_resv_held (ctx->dmabuf->resv)); +if (!map) +return ; + +rcu_assign_pointer (ctx->map,NULL ); +percpu_ref_kill (&map->refs); +/* make sure the map is not visible via ctx->map */ +synchronize_rcu_expedited (); +wait_for_completion (&map->drained); +dma_buf_io_map_release (map); +} + +int dma_buf_io_init_map (struct dma_buf_io_ctx *ctx,struct dma_buf_io_map *map, +struct sg_table *sgt) +{ +unsigned seg_shift = ~0U ; +struct scatterlist *sg; +unsigned long tmp; +int ret; + +for_each_sgtable_dma_sg (sgt, sg, tmp) + seg_shift =min (seg_shift,__ffs (sg_dma_len (sg))); + + ret =percpu_ref_init (&map->refs, dma_buf_io_map_refs_cb,0 ,GFP_KERNEL ); +if (ret) +return ret; +init_completion (&map->drained); + map->min_seg_shift = seg_shift; + map->ctx = ctx; +return 0 ; +} +EXPORT_SYMBOL_NS_GPL (dma_buf_io_init_map,"DMA_BUF" ); + +static 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; + +dma_resv_assert_held (dmabuf->resv); + +if (ctx->is_dead) +return ERR_PTR (-ENOENT ); + + map =__dma_buf_io_get_map (ctx); +if (map) +return map; + + map = ctx->dev_ops->map (ctx); +if (IS_ERR (map)) +return map; + +if (WARN_ON_ONCE (!map->min_seg_shift)) +return ERR_PTR (-EFAULT ); + + ret =dma_resv_wait_timeout (dmabuf->resv,DMA_RESV_USAGE_KERNEL , + true,MAX_SCHEDULE_TIMEOUT ); +if (ret <=0 ) { +if (!ret) + ret = -EAGAIN ; +dma_buf_io_map_release (map); +return ERR_PTR (ret); + } + +/* get a reference for the caller */ +percpu_ref_get (&map->refs); +rcu_assign_pointer (ctx->map, map); +return map; +} + +struct dma_buf_io_map *dma_buf_io_create_map (struct dma_buf_io_ctx *ctx) +{ +struct dma_buf_io_map *map; +int ret; + + ret =dma_resv_lock_interruptible (ctx->dmabuf->resv,NULL ); +if (ret) +return ERR_PTR (ret); + + map =__dma_buf_io_create_map (ctx); +dma_resv_unlock (ctx->dmabuf->resv); +return map; +} + +static void dma_buf_io_ctx_shutdown (struct dma_buf_io_ctx *ctx) +{ +dma_resv_lock (ctx->dmabuf->resv,NULL ); + ctx->is_dead = true; +dma_buf_io_kill_map (ctx); +dma_resv_unlock (ctx->dmabuf->resv); +} + +int dma_buf_io_ctx_create (struct file *file, +struct dma_buf *dmabuf, +enum dma_data_direction dir, +struct dma_buf_io_ctx **out_ctx) +{ +struct dma_buf_io_ctx *ctx; +int ret; + +if (!file->f_op->init_dma_buf_io_ctx) +return -EOPNOTSUPP ; + + ctx =kzalloc_obj (*ctx); +if (!ctx) +return -ENOMEM ; + ctx->dir = dir; + ctx->dmabuf = dmabuf; +get_dma_buf (dmabuf); + + ret = file->f_op->init_dma_buf_io_ctx (file, ctx); +if (ret) { +kfree (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 ; + + *out_ctx = ctx; +return 0 ; +} + +void dma_buf_io_ctx_release (struct dma_buf_io_ctx *ctx) +{ +dma_buf_io_ctx_shutdown (ctx); + +if (WARN_ON_ONCE (rcu_dereference_protected (ctx->map, true))) +return ; + + ctx->dev_ops->release (ctx); +dma_buf_put (ctx->dmabuf); +kfree (ctx); +} + +void dma_buf_io_invalidate_mappings (struct dma_buf_io_ctx *ctx) +{ +dma_buf_io_kill_map (ctx); +} +EXPORT_SYMBOL_NS_GPL (dma_buf_io_invalidate_mappings,"DMA_BUF" ); + +void dma_buf_io_detach (struct dma_buf_io_ctx *ctx) +{ +dma_buf_io_ctx_shutdown (ctx); +} +EXPORT_SYMBOL_NS_GPL (dma_buf_io_detach,"DMA_BUF" );include/linux/dma-buf-io.h new file +105
-0@@ -0,0 +1,105 @@ +/* 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_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; +/* +* Shift for the minimum segment size of the mapping. +*/ +unsigned min_seg_shift; + +struct dma_buf_io_ctx *ctx; +struct completion drained; +}; + +struct dma_buf_io_ctx { +struct dma_buf_io_map __rcu *map; +struct dma_buf *dmabuf; +enum dma_data_direction dir; +bool is_dead; + +void *dev_priv; +const struct dma_buf_io_ops *dev_ops; +}; + +int dma_buf_io_ctx_create (struct file *file, +struct dma_buf *dmabuf, +enum dma_data_direction dir, +struct dma_buf_io_ctx **ctx); +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 struct dma_buf_io_map * +dma_buf_io_get_map (struct dma_buf_io_ctx *ctx,bool nowait) +{ +struct dma_buf_io_map *map; + + map =__dma_buf_io_get_map (ctx); +if (likely (map)) +return map; + +if (nowait) +return ERR_PTR (-EAGAIN ); +return dma_buf_io_create_map (ctx); +} + +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, +struct sg_table *sgt); +void dma_buf_io_detach (struct dma_buf_io_ctx *ctx); + +#endif include/linux/fs.h +2
-0@@ -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 ;struct file_operations {@@ -1960,6 +1961,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 */ Signature · 1 line
2.54.0
Re: [PATCH v9 01/13] dma-buf: introduce initial file I/O infrastructure
Quoted 34 lines · Matthew Brost
Quoted 20 lines · Pavel Begunkov
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_ctxand structdma_buf_io_map. The ctx is used during initial registration and serves as an interface between the upper layer user likeio_uringand to the importer subsystem / driver. The map represents the actual dma map established for the target device[s] withdma_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 facilitate 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 any new I/O request will try to create a new one.Quoted 9 lines
The primary task of
dma_buf_io_mapis to count requests using it and wait for their completion when we want to destroy the DMA map.[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.Full disclosure Pavel incorporated some of my ideas wrt to locking cleanup which I shared off the list.
Quoted 7 lines
So I can't in fairness give an RB but:
Acked-by Matthew Brost <[email protected]>For the series - I can give this and I have pulled to code + have Xe nvme + IGT setup showing this working:
Tested-by Matthew Brost <[email protected]>
Thanks for the suggestions and testing!
Signature · 1 line
Pavel Begunkov
PATCH v9 2/13 iov_iter: add iterator type for dmabuf maps
In reply to Pavel BegunkovIntroduce a new iterator type for dmabuf maps. The map in an opaque object with internals and format specific to the subsystem / driver, and only it can use that subsystem / driver for issuing IO. The task of the middle layers is to pass the map / iterator further down, maybe doing basic splitting and length checking. The iterator can only be used by operations of the file the associated map was created for.
--- include/linux/iov_iter.h | 2 ++ include/linux/uio.h | 11 +++++++++++ lib/iov_iter.c | 29 +++++++++++++++++++++++------ 3 files changed, 36 insertions(+), 6 deletions(-)
include/linux/iov_iter.h +2 -0
@@ -310,6 +310,8 @@ size_t iterate_and_advance2(struct iov_iter *iter, size_t len, void *priv,return iterate_folioq (iter, len, priv, priv2, step);if (iov_iter_is_xarray (iter))return iterate_xarray (iter, len, priv, priv2, step); +if (WARN_ON_ONCE (iov_iter_is_dmabuf_map (iter))) +return 0 ;return iterate_discard (iter, len, priv, priv2, step); }
include/linux/uio.h +11 -0
@@ -12,6 +12,7 @@struct page ;struct folio_queue ; +struct dma_buf_io_map ;typedef unsigned int __bitwise iov_iter_extraction_t ;
@@ -29,6 +30,7 @@ enum iter_type {ITER_FOLIOQ ,ITER_XARRAY ,ITER_DISCARD , + ITER_DMABUF_MAP, };#define ITER_SOURCE 1 // == WRITE
@@ -71,6 +73,7 @@ struct iov_iter {const struct folio_queue *folioq;struct xarray *xarray;void __user *ubuf; +struct dma_buf_io_map *dmabuf_map; };size_t count; };
@@ -155,6 +158,11 @@ static inline bool iov_iter_is_xarray(const struct iov_iter *i)return iov_iter_type (i) ==ITER_XARRAY ; } +static inline bool iov_iter_is_dmabuf_map (const struct iov_iter *i) +{ +return iov_iter_type (i) ==ITER_DMABUF_MAP ; +} +static inline unsigned char iov_iter_rw (const struct iov_iter *i) {return i->data_source ?WRITE :READ ;
@@ -300,6 +308,9 @@ void iov_iter_folio_queue(struct iov_iter *i, unsigned int direction, unsigned int first_slot, unsigned int offset, size_t count);void iov_iter_xarray (struct iov_iter *i,unsigned int direction,struct xarray *xarray,loff_t start,size_t count); +void iov_iter_dmabuf_map (struct iov_iter *i,unsigned int direction, +struct dma_buf_io_map *map, +loff_t off,size_t count);ssize_t iov_iter_get_pages2 (struct iov_iter *i,struct page **pages,size_t maxsize,unsigned maxpages,size_t *start); ssize_t iov_iter_get_pages_alloc2(struct iov_iter *i, struct page ***pages,
lib/iov_iter.c +23 -6
@@ -575,7 +575,8 @@ void iov_iter_advance(struct iov_iter *i, size_t size) {if (unlikely (i->count < size)) size = i->count; -if (likely (iter_is_ubuf (i)) ||unlikely (iov_iter_is_xarray (i))) { +if (likely (iter_is_ubuf (i)) ||unlikely (iov_iter_is_xarray (i)) || +unlikely (iov_iter_is_dmabuf_map (i))) { i->iov_offset += size; i->count -= size; }else if (likely (iter_is_iovec (i) ||iov_iter_is_kvec (i))) {
@@ -631,7 +632,8 @@ void iov_iter_revert(struct iov_iter *i, size_t unroll)return ; } unroll -= i->iov_offset; -if (iov_iter_is_xarray (i) ||iter_is_ubuf (i)) { +if (iov_iter_is_xarray (i) ||iter_is_ubuf (i) || +iov_iter_is_dmabuf_map (i)) {BUG ();/* We should never go beyond the start of the specified * range since we might then be straying into pages that * aren't pinned.
@@ -775,6 +777,20 @@ void iov_iter_xarray(struct iov_iter *i, unsigned int direction, }EXPORT_SYMBOL (iov_iter_xarray); +void iov_iter_dmabuf_map (struct iov_iter *i,unsigned int direction, +struct dma_buf_io_map *map, +loff_t off,size_t count) +{ +WARN_ON (direction & ~(READ |WRITE )); + *i = (struct iov_iter ){ + .iter_type =ITER_DMABUF_MAP , + .data_source = direction, + .dmabuf_map = map, + .count = count, + .iov_offset = off, + }; +} +/** * iov_iter_discard - Initialise an I/O iterator that discards data * @i: The iterator to initialise.
@@ -856,7 +872,7 @@ unsigned long iov_iter_alignment(const struct iov_iter *i)return iov_iter_alignment_bvec (i);/* With both xarray and folioq types, we're dealing with whole folios. */ -if (iov_iter_is_folioq (i)) +if (iov_iter_is_folioq (i) ||iov_iter_is_dmabuf_map (i))return i->iov_offset | i->count;if (iov_iter_is_xarray (i))return (i->xarray_start + i->iov_offset) | i->count;
@@ -872,7 +888,7 @@ unsigned long iov_iter_gap_alignment(const struct iov_iter *i)size_t size = i->count;unsigned k; -if (iter_is_ubuf (i)) +if (iter_is_ubuf (i) ||iov_iter_is_dmabuf_map (i))return 0 ;if (WARN_ON (!iter_is_iovec (i)))
@@ -1469,11 +1485,12 @@ EXPORT_SYMBOL_GPL(import_ubuf);void iov_iter_restore (struct iov_iter *i,struct iov_iter_state *state) {if (WARN_ON_ONCE (!iov_iter_is_bvec (i) && !iter_is_iovec (i) && - !iter_is_ubuf (i)) && !iov_iter_is_kvec (i)) + !iter_is_ubuf (i) && !iov_iter_is_kvec (i) && + !iov_iter_is_dmabuf_map (i)))return ; i->iov_offset = state->iov_offset; i->count = state->count; -if (iter_is_ubuf (i)) +if (iter_is_ubuf (i) ||iov_iter_is_dmabuf_map (i))return ;/* * For the *vec iters, nr_segs + iov is constant - if we increment
Signature · 1 line
2.54.0
[email protected]
AI reviewRe: [PATCH v9 02/13] iov_iter: add iterator type for dmabuf maps
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [High] lib: iov_iter_dmabuf_map 64-bit offset truncation and revert underflow --
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] lib: iov_iter_dmabuf_map 64-bit offset truncation and revert underflow --
commit 51412f5d857b723775fa3b8143fbc7dfc3a8a633
Author: Pavel Begunkov <[email protected]>
iov_iter: add iterator type for dmabuf maps
This commit introduces a new iterator type, ITER_DMABUF_MAP, for dmabuf maps. The map is an opaque object specific to the subsystem or driver, and the iterator is used by the middle layers to pass the map further down for IO operations.
Quoted 4 lines
lib/iov_iter.c +0
-0
[ ... ]
lib/iov_iter.c void iov_iter_revert(struct iov_iter *i, size_t unroll) · line 637
Quoted 8 lines
lib/iov_iter.c +2
-1@@ -631,7 +632,8 @@ void iov_iter_revert(struct iov_iter *i, size_t unroll)return ; } unroll -= i->iov_offset; -if (iov_iter_is_xarray (i) ||iter_is_ubuf (i)) { +if (iov_iter_is_xarray (i) ||iter_is_ubuf (i) || +iov_iter_is_dmabuf_map (i)) {BUG ();/* We should never go beyond the start of the specified
[Severity: High]
Because ITER_DMABUF_MAP sets iov_offset to the absolute file offset in iov_iter_dmabuf_map(), does this break the safety invariant here?
For iterators validated by this BUG() check (such as ITER_XARRAY and ITER_UBUF), iov_offset is normally strictly initialized to 0 to track the relative number of bytes advanced. Since iov_offset starts at the absolute offset for ITER_DMABUF_MAP, the condition if (unroll <= i->iov_offset) just above this hunk could evaluate to true even if the caller attempts to unroll more bytes than were actually advanced, potentially allowing silent out-of-bounds iteration.
[ ... ]
Quoted 5 lines
lib/iov_iter.c +1
-0@@ -775,6 +777,20 @@ void iov_iter_xarray(struct iov_iter *i, unsigned int direction, }EXPORT_SYMBOL (iov_iter_xarray); +void iov_iter_dmabuf_map(struct iov_iter *i, unsigned int direction,
lib/iov_iter.c void iov_iter_xarray(struct iov_iter *i, unsigned int direction, · line 790
Quoted 10 lines
lib/iov_iter.c +10
-0+struct dma_buf_io_map *map, + loff_t off, size_t count) +{ + WARN_ON(direction & ~(READ | WRITE)); + *i = (struct iov_iter){ + .iter_type = ITER_DMABUF_MAP, + .data_source = direction, + .dmabuf_map = map, + .count = count, + .iov_offset = off,
[Severity: High]
Does this silently truncate the 64-bit offset?
The off argument is a 64-bit loff_t, but iov_offset is a size_t (which is 32 bits on 32-bit platforms). Any offset >= 4GB will be silently truncated here. Unlike ITER_XARRAY which uses a separate 64-bit xarray_start union field, ITER_DMABUF_MAP doesn't store this absolute offset anywhere else.
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2
PATCH v9 3/13 block: always adjust bi_offset on bio_advance_iter
In reply to Pavel BegunkovExtend bio_no_advance_iter() iteration to also increment the offset. It's not currently needed because the currently listed request types don't have a buffer, but will be useful once we add bios backed by dma-buf, which don't have a bvec but work with a single range.
--- drivers/md/dm-io-rewind.c | 6 ++++-- include/linux/bio.h | 12 ++++++++---- 2 files changed, 12 insertions(+), 6 deletions(-)
drivers/md/dm-io-rewind.c +4 -2
@@ -113,10 +113,12 @@ static inline void dm_bio_rewind_iter(const struct bio *bio, iter->bi_sector -= bytes >>9 ;/* No advance means no rewind */ -if (bio_no_advance_iter (bio)) +if (bio_no_advance_iter (bio)) { iter->bi_size += bytes; -else + iter->bi_offset -= bytes; + }else {dm_bvec_iter_rewind (bio->bi_io_vec, iter, bytes); + } }/**
include/linux/bio.h +8 -4
@@ -113,11 +113,13 @@ static inline void bio_advance_iter(const struct bio *bio, { iter->bi_sector += bytes >>9 ; -if (bio_no_advance_iter (bio)) +if (bio_no_advance_iter (bio)) { iter->bi_size -= bytes; -else + iter->bi_offset += bytes; + }else {bvec_iter_advance (bio->bi_io_vec, iter, bytes);/* TODO: It is reasonable to complete bio with error here. */ + } }/* @bytes should be less or equal to bvec[i->bi_idx].bv_len */
@@ -127,10 +129,12 @@ static inline void bio_advance_iter_single(const struct bio *bio, { iter->bi_sector += bytes >>9 ; -if (bio_no_advance_iter (bio)) +if (bio_no_advance_iter (bio)) { iter->bi_size -= bytes; -else + iter->bi_offset += bytes; + }else {bvec_iter_advance_single (bio->bi_io_vec, iter, bytes); + } }void __bio_advance (struct bio *,unsigned bytes);
Signature · 1 line
2.54.0
PATCH v9 4/13 block: introduce dma map backed bio type
In reply to Pavel BegunkovPremapped buffers don't require a generic bio_vec since these have already been dma mapped. Repurpose the bi_io_vec space to store dmabuf maps as they are mutually exclusive.
The bio splitting differs from the normal path because it's already pre-mapped and for the block layer it's just an offset into the dma-buf. The actual segmentation is only available to the importer driver and not the block layer, however, it doesn't contain alignment gaps and we don't have to check it. For the same reason we can't precisely split by the number of segments, but we use ->min_seg_shift stored in the map to calculate the minimum number of bytes a request consisting of lim->max_segments full segments can cover and split by that. It's stricter and can add extra splitting. E.g. for a {4K, 4G} segmentation, the min segment size is 4K, and we'll split it into bios of (4K * lim->max_segments) bytes each, but it should be good enough for now to cover the most popular use cases.
--- block/bio.c | 15 ++++++++++-- block/blk-merge.c | 50 +++++++++++++++++++++++++++++++++++++++ block/fops.c | 2 +- include/linux/bio.h | 9 +++---- include/linux/blk-mq.h | 7 ++++++ include/linux/blk_types.h | 14 ++++++++++- include/linux/bvec.h | 3 ++- 7 files changed, 91 insertions(+), 9 deletions(-)
block/bio.c +13 -2
@@ -881,7 +881,11 @@ static int __bio_clone(struct bio *bio, struct bio *bio_src, gfp_t gfp) bio->bi_write_stream = bio_src->bi_write_stream; bio->bi_bvec_gap_bit = bio_src->bi_bvec_gap_bit; bio->bi_iter = bio_src->bi_iter; - bio->bi_io_vec = bio_src->bi_io_vec; + +if (op_is_dmabuf (bio->bi_opf)) + bio->bi_dmabuf_map = bio_src->bi_dmabuf_map; +else + bio->bi_io_vec = bio_src->bi_io_vec;if (bio->bi_bdev) { if (bio->bi_bdev == bio_src->bi_bdev &&
@@ -1204,16 +1208,23 @@ EXPORT_SYMBOL_GPL(__bio_release_pages);bool bio_iov_iter_set (struct bio *bio,const struct iov_iter *iter) { -if (!iov_iter_is_bvec (iter)) +if (!iov_iter_is_bvec (iter) && !iov_iter_is_dmabuf_map (iter))return false;WARN_ON_ONCE (bio->bi_max_vecs); +static_assert (offsetof(struct bio , bi_io_vec) == + offsetof(struct bio , bi_dmabuf_map)); +static_assert (offsetof(struct iov_iter , bvec) == + offsetof(struct iov_iter , dmabuf_map)); + bio->bi_io_vec = (struct bio_vec *)iter->bvec; bio->bi_iter.bi_idx =0 ; bio->bi_iter.bi_offset = iter->iov_offset; bio->bi_iter.bi_size =iov_iter_count (iter);bio_set_flag (bio,BIO_CLONED ); +if (iov_iter_is_dmabuf_map (iter)) + bio->bi_opf |=REQ_NOMERGE |REQ_DMABUF ;return true; }
block/blk-merge.c +50 -0
74 lines not on this page · raw message
block/fops.c +1 -1
@@ -362,7 +362,7 @@ static ssize_t __blkdev_direct_IO_async(struct kiocb *iocb,* Users don't rely on the iterator being in any particular * state for async I/O returning -EIOCBQUEUED, hence we can * avoid expensive iov_iter_advance(). Bypass -* bio_iov_iter_get_pages() and set the bvec directly. +* bio_iov_iter_get_pages() and set the bvec/dmabuf directly. */ if (!bio_iov_iter_set (bio, iter)) { ret =blkdev_iov_iter_get_pages (bio, iter, bdev);
include/linux/bio.h +5 -4
@@ -80,7 +80,8 @@ static inline bool bio_no_advance_iter(const struct bio *bio) {return bio_op (bio) ==REQ_OP_DISCARD ||bio_op (bio) ==REQ_OP_SECURE_ERASE || -bio_op (bio) ==REQ_OP_WRITE_ZEROES ; +bio_op (bio) ==REQ_OP_WRITE_ZEROES || +op_is_dmabuf (bio->bi_opf); }static inline void *bio_data (struct bio *bio)
@@ -438,12 +439,12 @@ static inline void bio_wouldblock_error(struct bio *bio)/* * Calculate number of bvec segments that should be allocated to fit data -* pointed by @iter. If @iter is backed by bvec it's going to be reused -* instead of allocating a new one. +* pointed by @iter. If @iter is backed by a bvec or a dmabuf, the bvec array / +* the dma map are going to be reused, and so no extra allocation is required. */ static inline int bio_iov_vecs_to_alloc (struct iov_iter *iter,int max_segs) { -if (iov_iter_is_bvec (iter)) +if (iov_iter_is_bvec (iter) ||iov_iter_is_dmabuf_map (iter))return 0 ;return iov_iter_npages (iter, max_segs); }
include/linux/blk-mq.h +7 -0
@@ -1017,6 +1017,13 @@ static inline void *blk_mq_rq_to_pdu(struct request *rq)return rq +1 ; } +static inline bool blk_mq_rq_is_dmabuf (struct request *rq) +{ +if (!IS_ENABLED (CONFIG_DMA_SHARED_BUFFER )) +return false; +return rq->bio &&op_is_dmabuf (rq->bio->bi_opf); +} +static inline struct blk_mq_hw_ctx *queue_hctx (struct request_queue *q,int id) {struct blk_mq_hw_ctx *hctx;
include/linux/blk_types.h +13 -1
@@ -233,7 +233,12 @@ struct bio {atomic_t __bi_remaining;/* The actual vec list, preserved by bio_reset() */ -struct bio_vec *bi_io_vec; +union { +struct bio_vec *bi_io_vec; +/* Driver specific dma map, valid IFF REQ_DMABUF is set */ +struct dma_buf_io_map *bi_dmabuf_map; + }; +struct bvec_iter bi_iter;union {
@@ -402,6 +407,7 @@ enum req_flag_bits { __REQ_DRV,/* for driver use */ __REQ_FS_PRIVATE,/* for file system (submitter) use */ __REQ_ATOMIC,/* for atomic write operations */ + __REQ_DMABUF, /* Using premmaped dma buffers *//* * Command specific flags, keep last: */
@@ -434,6 +440,7 @@ enum req_flag_bits {#define REQ_DRV (__force blk_opf_t)(1ULL << __REQ_DRV)#define REQ_FS_PRIVATE (__force blk_opf_t)(1ULL << __REQ_FS_PRIVATE)#define REQ_ATOMIC (__force blk_opf_t)(1ULL << __REQ_ATOMIC) +#define REQ_DMABUF (__force blk_opf_t)(1ULL << __REQ_DMABUF)#define REQ_NOUNMAP (__force blk_opf_t)(1ULL << __REQ_NOUNMAP)
@@ -487,6 +494,11 @@ static inline bool op_is_discard(blk_opf_t op)return (op &REQ_OP_MASK ) ==REQ_OP_DISCARD ; } +static inline bool op_is_dmabuf (blk_opf_t op) +{ +return op &REQ_DMABUF ; +} +/* * Check if a bio or request operation is a zone management operation. */
include/linux/bvec.h +2 -1
@@ -108,7 +108,8 @@ struct bvec_iter {unsigned int bi_idx;/* -* Current offset in the bvec entry pointed to by `bi_idx`. +* Current offset in the bvec entry pointed to by `bi_idx` or into +* a dma-buf map. */ unsigned int bi_offset; } __packed__aligned(4) ;
Signature · 1 line
2.54.0
[email protected]
AI reviewRe: [PATCH v9 04/13] block: introduce dma map backed bio type
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [High] block: type confusion in blk_recalc_rq_segments() for REQ_DMABUF bios --
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] block: type confusion in blk_recalc_rq_segments() for REQ_DMABUF bios --
commit e3ead2f3d194e7e61660e2723f7786a4b6fec3ba
Author: Pavel Begunkov <[email protected]>
block: introduce dma map backed bio type
This patch introduces a dma map backed bio type. It repurposes the bi_io_vec space in the bio structure to store dmabuf maps, as premapped buffers do not require a generic bio_vec array.
Quoted 5 lines
include/linux/blk_types.h +0
-0
include/linux/blk_types.h struct bio { · line 241
Quoted 10 lines
include/linux/blk_types.h +6
-1@@ -233,7 +233,12 @@ struct bio {atomic_t __bi_remaining;/* The actual vec list, preserved by bio_reset() */ -struct bio_vec *bi_io_vec; +union { +struct bio_vec *bi_io_vec; +/* Driver specific dma map, valid IFF REQ_DMABUF is set */ +struct dma_buf_io_map *bi_dmabuf_map; + }; +
[Severity: High]
Does this memory alias cause a type confusion in existing block layer functions that don't check for REQ_DMABUF?
For example, if a REQ_DMABUF request completes partially, blk_update_request() will call blk_recalc_rq_segments(). That function iterates over the request using rq_for_each_bvec(), which internally uses bio_for_each_segment() and blindly dereferences bi_io_vec as a bio_vec array because bio_has_data() evaluates to true for these bios.
This means it reads the memory of struct dma_buf_io_map as bv_page, bv_len, and bv_offset. When the garbage bv_len is passed to bio_advance_iter_single(), it subtracts it from iter->bi_size, which can lead to an infinite loop or wild memory accesses when attempting to process the segments.
Is there a need to update blk_recalc_rq_segments() or other iterators to explicitly exclude REQ_DMABUF bios?
Quoted 3 lines
include/linux/blk_types.h +0
-0struct bvec_iter bi_iter;union {
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=4
PATCH v9 5/13 block: add dma-buf support for raw bdev
In reply to Pavel BegunkovAdd a simple proxy implementation of init_dma_buf_io_ctx() forwarding the call to a new struct block_device_operations operation. Also reject dma-buf backed iterators for buffered IO.
--- block/fops.c | 22 ++++++++++++++++++++++ include/linux/blkdev.h | 2 ++ 2 files changed, 24 insertions(+)
block/fops.c +22 -0
@@ -730,6 +730,9 @@ blkdev_direct_write(struct kiocb *iocb, struct iov_iter *from)static ssize_t blkdev_buffered_write (struct kiocb *iocb,struct iov_iter *from) { +if (unlikely (iov_iter_is_dmabuf_map (from))) +return -EOPNOTSUPP ; +return iomap_file_buffered_write (iocb, from, &blkdev_iomap_ops,NULL ,NULL ); }
@@ -855,6 +858,11 @@ static ssize_t blkdev_read_iter(struct kiocb *iocb, struct iov_iter *to)if (ret <0 || !count) goto reexpand; } +if (unlikely (iov_iter_is_dmabuf_map (to))) { +if (!ret) + ret = -EOPNOTSUPP ; + goto reexpand; + }/* * Take i_rwsem and invalidate_lock to avoid racing with set_blocksize
@@ -958,6 +966,19 @@ static int blkdev_mmap_prepare(struct vm_area_desc *desc)return generic_file_mmap_prepare (desc); } +static int blkdev_init_dma_buf_io_ctx (struct file *file, +struct dma_buf_io_ctx *ctx) +{ +struct block_device *bdev =file_bdev (file); +struct gendisk *disk = bdev->bd_disk; + +if (!(file->f_flags &O_DIRECT )) +return -EINVAL ; +if (!disk->fops->init_dma_buf_io_ctx) +return -EOPNOTSUPP ; +return disk->fops->init_dma_buf_io_ctx (bdev, ctx); +} +const struct file_operations def_blk_fops = { .open = blkdev_open, .release = blkdev_release,
@@ -976,6 +997,7 @@ const struct file_operations def_blk_fops = { .fallocate = blkdev_fallocate, .uring_cmd = blkdev_uring_cmd, .fop_flags = FOP_BUFFER_RASYNC | FOP_DONTCACHE, + .init_dma_buf_io_ctx = blkdev_init_dma_buf_io_ctx, };static __init int blkdev_init (void )
include/linux/blkdev.h +2 -0
@@ -1599,6 +1599,8 @@ struct block_device_operations {/* returns the length of the identifier or a negative errno: */ int (*get_unique_id)(struct gendisk *disk,u8 id[16 ],enum blk_unique_id id_type); +int (*init_dma_buf_io_ctx)(struct block_device *, +struct dma_buf_io_ctx *);struct module *owner;const struct pr_ops *pr_ops;
Signature · 1 line
2.54.0
Re: [PATCH v9 05/13] block: add dma-buf support for raw bdev
Review
Quoted 10 lines · Pavel Begunkov
(quoted diff) +10
-0+static int blkdev_init_dma_buf_io_ctx(struct file *file, + struct dma_buf_io_ctx *ctx) +{ + struct block_device *bdev = file_bdev(file); + struct gendisk *disk = bdev->bd_disk; + + if (!(file->f_flags & O_DIRECT)) + return -EINVAL; + if (!disk->fops->init_dma_buf_io_ctx) + return -EOPNOTSUPP;
dma-buf doesn't work with PI: bi_dmabuf_map aliases bi_io_vec, and blk_integrity_iterate() walks it as bvecs to generate/verify the checksum. rw-dmabuf liburing test hangs on a PI enabled NVMe. Something like below is needed:
block/blk-mq.c +6 -0
@@ -3150,6 +3150,12 @@ void blk_mq_submit_bio(struct bio *bio) goto queue_exit; } +if (unlikely (op_is_dmabuf (bio->bi_opf) && +bdev_has_integrity_csum (bio->bi_bdev))) { +bio_endio_status (bio,BLK_STS_NOTSUPP ); + goto queue_exit; + } +if ((bio->bi_opf &REQ_POLLED ) && !blk_mq_can_poll (q)) {bio_endio_status (bio,BLK_STS_NOTSUPP ); goto queue_exit;
block/fops.c +2 -0
@@ -976,6 +976,8 @@ static int blkdev_init_dma_buf_io_ctx(struct file *file,return -EINVAL ;if (!disk->fops->init_dma_buf_io_ctx)return -EOPNOTSUPP ; +if (bdev_has_integrity_csum (bdev)) +return -EOPNOTSUPP ;return disk->fops->init_dma_buf_io_ctx (bdev, ctx); }
Jens Axboe
maintainerRe: [PATCH v9 05/13] block: add dma-buf support for raw bdev
Quoted 39 lines · Anuj Gupta/Anuj Gupta
Review
Quoted 10 lines · Pavel Begunkov
(quoted diff) +10
-0+static int blkdev_init_dma_buf_io_ctx(struct file *file, + struct dma_buf_io_ctx *ctx) +{ + struct block_device *bdev = file_bdev(file); + struct gendisk *disk = bdev->bd_disk; + + if (!(file->f_flags & O_DIRECT)) + return -EINVAL; + if (!disk->fops->init_dma_buf_io_ctx) + return -EOPNOTSUPP;dma-buf doesn't work with PI:
bi_dmabuf_mapaliasesbi_io_vec, andblk_integrity_iterate()walks it as bvecs to generate/verify the checksum. rw-dmabuf liburing test hangs on a PI enabled NVMe. Something like below is needed:block/blk-mq.c +6
-0@@ -3150,6 +3150,12 @@ void blk_mq_submit_bio(struct bio *bio) goto queue_exit; } +if (unlikely (op_is_dmabuf (bio->bi_opf) && +bdev_has_integrity_csum (bio->bi_bdev))) { +bio_endio_status (bio,BLK_STS_NOTSUPP ); + goto queue_exit; + } +if ((bio->bi_opf &REQ_POLLED ) && !blk_mq_can_poll (q)) {bio_endio_status (bio,BLK_STS_NOTSUPP ); goto queue_exit;block/fops.c +0
-0
block/fops.c static int blkdev_init_dma_buf_io_ctx(struct file *file, · line 982
Quoted 7 lines
block/fops.c +2
-0@@ -976,6 +976,8 @@ static int blkdev_init_dma_buf_io_ctx(struct file *file,return -EINVAL ;if (!disk->fops->init_dma_buf_io_ctx)return -EOPNOTSUPP ; +if (bdev_has_integrity_csum (bdev)) +return -EOPNOTSUPP ;return disk->fops->init_dma_buf_io_ctx (bdev, ctx); }
Can you send a separate patch? I've queued this series up for some testing.
Signature · 1 line
Jens Axboe
Re: [PATCH v9 05/13] block: add dma-buf support for raw bdev
In reply to Anuj Gupta/Anuj GuptaQuoted 16 lines · Anuj Gupta/Anuj Gupta
Review
Quoted 10 lines · Pavel Begunkov
(quoted diff) +10
-0+static int blkdev_init_dma_buf_io_ctx(struct file *file, + struct dma_buf_io_ctx *ctx) +{ + struct block_device *bdev = file_bdev(file); + struct gendisk *disk = bdev->bd_disk; + + if (!(file->f_flags & O_DIRECT)) + return -EINVAL; + if (!disk->fops->init_dma_buf_io_ctx) + return -EOPNOTSUPP;dma-buf doesn't work with PI:
bi_dmabuf_mapaliasesbi_io_vec, andblk_integrity_iterate()walks it as bvecs to generate/verify the checksum. rw-dmabuf liburing test hangs on a PI enabled NVMe. Something like below is needed:
Thanks, I already took it into v10! There is some some confusion with the branches, but will get incorporated in either case.
Signature · 1 line
Pavel Begunkov
PATCH v9 6/13 nvme-pci: rename nvme_pci_sgl_set_data to nvme_pci_dma_iter_set_sgl
In reply to Pavel BegunkovFrom: Anuj Gupta <[email protected]>
Rename the blk_dma_iter based nvme_pci_sgl_set_data() to nvme_pci_dma_iter_set_sgl().
--- drivers/nvme/host/pci.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)
drivers/nvme/host/pci.c +5 -5
@@ -1147,7 +1147,7 @@ static blk_status_t nvme_pci_setup_data_prp(struct request *req,return BLK_STS_IOERR ; } -static void nvme_pci_sgl_set_data (struct nvme_sgl_desc *sge, +static void nvme_pci_dma_iter_set_sgl (struct nvme_sgl_desc *sge,struct blk_dma_iter *iter) { sge->addr =cpu_to_le64 (iter->addr);
@@ -1177,7 +1177,7 @@ static blk_status_t nvme_pci_setup_data_sgl(struct request *req, iod->cmd.common.flags =NVME_CMD_SGL_METABUF ;if (entries ==1 ||blk_rq_dma_map_coalesce (&iod->dma_state)) { -nvme_pci_sgl_set_data (&iod->cmd.common.dptr.sgl, iter); +nvme_pci_dma_iter_set_sgl (&iod->cmd.common.dptr.sgl, iter); iod->total_len += iter->len;return BLK_STS_OK ; }
@@ -1199,7 +1199,7 @@ static blk_status_t nvme_pci_setup_data_sgl(struct request *req, iter->status =BLK_STS_IOERR ;break ; } -nvme_pci_sgl_set_data (&sg_list[mapped++], iter); +nvme_pci_dma_iter_set_sgl (&sg_list[mapped++], iter); iod->total_len += iter->len; }while (blk_rq_dma_map_iter_next (req, nvmeq->dev->dev, iter));
@@ -1359,13 +1359,13 @@ static blk_status_t nvme_pci_setup_meta_iter(struct request *req) iod->cmd.common.metadata =cpu_to_le64 (sgl_dma);if (entries ==1 ) { iod->meta_total_len = iter.len; -nvme_pci_sgl_set_data (sg_list, &iter); +nvme_pci_dma_iter_set_sgl (sg_list, &iter);return BLK_STS_OK ; } sgl_dma +=sizeof (*sg_list);do { -nvme_pci_sgl_set_data (&sg_list[++i], &iter); +nvme_pci_dma_iter_set_sgl (&sg_list[++i], &iter); iod->meta_total_len += iter.len; }while (blk_rq_integrity_dma_map_iter_next (req, dev->dev, &iter));
Signature · 1 line
2.54.0
PATCH v9 7/13 nvme-pci: implement dma-buf backed requests
In reply to Pavel BegunkovEnable BIO_DMABUF_MAP backed requests. On registration we map the dma-buf and store it as a prp list, which is then used to initialise requests. All attached contexts are stored in a new list dmabuf_ctxs, and additions/removals are synchronised with dmabuf_lock.
--- drivers/nvme/host/Makefile | 1 + drivers/nvme/host/core.c | 12 +++ drivers/nvme/host/nvme.h | 2 + drivers/nvme/host/pci-dmabuf.c | 164 +++++++++++++++++++++++++++++++++ drivers/nvme/host/pci-dmabuf.h | 73 +++++++++++++++ drivers/nvme/host/pci.c | 93 +++++++++++++++++++ 6 files changed, 345 insertions(+) create mode 100644 drivers/nvme/host/pci-dmabuf.c create mode 100644 drivers/nvme/host/pci-dmabuf.h
drivers/nvme/host/Makefile +1 -0
@@ -21,6 +21,7 @@ nvme-core-$(CONFIG_NVME_HWMON) += hwmon.o nvme-core-$(CONFIG_NVME_HOST_AUTH) += auth.o nvme-y += pci.o +nvme-$(CONFIG_DMA_SHARED_BUFFER) += pci-dmabuf.o nvme-fabrics-y += fabrics.o
drivers/nvme/host/core.c +12 -0
@@ -2708,6 +2708,17 @@ static int nvme_report_zones(struct gendisk *disk, sector_t sector,#define nvme_report_zones NULL#endif /* CONFIG_BLK_DEV_ZONED */ +static int nvme_init_dma_buf_io_ctx (struct block_device *bdev, +struct dma_buf_io_ctx *ctx) +{ +struct nvme_ns *ns = bdev->bd_disk->private_data; +struct nvme_ctrl *ctrl = ns->ctrl; + +if (!ctrl->ops->init_dma_buf_io_ctx) +return -EINVAL ; +return ctrl->ops->init_dma_buf_io_ctx (ctrl, ctx); +} +const struct block_device_operations nvme_bdev_ops = { .owner =THIS_MODULE , .ioctl = nvme_ioctl,
@@ -2718,6 +2729,7 @@ const struct block_device_operations nvme_bdev_ops = { .get_unique_id = nvme_get_unique_id, .report_zones = nvme_report_zones, .pr_ops = &nvme_pr_ops, + .init_dma_buf_io_ctx = nvme_init_dma_buf_io_ctx, }; static int nvme_wait_ready(struct nvme_ctrl *ctrl, u32 mask, u32 val,
drivers/nvme/host/nvme.h +2 -0
@@ -668,6 +668,8 @@ struct nvme_ctrl_ops {int (*get_address)(struct nvme_ctrl *ctrl,char *buf,int size);void (*print_device_info)(struct nvme_ctrl *ctrl);bool (*supports_pci_p2pdma)(struct nvme_ctrl *ctrl); +int (*init_dma_buf_io_ctx)(struct nvme_ctrl *ctrl, +struct dma_buf_io_ctx *ctx);unsigned long (*get_virt_boundary)(struct nvme_ctrl *ctrl,bool is_admin); };
drivers/nvme/host/pci-dmabuf.c new file +164 -0
164 lines not on this page · raw message
drivers/nvme/host/pci-dmabuf.h new file +73 -0
74 lines not on this page · raw message
drivers/nvme/host/pci.c +93 -0
158 lines not on this page · raw message
Signature · 1 line
2.54.0
[email protected]
AI reviewRe: [PATCH v9 07/13] nvme-pci: implement dma-buf backed requests
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [High] block: nvme: heap buffer overflow in nvme_dma_buf_io_map() due to missing bounds check --
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] block: nvme: heap buffer overflow in nvme_dma_buf_io_map() due to missing bounds check --
commit 6ab5bd531acb81cbd400d2dec375c79d1f832e58
Author: Pavel Begunkov <[email protected]>
nvme-pci: implement dma-buf backed requests
This patch enables BIO_DMABUF_MAP backed requests. On registration, it maps the dma-buf and stores it as a prp list used to initialize requests. All attached contexts are stored in a new list dmabuf_ctxs, and synchronized with dmabuf_lock.
Quoted 5 lines
drivers/nvme/host/pci-dmabuf.c new file +0
-0
[ ... ]
Quoted 29 lines
drivers/nvme/host/pci-dmabuf.c new file +29
-0+static struct dma_buf_io_map *nvme_dma_buf_io_map (struct dma_buf_io_ctx *ctx) +{ +unsigned nr_entries = ctx->dmabuf->size /NVME_CTRL_PAGE_SIZE ; +struct nvme_dma_buf_io_ctx *nvme_ctx = ctx->dev_priv; +struct dma_buf_attachment *attach = nvme_ctx->attach; +unsigned long tmp, i =0 ; +struct nvme_dmabuf_map *map; +struct scatterlist *sg; +struct sg_table *sgt; +int ret; + +dma_resv_assert_held (ctx->dmabuf->resv); + +if (ctx->dmabuf->size %NVME_CTRL_PAGE_SIZE ) +return ERR_PTR (-EINVAL ); + + map =kvmalloc_flex (*map, dma_list, nr_entries); +if (!map) +return ERR_PTR (-ENOMEM ); + map->nr_entries = nr_entries; + + sgt =dma_buf_map_attachment (attach, ctx->dir); +if (IS_ERR (sgt)) { + ret =PTR_ERR (sgt); + sgt =NULL ; + goto err; + } + +for_each_sgtable_dma_sg (sgt, sg, tmp) {
drivers/nvme/host/pci-dmabuf.c line 60
Quoted 10 lines
drivers/nvme/host/pci-dmabuf.c new file +10
-0+dma_addr_t dma_addr =sg_dma_address (sg); +unsigned long sg_len =sg_dma_len (sg); + +if ((sg_len %NVME_CTRL_PAGE_SIZE ) || + (dma_addr %NVME_CTRL_PAGE_SIZE )) { + ret = -EINVAL ; + goto err; + } +while (sg_len) { + map->dma_list[i++] = dma_addr;
[Severity: High]
Does this code overflow the map->dma_list[] flex array if the total mapped length exceeds the calculated ctx->dmabuf->size?
nvme_dma_buf_io_map() calculates the number of flex array entries (nr_entries) based strictly on ctx->dmabuf->size. However, the loop that populates map->dma_list is bounded by the total sg_dma_len(sg) across all entries in the scatterlist returned by dma_buf_map_attachment().
The DMA mapping API explicitly allows the mapped DMA length (sg_dma_len) to be larger than the original buffer size. For instance, this can occur when an IOMMU coalesces entries or aligns sizes to a larger IOMMU page size. A buggy exporter could also supply a larger-than-expected scatterlist.
Without bounds-checking i against nr_entries, writing to map->dma_list[i++] could result in a heap buffer overflow.
Quoted 4 lines
drivers/nvme/host/pci-dmabuf.c new file +4
-0+ dma_addr +=NVME_CTRL_PAGE_SIZE ; + sg_len -=NVME_CTRL_PAGE_SIZE ; + } + }
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=7
PATCH v9 8/13 nvme-pci: add SGL support for the dmabuf path
In reply to Pavel BegunkovFrom: Anuj Gupta <[email protected]>
Add SGL support in addition to PRP for dmabuf-backed requests, building the descriptor list from the mapping's sg_table.
--- drivers/nvme/host/pci.c | 150 ++++++++++++++++++++++++++++++++++++++-- 1 file changed, 146 insertions(+), 4 deletions(-)
drivers/nvme/host/pci.c +146 -4
@@ -1223,12 +1223,18 @@ static blk_status_t nvme_pci_setup_data_prp(struct request *req,return BLK_STS_IOERR ; } +static void nvme_pci_sgl_set_data (struct nvme_sgl_desc *sge, +dma_addr_t addr,u32 len) +{ + sge->addr =cpu_to_le64 (addr); + sge->length =cpu_to_le32 (len); + sge->type =NVME_SGL_FMT_DATA_DESC <<4 ; +} +static void nvme_pci_dma_iter_set_sgl (struct nvme_sgl_desc *sge,struct blk_dma_iter *iter) { - sge->addr =cpu_to_le64 (iter->addr); - sge->length =cpu_to_le32 (iter->len); - sge->type =NVME_SGL_FMT_DATA_DESC <<4 ; +nvme_pci_sgl_set_data (sge, iter->addr, iter->len); } static void nvme_pci_sgl_set_seg(struct nvme_sgl_desc *sge,
@@ -1239,6 +1245,142 @@ static void nvme_pci_sgl_set_seg(struct nvme_sgl_desc *sge, sge->type =NVME_SGL_FMT_LAST_SEG_DESC <<4 ; } +static unsigned int nvme_pci_dmabuf_sgl_nents (struct request *req) +{ +struct bio *bio = req->bio; +struct nvme_dmabuf_map *map =to_nvme_dmabuf_map (bio->bi_dmabuf_map); +struct scatterlist *sg; +unsigned long tmp; +size_t offset = bio->bi_iter.bi_offset; +size_t remaining =blk_rq_payload_bytes (req); +unsigned int nents =0 ; + +for_each_sgtable_dma_sg (map->sgt, sg, tmp) { +size_t sg_len =sg_dma_len (sg); + +if (!remaining) +break ; +if (offset >= sg_len) { + offset -= sg_len; +continue ; + } + + sg_len -= offset; + offset =0 ; + +do { +size_t chunk =min (remaining, sg_len); + + nents++; + sg_len -= chunk; + remaining -= chunk; + }while (sg_len && remaining); + } + +if (unlikely (remaining)) +return 0 ; + +return nents; +} + +static blk_status_t nvme_rq_setup_dmabuf_sgl (struct request *req, +struct nvme_queue *nvmeq,unsigned int entries) +{ +struct nvme_iod *iod =blk_mq_rq_to_pdu (req); +struct nvme_dmabuf_map *map =nvme_req_to_map (req); +size_t length =blk_rq_payload_bytes (req); +struct nvme_sgl_desc *sg_list = &iod->cmd.common.dptr.sgl; +size_t offset = req->bio->bi_iter.bi_offset; +bool pooled = entries >1 ; +dma_addr_t sgl_dma =0 ; +unsigned int mapped =0 ; +unsigned long tmp; +struct scatterlist *sg; +size_t remaining = length; + +if (!entries) +return BLK_STS_IOERR ; + + iod->cmd.common.flags =NVME_CMD_SGL_METABUF ; + iod->total_len = length; + +/* entries == 1 fits in the inline descriptor; more needs a pool. */ +if (pooled) { +if (entries <=NVME_SMALL_POOL_SIZE /sizeof (*sg_list)) + iod->flags |=IOD_SMALL_DESCRIPTOR ; + + sg_list =dma_pool_alloc (nvme_dma_pool (nvmeq, iod),GFP_ATOMIC , + &sgl_dma); +if (!sg_list) +return BLK_STS_RESOURCE ; + iod->descriptors[iod->nr_descriptors++] = sg_list; + } + +for_each_sgtable_dma_sg (map->sgt, sg, tmp) { +size_t sg_len =sg_dma_len (sg); +dma_addr_t addr =sg_dma_address (sg); + +if (!remaining) +break ; +if (offset >= sg_len) { + offset -= sg_len; +continue ; + } + + addr += offset; + sg_len -= offset; + offset =0 ; + +do { +u32 chunk =min_t (size_t, remaining, sg_len); + +if (WARN_ON_ONCE (mapped == entries)) + goto err_free; +nvme_pci_sgl_set_data (&sg_list[mapped++], addr, chunk); + + addr += chunk; + sg_len -= chunk; + remaining -= chunk; + }while (sg_len && remaining); + } + +if (unlikely (remaining)) + goto err_free; + +if (pooled) +nvme_pci_sgl_set_seg (&iod->cmd.common.dptr.sgl, sgl_dma, + mapped); +return BLK_STS_OK ; +err_free: +if (pooled) { + iod->nr_descriptors--; +dma_pool_free (nvme_dma_pool (nvmeq, iod), sg_list, sgl_dma); + } +return BLK_STS_IOERR ; +} + +static blk_status_t nvme_rq_setup_dmabuf (struct request *req, +struct nvme_queue *nvmeq,enum nvme_use_sgl use_sgl) +{ +unsigned int entries; +size_t avg_seg; + +if (use_sgl ==SGL_UNSUPPORTED ) +return nvme_rq_setup_dmabuf_map (req, nvmeq); + + entries =nvme_pci_dmabuf_sgl_nents (req); + +if (use_sgl ==SGL_FORCED ) +return nvme_rq_setup_dmabuf_sgl (req, nvmeq, entries); + + avg_seg = entries ? +DIV_ROUND_UP (blk_rq_payload_bytes (req), entries) :0 ; +if (sgl_threshold && avg_seg >= sgl_threshold) +return nvme_rq_setup_dmabuf_sgl (req, nvmeq, entries); + +return nvme_rq_setup_dmabuf_map (req, nvmeq); +} +static blk_status_t nvme_pci_setup_data_sgl (struct request *req,struct blk_dma_iter *iter) {
@@ -1334,7 +1476,7 @@ static blk_status_t nvme_map_data(struct request *req)blk_status_t ret;if (blk_mq_rq_is_dmabuf (req)) -return nvme_rq_setup_dmabuf_map (req, nvmeq); +return nvme_rq_setup_dmabuf (req, nvmeq, use_sgl);/* * Try to skip the DMA iterator for single segment requests, as that
Signature · 1 line
2.54.0
[email protected]
AI reviewRe: [PATCH v9 08/13] nvme-pci: add SGL support for the dmabuf path
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [Low] nvme: redundant loop in dmabuf SGL chunk calculation executes only once --
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] nvme: redundant loop in dmabuf SGL chunk calculation executes only once --
commit 5d24f863d7ba89114279230e25c2589da96688b4
Author: Anuj Gupta <[email protected]>
nvme-pci: add SGL support for the dmabuf path
Add SGL support in addition to PRP for dmabuf-backed requests, building the descriptor list from the mapping's sg_table.
Quoted 3 lines
drivers/nvme/host/pci.c +0
-0
[ ... ]
Quoted 2 lines
drivers/nvme/host/pci.c +2
-0+static unsigned int nvme_pci_dmabuf_sgl_nents (struct request *req) +{
[ ... ]
drivers/nvme/host/pci.c static void nvme_pci_sgl_set_seg(struct nvme_sgl_desc *sge, · line 1280
Quoted 10 lines
drivers/nvme/host/pci.c +10
-0+do { +size_t chunk =min (remaining, sg_len); + + nents++; + sg_len -= chunk; + remaining -= chunk; + }while (sg_len && remaining); + } + +if (unlikely (remaining))
[Severity: Low]
Since chunk is the minimum of remaining and sg_len, subtracting it from both guarantees that at least one of them will become zero on the first iteration.
Does this mean the while (sg_len && remaining) condition is always false immediately, making the loop execute exactly once?
Is there a missing SGL segment length cap that was intended here?
[ ... ]
Quoted 3 lines
drivers/nvme/host/pci.c +3
-0+static blk_status_t nvme_rq_setup_dmabuf_sgl (struct request *req, +struct nvme_queue *nvmeq,unsigned int entries) +{
[ ... ]
drivers/nvme/host/pci.c static void nvme_pci_sgl_set_seg(struct nvme_sgl_desc *sge, · line 1345
Quoted 12 lines
drivers/nvme/host/pci.c +12
-0+do { +u32 chunk =min_t (size_t, remaining, sg_len); + +if (WARN_ON_ONCE (mapped == entries)) + goto err_free; +nvme_pci_sgl_set_data (&sg_list[mapped++], addr, chunk); + + addr += chunk; + sg_len -= chunk; + remaining -= chunk; + }while (sg_len && remaining); + }
[Severity: Low]
This mapping loop shares the same logic as nvme_pci_dmabuf_sgl_nents().
Since chunk will fully consume either remaining or sg_len, the loop condition (sg_len && remaining) will evaluate to false after the first iteration.
Could this logic be simplified, or was a chunk size limit omitted?
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=8
PATCH v9 9/13 io_uring/rsrc: introduce buf registration structure
In reply to Pavel BegunkovIn preparation to following changes, instead of passing an iovec for buffer registration introduce a new structure. It'll be moved to uapi later, but for now it's initialised early from a user provided iovec.
--- io_uring/rsrc.c | 47 ++++++++++++++++++++++++++++++++--------------- 1 file changed, 32 insertions(+), 15 deletions(-)
io_uring/rsrc.c +32 -15
@@ -27,8 +27,13 @@ struct io_rsrc_update {u32 offset; }; +struct io_uring_regbuf_desc { +__u64 uaddr; +__u64 size; +}; +static struct io_rsrc_node *io_sqe_buffer_register (struct io_ring_ctx *ctx, -struct iovec *iov); +struct io_uring_regbuf_desc *desc);static int hpage_acct_ref (struct io_ring_ctx *ctx,struct page *hpage,bool *acct_new)
@@ -81,6 +86,15 @@ static bool hpage_acct_unref(struct io_ring_ctx *ctx, struct page *hpage)#define IO_CACHED_BVECS_SEGS 32 +static void io_iov_to_regbuf_desc (const struct iovec *iov, +struct io_uring_regbuf_desc *desc) +{ + *desc = (struct io_uring_regbuf_desc ) { + .uaddr = (u64 )(uintptr_t )iov->iov_base, + .size = iov->iov_len, + }; +} +int __io_account_mem (struct user_struct *user,unsigned long nr_pages) {unsigned long page_limit, cur_pages, new_pages;
@@ -398,6 +412,7 @@ static int __io_sqe_buffers_update(struct io_ring_ctx *ctx,return -EINVAL ;for (done =0 ; done < nr_args; done++) { +struct io_uring_regbuf_desc desc;struct io_rsrc_node *node;u64 tag =0 ;
@@ -411,7 +426,9 @@ static int __io_sqe_buffers_update(struct io_ring_ctx *ctx, err = -EFAULT ;break ; } - node =io_sqe_buffer_register (ctx, iov); + +io_iov_to_regbuf_desc (iov, &desc); + node =io_sqe_buffer_register (ctx, &desc);if (IS_ERR (node)) { err =PTR_ERR (node);break ;
@@ -870,26 +887,26 @@ bool io_check_coalesce_buffer(struct page **page_array, int nr_pages, }static struct io_rsrc_node *io_sqe_buffer_register (struct io_ring_ctx *ctx, -struct iovec *iov) +struct io_uring_regbuf_desc *desc) { +unsigned long uaddr = (unsigned long )desc->uaddr; +size_t size = desc->size;struct io_mapped_ubuf *imu =NULL ;struct page **pages =NULL ;struct io_rsrc_node *node;unsigned long off; -size_t size;int ret, nr_pages, i;struct io_imu_folio_data data;bool coalesced = false; -if (!iov->iov_base) { -if (iov->iov_len) +if (!uaddr) { +if (size)return ERR_PTR (-EFAULT );/* remove the buffer without installing a new one */ return NULL ; } - ret =io_validate_user_buf_range ((unsigned long )iov->iov_base, - iov->iov_len); + ret =io_validate_user_buf_range (uaddr, size);if (ret)return ERR_PTR (ret);
@@ -898,8 +915,7 @@ static struct io_rsrc_node *io_sqe_buffer_register(struct io_ring_ctx *ctx,return ERR_PTR (-ENOMEM ); ret = -ENOMEM ; - pages =io_pin_pages ((unsigned long ) iov->iov_base, iov->iov_len, - &nr_pages); + pages =io_pin_pages (uaddr, size, &nr_pages);if (IS_ERR (pages)) { ret =PTR_ERR (pages); pages =NULL ;
@@ -921,10 +937,9 @@ static struct io_rsrc_node *io_sqe_buffer_register(struct io_ring_ctx *ctx,if (ret) goto done; - size = iov->iov_len;/* store original address for later verification */ - imu->ubuf = (unsigned long ) iov->iov_base; - imu->len = iov->iov_len; + imu->ubuf = uaddr; + imu->len = size; imu->folio_shift =PAGE_SHIFT ; imu->release = io_release_ubuf; imu->priv = imu;
@@ -934,7 +949,7 @@ static struct io_rsrc_node *io_sqe_buffer_register(struct io_ring_ctx *ctx, imu->folio_shift = data.folio_shift;refcount_set (&imu->refs,1 ); - off = (unsigned long )iov->iov_base & ~PAGE_MASK ; + off = uaddr & ~PAGE_MASK ;if (coalesced) off += data.first_folio_page_idx <<PAGE_SHIFT ;
@@ -986,6 +1001,7 @@ int io_sqe_buffers_register(struct io_ring_ctx *ctx, void __user *arg,memset (iov,0 ,sizeof (*iov));for (i =0 ; i < nr_args; i++) { +struct io_uring_regbuf_desc desc;struct io_rsrc_node *node;u64 tag =0 ;
@@ -1009,7 +1025,8 @@ int io_sqe_buffers_register(struct io_ring_ctx *ctx, void __user *arg, } } - node =io_sqe_buffer_register (ctx, iov); +io_iov_to_regbuf_desc (iov, &desc); + node =io_sqe_buffer_register (ctx, &desc);if (IS_ERR (node)) { ret =PTR_ERR (node);break ;
Signature · 1 line
2.54.0
PATCH v9 10/13 io_uring/rsrc: extend buffer update
In reply to Pavel BegunkovWe need to pass more information to buffer registration than we can fit into a single struct iovec. This patch allows users to optionally pass struct io_uring_regbuf_desc. Apart from having more space for future use cases, it also introduces registration types.
Currently, the type can be either of IO_REGBUF_TYPE_UADDR, which mirrors the iovec path, or IO_REGBUF_TYPE_EMPTY for leaving a buffer table slot empty. The next patch introduces a dmabuf backed type, and can be useful for other extensions like splicing a list of user addresses (i.e. iovec[]), interoperability with zcrx, kernel allocated memory like was brough up by Cristoph. Note, the type only represents a registration option, which is distinct from how io_uring internally stores it.
The flags field is not used yet but always useful to have, e.g. we can encode read-only / write-only restrictions using it.
--- include/uapi/linux/io_uring.h | 27 +++++++++++++- io_uring/rsrc.c | 69 ++++++++++++++++++++++------------- 2 files changed, 69 insertions(+), 27 deletions(-)
include/uapi/linux/io_uring.h +26 -1
@@ -791,13 +791,38 @@ struct io_uring_rsrc_update {struct io_uring_rsrc_update2 {__u32 offset; -__u32 resv; +__u32 flags;__aligned_u64 data;__aligned_u64 tags;__u32 nr;__u32 resv2; }; +/* struct io_uring_rsrc_update2::flags */ +enum io_uring_rsrc_reg_flags { +/* +* Use the extended descriptor format for buffer updates, +* see struct io_uring_regbuf_desc +*/ +IORING_RSRC_UPDATE_EXTENDED =1U <<1 , +}; + +/* Buffer registration type, passed in struct io_uring_regbuf_desc::type */ +enum io_uring_regbuf_type { +IO_REGBUF_TYPE_EMPTY , +IO_REGBUF_TYPE_UADDR , + + __IO_REGBUF_TYPE_MAX, +}; + +struct io_uring_regbuf_desc { +__u32 type;/* enum io_uring_regbuf_type */ +__u32 flags; +__u64 size; +__u64 uaddr; +__u64 __resv[7 ]; +}; +/* Skip updating fd indexes set to this value in the fd table */ #define IORING_REGISTER_FILES_SKIP (-2)
io_uring/rsrc.c +43 -26
151 lines not on this page · raw message
Signature · 1 line
2.54.0
[email protected]
AI reviewRe: [PATCH v9 10/13] io_uring/rsrc: extend buffer update
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [Medium] io_uring: silent 32-bit truncation of __u64 uaddr and size in io_sqe_buffer_register --
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] io_uring: silent 32-bit truncation of __u64 uaddr and size in io_sqe_buffer_register --
commit a910d9c1c59f845e8a642099135fc16862f3fe7d
Author: Pavel Begunkov <[email protected]>
io_uring/rsrc: extend buffer update
We need to pass more information to buffer registration than we can fit into a single struct iovec. This patch allows users to optionally pass struct io_uring_regbuf_desc, introducing an extended registration format with a new flags field and types.
Quoted 4 lines
io_uring/rsrc.c +0
-0
[ ... ]
io_uring/rsrc.c static struct io_rsrc_node *io_sqe_buffer_register(struct io_ring_ctx *ctx, · line 903
Quoted 13 lines
io_uring/rsrc.c +7
-2@@ -882,8 +894,13 @@ static struct io_rsrc_node *io_sqe_buffer_register(struct io_ring_ctx *ctx,struct io_imu_folio_data data;bool coalesced = false; -if (!uaddr) { -if (size) +if (desc->type >= __IO_REGBUF_TYPE_MAX) +return ERR_PTR (-EINVAL ); +if (!mem_is_zero (&desc->__resv,sizeof (desc->__resv)) || desc->flags) +return ERR_PTR (-EINVAL ); + +if (desc->type ==IO_REGBUF_TYPE_EMPTY ) { +if (uaddr || size)
[Severity: Medium]
Does this code silently truncate 64-bit user input on 32-bit platforms?
The new struct io_uring_regbuf_desc defines uaddr and size as __u64:
struct io_uring_regbuf_desc {
...
__u64 size; __u64 uaddr; };
At the start of io_sqe_buffer_register(), these fields are immediately cast to unsigned long and size_t:
static struct io_rsrc_node *io_sqe_buffer_register(struct io_ring_ctx *ctx,
struct io_uring_regbuf_desc *desc)
{
unsigned long uaddr = (unsigned long)desc->uaddr;
size_t size = desc->size;On 32-bit platforms, this drops the upper 32 bits before any validation occurs.
If userspace passes 0x100000000 alongside IO_REGBUF_TYPE_EMPTY, the truncated uaddr and size evaluate to 0.
Would this allow an invalid 64-bit address to pass the emptiness check and incorrectly bypass the -EFAULT error path?
Quoted 3 lines
io_uring/rsrc.c +0
-0return ERR_PTR (-EFAULT );/* remove the buffer without installing a new one */ return NULL ;
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=10
PATCH v9 11/13 io_uring/rsrc: add uncloneable regbuf flag
In reply to Pavel BegunkovIt's hard to implement cloning if the internal structure needs to be mutable and/or relies on other ring resources. In preparation to such buffer types, add a flag indicating that the buffer can't be cloned. It might be possible to add cloning in the future for them, but that would likely need reallocating the structure and reacquiring resources in case by case manner.
--- io_uring/rsrc.c | 5 +++++ io_uring/rsrc.h | 3 ++- 2 files changed, 7 insertions(+), 1 deletion(-)
io_uring/rsrc.c +5 -0
@@ -1456,6 +1456,11 @@ static int io_clone_buffers(struct io_ring_ctx *ctx, struct io_ring_ctx *src_ctxif (!src_node) { dst_node =NULL ; }else { +if (src_node->buf->flags &IO_REGBUF_F_UNCLONEABLE ) { +io_rsrc_data_free (ctx, &data); +return -EOPNOTSUPP ; + } + dst_node =io_rsrc_node_alloc (ctx,IORING_RSRC_BUFFER );if (!dst_node) {io_rsrc_data_free (ctx, &data);
io_uring/rsrc.h +2 -1
@@ -26,7 +26,8 @@ struct io_rsrc_node { };enum { -IO_REGBUF_F_KBUF =1 , +IO_REGBUF_F_KBUF =1 <<0 , +IO_REGBUF_F_UNCLONEABLE =1 <<1 , };struct io_mapped_ubuf {
Signature · 1 line
2.54.0
PATCH v9 12/13 io_uring/rsrc: add regbuf import flags
In reply to Pavel BegunkovWe'll have special registered buffer types that can't be used with all opcodes and need special handling. Add separate flags to control registered buffer import, which will be used to specify what kind of buffers the caller can handle.
--- io_uring/rsrc.c | 8 ++++---- io_uring/rsrc.h | 24 ++++++++++++++++++++---- 2 files changed, 24 insertions(+), 8 deletions(-)
io_uring/rsrc.c +4 -4
@@ -1315,9 +1315,9 @@ inline struct io_rsrc_node *io_find_buf_node(struct io_kiocb *req,return NULL ; } -int io_import_reg_buf (struct io_kiocb *req,struct iov_iter *iter, +int __io_import_reg_buf (struct io_kiocb *req,struct iov_iter *iter,u64 buf_addr,size_t len,int ddir, -unsigned issue_flags) +unsigned issue_flags,unsigned import_flags) {struct io_rsrc_node *node;
@@ -1731,9 +1731,9 @@ static int io_kern_bvec_size(struct iovec *iov, unsigned nr_iovs,return 0 ; } -int io_import_reg_vec (int ddir,struct iov_iter *iter, +int __io_import_reg_vec (int ddir,struct iov_iter *iter,struct io_kiocb *req,struct iou_vec *vec, -unsigned nr_iovs,unsigned issue_flags) +unsigned nr_iovs,unsigned issue_flags,unsigned import_flags) {struct io_rsrc_node *node;struct io_mapped_ubuf *imu;
io_uring/rsrc.h +20 -4
@@ -62,12 +62,28 @@ int io_rsrc_data_alloc(struct io_rsrc_data *data, unsigned nr);struct io_rsrc_node *io_find_buf_node (struct io_kiocb *req,unsigned issue_flags); -int io_import_reg_buf (struct io_kiocb *req,struct iov_iter *iter, +int __io_import_reg_buf (struct io_kiocb *req,struct iov_iter *iter,u64 buf_addr,size_t len,int ddir, -unsigned issue_flags); -int io_import_reg_vec (int ddir,struct iov_iter *iter, +unsigned issue_flags,unsigned import_flags); +int __io_import_reg_vec (int ddir,struct iov_iter *iter,struct io_kiocb *req,struct iou_vec *vec, -unsigned nr_iovs,unsigned issue_flags); +unsigned nr_iovs,unsigned issue_flags, +unsigned import_flags); + +static inline int io_import_reg_buf (struct io_kiocb *req,struct iov_iter *iter, +u64 buf_addr,size_t len,int ddir, +unsigned issue_flags) +{ +return __io_import_reg_buf (req, iter, buf_addr, len, ddir, issue_flags,0 ); +} + +static inline int io_import_reg_vec (int ddir,struct iov_iter *iter, +struct io_kiocb *req,struct iou_vec *vec, +unsigned nr_iovs,unsigned issue_flags) +{ +return __io_import_reg_vec (ddir, iter, req, vec, nr_iovs, issue_flags,0 ); +} +int io_prep_reg_iovec (struct io_kiocb *req,struct iou_vec *iv,const struct iovec __user *uvec,size_t uvec_segs);
Signature · 1 line
2.54.0
PATCH v9 13/13 io_uring/rsrc: add dmabuf backed registered buffers
In reply to Pavel BegunkovImplement dmabuf backed registered buffers. To register them, the user should specify IO_REGBUF_TYPE_DMABUF for the regitration and pass the desired dmabuf fd and a file for which it should be registered.
From there, it can be used with io_uring read/write requests IORING_OP_{READ,WRITE}_FIXED) as normal. The requests should be issued against the file specified during registration, and otherwise they'll be failed. The user should also be prepared to handle spurious -EAGAIN by reissuing the request.
Internally, dmabuf registered buffers is an optin feature for io_uring request opcodes and they should pass a special flag on import to use it.
--- include/linux/io_uring_types.h | 5 +++ include/uapi/linux/io_uring.h | 6 ++- io_uring/Makefile | 1 + io_uring/dma-buf.c | 68 ++++++++++++++++++++++++++++++++++ io_uring/dma-buf.h | 67 +++++++++++++++++++++++++++++++++ io_uring/rsrc.c | 59 +++++++++++++++++++++++++++-- io_uring/rsrc.h | 5 +++ io_uring/rw.c | 33 ++++++++++++----- 8 files changed, 230 insertions(+), 14 deletions(-) create mode 100644 io_uring/dma-buf.c create mode 100644 io_uring/dma-buf.h
include/linux/io_uring_types.h +5 -0
@@ -11,6 +11,7 @@struct iou_loop_params ;struct io_uring_bpf_ops ; +struct dma_buf_io_map ;enum {/*
@@ -610,6 +611,7 @@ enum {REQ_F_IMPORT_BUFFER_BIT ,REQ_F_SQE_COPIED_BIT ,REQ_F_IOPOLL_BIT , +REQ_F_DMABUF_BIT ,/* not a real bit, just to check we're not overflowing the space */ __REQ_F_LAST_BIT,
@@ -705,6 +707,8 @@ enum {REQ_F_SQE_COPIED =IO_REQ_FLAG (REQ_F_SQE_COPIED_BIT ),/* request must be iopolled to completion (set in ->issue()) */ REQ_F_IOPOLL =IO_REQ_FLAG (REQ_F_IOPOLL_BIT ), +/* there is a dma map attached to request that needs to be dropped */ + REQ_F_DMABUF = IO_REQ_FLAG(REQ_F_DMABUF_BIT), }; struct io_tw_req {
@@ -827,6 +831,7 @@ struct io_kiocb {/* custom credentials, valid IFF REQ_F_CREDS is set */ const struct cred *creds;struct io_wq_work work; +struct dma_buf_io_map *dmabuf_map;struct io_big_cqe {u64 extra1;
include/uapi/linux/io_uring.h +5 -1
@@ -811,6 +811,7 @@ enum io_uring_rsrc_reg_flags {enum io_uring_regbuf_type {IO_REGBUF_TYPE_EMPTY ,IO_REGBUF_TYPE_UADDR , +IO_REGBUF_TYPE_DMABUF , __IO_REGBUF_TYPE_MAX, };
@@ -820,7 +821,10 @@ struct io_uring_regbuf_desc {__u32 flags;__u64 size;__u64 uaddr; -__u64 __resv[7 ]; + +__s32 dmabuf_fd; +__s32 target_fd; +__u64 __resv[6 ]; };/* Skip updating fd indexes set to this value in the fd table */
io_uring/Makefile +1 -0
@@ -26,3 +26,4 @@ obj-$(CONFIG_PROC_FS) += fdinfo.o obj-$(CONFIG_IO_URING_MOCK_FILE) += mock_file.o obj-$(CONFIG_IO_URING_BPF) += bpf_filter.o obj-$(CONFIG_IO_URING_BPF_OPS) += bpf-ops.o +obj-$(CONFIG_DMA_SHARED_BUFFER) += dma-buf.o
io_uring/dma-buf.c new file +68 -0
68 lines not on this page · raw message
io_uring/dma-buf.h new file +67 -0
67 lines not on this page · raw message
io_uring/rsrc.c +55 -4
108 lines not on this page · raw message
io_uring/rsrc.h +5 -0
@@ -28,6 +28,11 @@ struct io_rsrc_node {enum {IO_REGBUF_F_KBUF =1 <<0 ,IO_REGBUF_F_UNCLONEABLE =1 <<1 , +IO_REGBUF_F_DMABUF =1 <<3 , +}; + +enum { +IO_REGBUF_IMPORT_ALLOW_DMABUF =1 <<1 , };struct io_mapped_ubuf {
io_uring/rw.c +24 -9
@@ -23,6 +23,7 @@#include "rsrc.h" #include "poll.h" #include "rw.h" +#include "dma-buf.h" static void io_complete_rw (struct kiocb *kiocb,long res);static void io_complete_rw_iopoll (struct kiocb *kiocb,long res);
@@ -358,13 +359,18 @@ static int io_init_rw_fixed(struct io_kiocb *req, unsigned int issue_flags,struct io_async_rw *io = req->async_data;int ret; -if (io->bytes_done) -return 0 ; +if (!io->bytes_done) { + ret =__io_import_reg_buf (req, &io->iter, rw->addr, rw->len, ddir, + issue_flags,IO_REGBUF_IMPORT_ALLOW_DMABUF ); +if (ret) +return ret; +iov_iter_save_state (&io->iter, &io->iter_state); + } - ret =io_import_reg_buf (req, &io->iter, rw->addr, rw->len, ddir, - issue_flags); -iov_iter_save_state (&io->iter, &io->iter_state); -return ret; + ret =io_attach_dmabuf_map (req, &io->iter, issue_flags); +if (ret) +return ret; +return 0 ; }int io_prep_read_fixed (struct io_kiocb *req,const struct io_uring_sqe *sqe)
@@ -577,6 +583,8 @@ static void io_complete_rw(struct kiocb *kiocb, long res) struct io_rw *rw = container_of(kiocb, struct io_rw, kiocb);struct io_kiocb *req =cmd_to_io_kiocb (rw); +io_detach_dmabuf_map (req); +/* ring owner may block in freeze_super() before task_work runs */ if (kiocb->ki_flags &IOCB_WRITE )io_req_end_write (req);
@@ -698,7 +706,7 @@ static ssize_t loop_rw_iter(int ddir, struct io_rw *rw, struct iov_iter *iter) !(kiocb->ki_filp->f_flags & O_NONBLOCK))return -EAGAIN ;if ((req->flags &REQ_F_BUF_NODE ) && - (req->buf_node->buf->flags &IO_REGBUF_F_KBUF )) + (req->buf_node->buf->flags & (IO_REGBUF_F_KBUF |IO_REGBUF_F_DMABUF )))return -EFAULT ; ppos =io_kiocb_ppos (kiocb);
@@ -1226,7 +1234,10 @@ int io_read_fixed(struct io_kiocb *req, unsigned int issue_flags)if (unlikely (ret))return ret; -return io_read (req, issue_flags); + ret =io_read (req, issue_flags); +if (ret != -EIOCBQUEUED ) +io_detach_dmabuf_map (req); +return ret; }int io_write_fixed (struct io_kiocb *req,unsigned int issue_flags)
@@ -1237,7 +1248,11 @@ int io_write_fixed(struct io_kiocb *req, unsigned int issue_flags)if (unlikely (ret))return ret; -return io_write (req, issue_flags); + ret =io_write (req, issue_flags); +if (ret != -EIOCBQUEUED ) +io_detach_dmabuf_map (req); +return ret; + }void io_rw_fail (struct io_kiocb *req)
Signature · 1 line
2.54.0
[email protected]
AI reviewRe: [PATCH v9 13/13] io_uring/rsrc: add dmabuf backed registered buffers
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [High] io_uring/rw: dmabuf_map memory leak upon request reissue overwriting req->dmabuf_map - [High]…
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] io_uring/rw: dmabuf_map memory leak upon request reissue overwriting req->dmabuf_map
- [High] io_uring/rsrc: dmabuf_map leak via cloning dmabuf to IOPOLL ring bypassing restrictions --
commit 6cf565430e55d29c9af3badb73c4c8775d233005
Author: Pavel Begunkov <[email protected]>
io_uring/rsrc: add dmabuf backed registered buffers
Implement dmabuf backed registered buffers. To register them, the user should specify IO_REGBUF_TYPE_DMABUF for the regitration and pass the desired dmabuf fd and a file for which it should be registered.
Quoted 4 lines
io_uring/dma-buf.c new file +0
-0
[ ... ]
Quoted 4 lines
io_uring/dma-buf.c new file +4
-0+int io_register_dmabuf (struct io_ring_ctx *ctx, +struct io_uring_regbuf_desc *desc, +struct io_mapped_ubuf *imu) +{
[ ... ]
Quoted 2 lines
io_uring/dma-buf.c new file +2
-0+if (ctx->flags &IORING_SETUP_IOPOLL ) +return -EOPNOTSUPP ;
[ ... ]
io_uring/dma-buf.c line 59
Quoted 8 lines
io_uring/dma-buf.c new file +8
-0+ bd->target_file = target_file; + imu->len = dmabuf->size; + imu->release = io_release_dmabuf; + imu->priv = bd; + imu->flags =IO_REGBUF_F_DMABUF ; + imu->dir =IO_BUF_DEST |IO_BUF_SOURCE ; +dma_buf_put (dmabuf); +return 0 ;
[Severity: High]
Should IO_REGBUF_F_UNCLONEABLE be included in imu->flags here to prevent IOPOLL rings from cloning this buffer?
While io_register_dmabuf() explicitly rejects rings with IORING_SETUP_IOPOLL set during initial registration, omitting the uncloneable flag allows an IOPOLL ring to bypass this restriction by cloning the dmabuf via io_clone_buffers().
If the buffer is cloned to an IOPOLL ring, polled I/O requests will finish via io_complete_rw_iopoll(), which does not call io_detach_dmabuf_map(req) (unlike io_complete_rw). Does this result in a permanent leak of the dma_buf_io_map reference for any async polled request?
Quoted 4 lines
io_uring/dma-buf.h new file +0
-0
[ ... ]
Quoted 4 lines
io_uring/dma-buf.h new file +4
-0+static inline int io_attach_dmabuf_map (struct io_kiocb *req, +struct iov_iter *iter, +unsigned issue_flags) +{
[ ... ]
io_uring/dma-buf.h line 45
Quoted 8 lines
io_uring/dma-buf.h new file +8
-0+ bd = req->buf_node->buf->priv; + map =dma_buf_io_get_map (bd->ctx, nowait); +if (unlikely (IS_ERR (map))) +return PTR_ERR (map); + req->dmabuf_map = map; + iter->dmabuf_map = map; +return 0 ; +}
[Severity: High]
Could this overwrite and leak the previous req->dmabuf_map if the request is reissued?
When a read or write request encounters a reissue condition, it returns -EIOCBQUEUED. In io_read_fixed() and io_write_fixed(), this causes it to skip calling io_detach_dmabuf_map().
When the request is subsequently reissued, it executes io_init_rw_fixed() again, which unconditionally calls io_attach_dmabuf_map(). This fetches a new map and overwrites req->dmabuf_map without dropping the previous reference.
Would it make sense to check if req->dmabuf_map is already populated before requesting and assigning a new map, or to ensure it is detached before reassignment?
Quoted 4 lines
io_uring/rw.c +0
-0
[ ... ]
Quoted 23 lines
io_uring/rw.c +11
-6@@ -363,13 +364,18 @@ static int io_init_rw_fixed(struct io_kiocb *req, unsigned int issue_flags,struct io_async_rw *io = req->async_data;int ret; -if (io->bytes_done) -return 0 ; +if (!io->bytes_done) { + ret =__io_import_reg_buf (req, &io->iter, rw->addr, rw->len, ddir, + issue_flags,IO_REGBUF_IMPORT_ALLOW_DMABUF ); +if (ret) +return ret; +iov_iter_save_state (&io->iter, &io->iter_state); + } - ret =io_import_reg_buf (req, &io->iter, rw->addr, rw->len, ddir, - issue_flags); -iov_iter_save_state (&io->iter, &io->iter_state); -return ret; + ret =io_attach_dmabuf_map (req, &io->iter, issue_flags); +if (ret) +return ret; +return 0 ; }
[ ... ]
Quoted 10 lines
io_uring/rw.c +4
-1@@ -1231,7 +1239,10 @@ int io_read_fixed(struct io_kiocb *req, unsigned int issue_flags)if (unlikely (ret))return ret; -return io_read (req, issue_flags); + ret =io_read (req, issue_flags); +if (ret != -EIOCBQUEUED ) +io_detach_dmabuf_map (req); +return ret; }
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=13
Quoted 8 lines · Pavel Begunkov
The patch set allows to register a dmabuf to an
io_uringinstance for a specified file and use it withio_uringread / write requests. The infrastructure is not tied toio_uringand there could be more users in the future. A similar idea was attempted some years ago by Keith [1], from where I borrowed a number of changes. Later it was brough back up to life by Tushar and Vishal.[...]
Applied, thanks!
[01/13] dma-buf: introduce initial file I/O infrastructure
commit: b899f5d8a9594f3e5be7c481e4631521f0a6e42d
[02/13] iov_iter: add iterator type for dmabuf maps
commit: f41f84796a1129838524d2aa542598c22822a2ec
[03/13] block: always adjust bi_offset on bio_advance_iter
commit: dda48c57e1484f967727d827923d3fb47d6b8aa3
[04/13] block: introduce dma map backed bio type
commit: dfc9cc23122deeffcbf562a0989c695e8505d54e
[05/13] block: add dma-buf support for raw bdev
commit: 738881239a914b2d92cafdf1030c382f1614a293
[06/13] nvme-pci: rename nvme_pci_sgl_set_data to nvme_pci_dma_iter_set_sgl
commit: db93ee31780323044eb5d6279d89f6ae49493f39
[07/13] nvme-pci: implement dma-buf backed requests
commit: bb3e58f540e95ad55bc7ba6a6f713f79b4e79e7e
[08/13] nvme-pci: add SGL support for the dmabuf path
commit: 3676bad01d84421fbf8567edafac63bef9f648cc
[09/13] io_uring/rsrc: introduce buf registration structure
commit: fa1925e901c00b00695edd29d053564a6de63d9c
[10/13] io_uring/rsrc: extend buffer update
commit: b497df80ba7536cffb2c5364e725c0812135886c
[11/13] io_uring/rsrc: add uncloneable regbuf flag
commit: 10b6521fae14c5941b3fd77d1a48eb7a424ee487
[12/13] io_uring/rsrc: add regbuf import flags
commit: b5a1e426d22fa059eaac1106292ad60c8da52d17
[13/13] io_uring/rsrc: add dmabuf backed registered buffers
commit: 4bb797852328975051b001189f3e3840cfaad2d7Best regards,
Signature · 1 line
Jens Axboe
Jens Axboe
maintainerQuoted 7 lines · Pavel Begunkov
The patch set allows to register a dmabuf to an
io_uringinstance for a specified file and use it withio_uringread / write requests. The infrastructure is not tied toio_uringand there could be more users in the future. A similar idea was attempted some years ago by Keith [1], from where I borrowed a number of changes. Later it was brough back up to life by Tushar and Vishal.
Quoted 9 lines
It's an opt-in feature for files, and they need to implement a new file operation to use it. Only NVMe block devices are supported in this series. The user API is built on top of io_uring's "registered buffers", where a dmabuf is registered in a special way, but after it can be used as any other "registered buffer" with IORING_OP_{READ,WRITE}_FIXED requests. It's created via a new file operation and the resulted map is then passed through the I/O stack in a new iterator type. There is some additional infrastructure to glue it together, count requests, manage lifetime and implement invalidation.
I've queued this series up for some testing, in a separate branch. JFYI.
Signature · 1 line
Jens Axboe
Christoph Hellwig
maintainerQuoted 1 line · Jens Axboe
I've queued this series up for some testing, in a separate branch. JFYI.
More testing it obviously good. I've been waiting for addressing of the Sashiko comments on the nvme patches before doing more ACKs, and while I can't really judge the code those in the dma-buf code also look somewhat serious. So it would be useful to get them addressed and the series reposted. Same for folding in the PI vs dmabuf fix from Anuj, although making the combination work would of course be even better.
Quoted 11 lines · Christoph Hellwig
Quoted 1 line · Jens Axboe
I've queued this series up for some testing, in a separate branch. JFYI.
More testing it obviously good. I've been waiting for addressing of the Sashiko comments on the nvme patches before doing more ACKs, and while I can't really judge the code those in the dma-buf code also look somewhat serious. So it would be useful to get them addressed and the series reposted. Same for folding in the PI vs dmabuf fix from Anuj, although making the combination work would of course be even better.
Oops, I forgot to look at Sashiko's feedback when I acked the dma-buf patch. I think the two cleanup issues are valid, but the reclaim-related ones are not. I'm not sure why Sashiko is confused here.
I wonder if there are some shrinkers in the tree that incorrectly take dma-resv locks in a blocking manner. They should only use trylocks in shrinkers and direct reclaim paths.
Matt
Quoted 13 lines · Matthew Brost
Quoted 11 lines · Christoph Hellwig
Quoted 1 line · Jens Axboe
I've queued this series up for some testing, in a separate branch. JFYI.
More testing it obviously good. I've been waiting for addressing of the Sashiko comments on the nvme patches before doing more ACKs, and while I can't really judge the code those in the dma-buf code also look somewhat serious. So it would be useful to get them addressed and the series reposted. Same for folding in the PI vs dmabuf fix from Anuj, although making the combination work would of course be even better.
Quoted 2 lines
Oops, I forgot to look at Sashiko's feedback when I acked the dma-buf patch. I think the two cleanup issues are valid,
I stopped answering to sashiko as it repeats the same from version to version when false positive. Those two are warning paths, which should never happen, I'd rather leak than crash on double free or so.
but the reclaim-related
Quoted 1 line
ones are not.
Thanks for confirming, you already elaborated on one of them in previous revision, the new one looked to me as the same class of issue as well.
I'm not sure why Sashiko is confused here.
Quoted 4 lines
I wonder if there are some shrinkers in the tree that incorrectly take dma-resv locks in a blocking manner. They should only use trylocks in shrinkers and direct reclaim paths.
Signature · 1 line
Pavel Begunkov
Jens Axboe
maintainerQuoted 10 lines · Christoph Hellwig
Quoted 1 line · Jens Axboe
I've queued this series up for some testing, in a separate branch. JFYI.
More testing it obviously good. I've been waiting for addressing of the Sashiko comments on the nvme patches before doing more ACKs, and while I can't really judge the code those in the dma-buf code also look somewhat serious. So it would be useful to get them addressed and the series reposted. Same for folding in the PI vs dmabuf fix from Anuj, although making the combination work would of course be even better.
Agree on both of those (sashiko and folding the PI fix), but it's been forever and let's at least get some testing done. As mentioned, it's in a separate branch, easy to update. But if we were to have any hope of making 7.4 and also not missing yet another release, then it would basically have to happen asap.
Signature · 1 line
Jens Axboe
Quoted 5 lines · Christoph Hellwig
Quoted 1 line · Jens Axboe
I've queued this series up for some testing, in a separate branch. JFYI.
More testing it obviously good. I've been waiting for addressing of the Sashiko comments on the nvme patches before doing more ACKs,
The nvme array out-of-bounds one is sashiko getting confused, we check all sg chunks on alignment, and seems like sashiko ignores it.
Quoted 2 lines
and while I can't really judge the code those in the dma-buf code also look somewhat serious.
Should be false positives as well.
So it would be useful to get them addressed
Quoted 1 line
and the series reposted. Same for folding in the PI vs dmabuf fix
I was going to repost, I guess Jens staged it before. I'll send v10 anyway, should be less confusing this way.
Quoted 2 lines
from Anuj, although making the combination work would of course be even better.
Signature · 1 line
Pavel Begunkov