[PATCH 0/5] allow dma-buf-io ctx reuse across files

[PATCH 0/5] allow dma-buf-io ctx reuse across files

[Pavel Begunkov]

Currently, dma-buf maps can only be used by the file they were
registered with. This means that we can't open a new file, even if it
comes from the same subsystem and point to the same block device and
use it with an old dma-buf registration. That would be too restrictive
for upcoming filsystem support, so lift it now and allow reusing
contexts between files pointing to the same NVMe, even if they refer
to different partitions.

To do that, we add a tag representing for each file, save it from
the file passed during registration and use it to reject files
which tags don't match it. Tags are set by subsystems creating
files, should be unique and represent the instance of the
subsystem.

Pavel Begunkov (5):
  dma-buf-io: add tags for sharing ctx b/w files
  block: set dma-buf-io tag to block files
  io_uring: add data to dma-buf convertion helpers
  io_uring/dma-buf: switch to tag file filtering
  io_uring/dma-buf: get rid of struct io_buf_dma

 block/fops.c                 |  1 +
 drivers/dma-buf/dma-buf-io.c |  3 +++
 include/linux/dma-buf-io.h   | 22 ++++++++++++++++++++++
 include/linux/fs.h           |  6 ++++++
 io_uring/dma-buf.c           | 19 +++++--------------
 io_uring/dma-buf.h           | 24 ++++++++++++++----------
 6 files changed, 51 insertions(+), 24 deletions(-)

-- 
2.54.0

[PATCH 1/5] dma-buf-io: add tags for sharing ctx b/w files

[Pavel Begunkov]

Currently, it's only allowed to pass a ctx to the file it was registered
with, which is enforced on the io_uring side. Going forward we want to
support filesystems and not just block device files, and it'd be too
limiting.

Add a dma-buf-io tag to all files. We're going to save a tag of the file
passed during registration and later use to reject files which tags
don't match. A tag should be set for a file by the importer subsystem
implementing ->init_dma_buf_io_ctx on creation and should be unique for
this instance of the subsystem. For simplicity, we're going to use
pointers for which we're going to use pointers, e.g. gendisk for block
files and superblock for filesystems.

This patch adds a new field in struct file to keep a tag, but it can be
then be switched to a new f_op method.

Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>
---
 drivers/dma-buf/dma-buf-io.c |  3 +++
 include/linux/dma-buf-io.h   | 22 ++++++++++++++++++++++
 include/linux/fs.h           |  6 ++++++
 3 files changed, 31 insertions(+)

diff --git a/drivers/dma-buf/dma-buf-io.c b/drivers/dma-buf/dma-buf-io.c
index 8440151de860..60dc58fc0c23 100644
--- a/drivers/dma-buf/dma-buf-io.c
+++ b/drivers/dma-buf/dma-buf-io.c
@@ -166,12 +166,15 @@ int dma_buf_io_ctx_create(struct file *file,
 
 	if (!file->f_op->init_dma_buf_io_ctx)
 		return -EOPNOTSUPP;
+	if (WARN_ON(!file->f_dma_io_tag))
+		return -EFAULT;
 
 	ctx = kzalloc_obj(*ctx);
 	if (!ctx)
 		return -ENOMEM;
 	ctx->dir = dir;
 	ctx->dmabuf = dmabuf;
+	ctx->importer_tag = file->f_dma_io_tag;
 	get_dma_buf(dmabuf);
 
 	ret = file->f_op->init_dma_buf_io_ctx(file, ctx);
diff --git a/include/linux/dma-buf-io.h b/include/linux/dma-buf-io.h
index d9f27650a851..299edf0baaf3 100644
--- a/include/linux/dma-buf-io.h
+++ b/include/linux/dma-buf-io.h
@@ -48,10 +48,32 @@ struct dma_buf_io_ctx {
 	enum dma_data_direction			dir;
 	bool					is_dead;
 
+	/*
+	 * Tags represent an instance of the subsystem the dma-buf I/O
+	 * context is bound to, the set of underlying devices and subsystem
+	 * specific map / context structure representations. Remember the
+	 * registration file's tag (see struct file::f_dma_io_tag) and use
+	 * it to filter out files that don't share it.
+	 */
+	unsigned long				importer_tag;
+
 	void					*dev_priv;
 	const struct dma_buf_io_ops		*dev_ops;
 };
 
+/**
+ * dma_buf_io_file_compatible - Check compatibility b/w context and file.
+ * @ctx:	[in]	dma-buf I/O context
+ * @file:	[in]	file
+ *
+ * Returns true if the context can be used to issue I/O to the given file.
+ */
+static inline bool dma_buf_io_file_compatible(const struct dma_buf_io_ctx *ctx,
+					      const struct file *file)
+{
+	return file->f_dma_io_tag == ctx->importer_tag;
+}
+
 int dma_buf_io_ctx_create(struct file *file,
 			   struct dma_buf *dmabuf,
 			   enum dma_data_direction dir,
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 99caef43ff3c..a9bb63cf6f9d 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -1293,6 +1293,12 @@ struct file {
 		freeptr_t		f_freeptr;
 	};
 	file_ref_t			f_ref;
+
+	/*
+	 * A tag representing the underlying subsystem for dma-buf I/O.
+	 * See struct dma_buf_io_ctx::importer_tag.
+	 */
+	unsigned long			f_dma_io_tag;
 	/* --- cacheline 3 boundary (192 bytes) --- */
 } __randomize_layout
   __attribute__((aligned(4)));	/* lest something weird decides that 2 is OK */
-- 
2.54.0

Re: [PATCH 1/5] dma-buf-io: add tags for sharing ctx b/w files

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] fs: uninitialized or stale f_dma_io_tag in init_file()
- [Low] fs: missing kernel-doc entry for f_dma_io_tag in struct file
--

commit 4c6695df72954d7137a1d0c3d35307639c41ca7f
Author: Pavel Begunkov <asml.silence@gmail.com>

dma-buf-io: add tags for sharing ctx b/w files

This commit adds an importer_tag field to struct dma_buf_io_ctx and an
f_dma_io_tag field to struct file so that a dma-buf I/O context can be
shared across compatible files belonging to the same subsystem instance.

> diff --git a/include/linux/dma-buf-io.h b/include/linux/dma-buf-io.h
> index d9f27650a851e..299edf0baaf32 100644
> --- a/include/linux/dma-buf-io.h
> +++ b/include/linux/dma-buf-io.h
> @@ -48,10 +48,32 @@ struct dma_buf_io_ctx {
>  	enum dma_data_direction			dir;
>  	bool					is_dead;
>  
> +	/*
> +	 * Tags represent an instance of the subsystem the dma-buf I/O
> +	 * context is bound to, the set of underlying devices and subsystem
> +	 * specific map / context structure representations. Remember the
> +	 * registration file's tag (see struct file::f_dma_io_tag) and use
> +	 * it to filter out files that don't share it.
> +	 */
> +	unsigned long				importer_tag;
> +
>  	void					*dev_priv;
>  	const struct dma_buf_io_ops		*dev_ops;
>  };
>  
> +/**
> + * dma_buf_io_file_compatible - Check compatibility b/w context and file.
> + * @ctx:	[in]	dma-buf I/O context
> + * @file:	[in]	file
> + *
> + * Returns true if the context can be used to issue I/O to the given file.
> + */
> +static inline bool dma_buf_io_file_compatible(const struct dma_buf_io_ctx *ctx,
> +					      const struct file *file)
> +{
> +	return file->f_dma_io_tag == ctx->importer_tag;
> +}

[Severity: High]
Can file->f_dma_io_tag contain uninitialized or stale slab data here and in
dma_buf_io_ctx_create() in drivers/dma-buf/dma-buf-io.c?

In fs/file_table.c, alloc_empty_file(), alloc_empty_file_noaccount(), and
alloc_empty_backing_file() allocate struct file objects from filp_cache and
bfilp_cache via kmem_cache_alloc() without __GFP_ZERO. Because those caches
use SLAB_TYPESAFE_BY_RCU, init_file() explicitly initializes the fields of
struct file on allocation and reuse:

fs/file_table.c:init_file() {
	...
	f->f_iocb_flags = 0;
	f->f_pos	= 0;
	f->f_wb_err	= 0;
	f->f_sb_err	= 0;

	file_ref_init(&f->f_ref, 1);
	return 0;
}

Since init_file() does not zero f->f_dma_io_tag, if a tagged file is closed
and its slab object is reused for an unrelated file whose open path does not
set f_dma_io_tag, wouldn't the new file retain the previous file's
f_dma_io_tag and cause dma_buf_io_file_compatible() to report a match?

> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index 99caef43ff3c4..a9bb63cf6f9d8 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -1293,6 +1293,12 @@ struct file {
>  		freeptr_t		f_freeptr;
>  	};
>  	file_ref_t			f_ref;
> +
> +	/*
> +	 * A tag representing the underlying subsystem for dma-buf I/O.
> +	 * See struct dma_buf_io_ctx::importer_tag.
> +	 */
> +	unsigned long			f_dma_io_tag;

[Severity: Low]
struct file in include/linux/fs.h has a kernel-doc comment block above its
definition. Should an @f_dma_io_tag entry be added there as well so
scripts/kernel-doc does not warn about an undescribed struct member during
documentation or W=1 builds?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1793751001.git.asml.silence@gmail.com?part=1

[PATCH 2/5] block: set dma-buf-io tag to block files

[Pavel Begunkov]

We need to set dma-buf-io tag to be able to share contexts between
multiple files. Use gendisk pointer as a tag, this allows sharing
b/w different partitions of the same block device.

Fixes: 839231d19393 ("block: add dma-buf support for raw bdev")
Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>
---
 block/fops.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/block/fops.c b/block/fops.c
index 60b9c9b1cefe..6f013780b76b 100644
--- a/block/fops.c
+++ b/block/fops.c
@@ -687,6 +687,7 @@ static int blkdev_open(struct inode *inode, struct file *filp)
 	ret = bdev_open(bdev, mode, filp->private_data, NULL, filp);
 	if (ret)
 		blkdev_put_no_open(bdev);
+	filp->f_dma_io_tag = (unsigned long)bdev->bd_disk;
 	return ret;
 }
 
-- 
2.54.0

Re: [PATCH 2/5] block: set dma-buf-io tag to block files

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] block: use-after-free of bdev in blkdev_open() when bdev_open() fails
- [High] block: unpinned gendisk pointer in f_dma_io_tag allows tag ABA reuse
--

commit 8ffc425965529fcad78b514404dc0182c1ca1bfb
Author: Pavel Begunkov <asml.silence@gmail.com>

block: set dma-buf-io tag to block files

This commit sets filp->f_dma_io_tag to bdev->bd_disk in blkdev_open() so that
dma-buf-io contexts can be shared across files and partitions belonging to the
same block device.

> diff --git a/block/fops.c b/block/fops.c
> index e5c27cc35ca1f..2b10b835e99dc 100644
> --- a/block/fops.c
> +++ b/block/fops.c
> @@ -686,6 +686,7 @@ static int blkdev_open(struct inode *inode, struct file *filp)
>  	ret = bdev_open(bdev, mode, filp->private_data, NULL, filp);
>  	if (ret)
>  		blkdev_put_no_open(bdev);
> +	filp->f_dma_io_tag = (unsigned long)bdev->bd_disk;

[Severity: High]
Can this trigger a use-after-free of bdev when bdev_open() fails?

In blkdev_open(), blkdev_get_no_open() acquires a reference to
bdev->bd_device and drops the temporary blockdev_superblock inode reference.
If bdev_open() returns an error (for example, when racing with disk or
partition removal, or when bdev_disk_changed() drops partitions),
blkdev_put_no_open(bdev) calls put_device(&bdev->bd_device) and drops
blkdev_open()'s only reference to bdev:

block/fops.c:blkdev_open()
    bdev = blkdev_get_no_open(inode->i_rdev, true);
    ...
    ret = bdev_open(bdev, mode, filp->private_data, NULL, filp);
    if (ret)
        blkdev_put_no_open(bdev);
            put_device(&bdev->bd_device);
                disk_release() / part_release()
                    bdev_drop(bdev);
    filp->f_dma_io_tag = (unsigned long)bdev->bd_disk;

Dereferencing bdev->bd_disk unconditionally after blkdev_put_no_open(bdev)
reads from bdev after it may already have been freed. Should blkdev_open()
return early on error, or set f_dma_io_tag inside bdev_open() alongside the
other bdev_file fields?

[Severity: High]
Also, is it safe to use the raw bdev->bd_disk pointer as f_dma_io_tag without
pinning the gendisk for the lifetime of the dma_buf_io_ctx?

When a dmabuf is registered via __io_register_dmabuf(),
dma_buf_io_ctx_create() stores file->f_dma_io_tag in ctx->importer_tag and
calls into the block layer:

dma_buf_io_ctx_create()
    ctx->importer_tag = file->f_dma_io_tag;
    blkdev_init_dma_buf_io_ctx()
        nvme_init_dma_buf_io_ctx()
            __nvme_init_dma_buf_io_ctx()
                nvme_init_io_ctx_locked()

nvme_init_io_ctx_locked() only takes a reference to struct nvme_ctrl via
nvme_get_ctrl(ctrl); neither blkdev_init_dma_buf_io_ctx() nor
nvme_init_dma_buf_io_ctx() pins bdev->bd_disk or struct nvme_ns.

Once the registration file is closed (or dropped in __io_register_dmabuf()),
if the NVMe namespace is removed while the controller stays live (such as via
nvme_ns_remove()), del_gendisk() and put_disk() can free the struct gendisk
while ctx remains active with ctx->is_dead still false.

If a new struct gendisk is later allocated at the same address for a
different NVMe controller or a non-NVMe block device, opening that device
will assign the same address to filp->f_dma_io_tag. Subsequent I/O using the
registered dmabuf will pass dma_buf_io_file_compatible() and dispatch a
REQ_DMABUF bio carrying nvme_dmabuf_map to the unrelated block device, where
a non-NVMe driver will interpret bio->bi_io_vec (aliased with
bio->bi_dmabuf_map) as a bio_vec array. Could this cause cross-device DMA or
type confusion?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1793751001.git.asml.silence@gmail.com?part=2

[PATCH 3/5] io_uring: add data to dma-buf convertion helpers

[Pavel Begunkov]

struct io_buf_dma is stored as void * in registered buffers, and it's
casted in multiple places. It's inconvenient as void* casts can't be
type checked, so wrap all of them into helpers and keep casting
centralised. No functional changes.

Fixes: 62e4bf5b25d44 ("io_uring/rsrc: add dmabuf backed registered buffers")
Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>
---
 io_uring/dma-buf.c |  2 +-
 io_uring/dma-buf.h | 14 ++++++++++++--
 2 files changed, 13 insertions(+), 3 deletions(-)

diff --git a/io_uring/dma-buf.c b/io_uring/dma-buf.c
index 132e3d0d7023..6f1d21c0f558 100644
--- a/io_uring/dma-buf.c
+++ b/io_uring/dma-buf.c
@@ -2,7 +2,7 @@
 
 static inline void io_release_dmabuf(void *priv)
 {
-	struct io_buf_dma *bd = priv;
+	struct io_buf_dma *bd = io_priv_to_dmabuf(priv);
 
 	fput(bd->target_file);
 	dma_buf_io_ctx_release(bd->ctx);
diff --git a/io_uring/dma-buf.h b/io_uring/dma-buf.h
index 534c06b6879f..a97412ebbb3f 100644
--- a/io_uring/dma-buf.h
+++ b/io_uring/dma-buf.h
@@ -12,6 +12,16 @@ struct io_buf_dma {
 	struct file			*target_file;
 };
 
+static inline struct io_buf_dma *io_priv_to_dmabuf(void *data)
+{
+	return data;
+}
+
+static inline struct io_buf_dma *io_buf_to_dmabuf(struct io_mapped_ubuf *imu)
+{
+	return io_priv_to_dmabuf(imu->priv);
+}
+
 int __io_register_dmabuf(struct io_ring_ctx *ctx,
 			struct io_uring_regbuf_desc *desc,
 			struct io_mapped_ubuf *imu);
@@ -39,7 +49,7 @@ static inline int io_attach_dmabuf_map(struct io_kiocb *req,
 	if (!IS_ENABLED(CONFIG_DMA_SHARED_BUFFER))
 		return -EOPNOTSUPP;
 
-	bd = req->buf_node->buf->priv;
+	bd = io_buf_to_dmabuf(req->buf_node->buf);
 	map = dma_buf_io_get_map(bd->ctx, nowait);
 	if (unlikely(IS_ERR(map)))
 		return PTR_ERR(map);
@@ -53,7 +63,7 @@ static inline int io_import_dmabuf_map(struct io_kiocb *req,
 				struct io_mapped_ubuf *imu,
 				size_t len, size_t offset)
 {
-	struct io_buf_dma *bd = imu->priv;
+	struct io_buf_dma *bd = io_buf_to_dmabuf(imu);
 
 	if (!IS_ENABLED(CONFIG_DMA_SHARED_BUFFER))
 		return -EOPNOTSUPP;
-- 
2.54.0

[PATCH 4/5] io_uring/dma-buf: switch to tag file filtering

[Pavel Begunkov]

Instead of enforcing that a dma-buf-io ctx is used only with the
registered file, allow sharing using tags.

Fixes: 62e4bf5b25d44 ("io_uring/rsrc: add dmabuf backed registered buffers")
Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>
---
 io_uring/dma-buf.c | 3 +--
 io_uring/dma-buf.h | 6 ++----
 2 files changed, 3 insertions(+), 6 deletions(-)

diff --git a/io_uring/dma-buf.c b/io_uring/dma-buf.c
index 6f1d21c0f558..f25749f7da22 100644
--- a/io_uring/dma-buf.c
+++ b/io_uring/dma-buf.c
@@ -4,7 +4,6 @@ static inline void io_release_dmabuf(void *priv)
 {
 	struct io_buf_dma *bd = io_priv_to_dmabuf(priv);
 
-	fput(bd->target_file);
 	dma_buf_io_ctx_release(bd->ctx);
 	kfree(bd);
 }
@@ -52,13 +51,13 @@ int __io_register_dmabuf(struct io_ring_ctx *ctx,
 	if (ret)
 		goto err;
 
-	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);
+	fput(target_file);
 	return 0;
 err:
 	kfree(bd);
diff --git a/io_uring/dma-buf.h b/io_uring/dma-buf.h
index a97412ebbb3f..9bd7c8c64397 100644
--- a/io_uring/dma-buf.h
+++ b/io_uring/dma-buf.h
@@ -9,7 +9,6 @@
 
 struct io_buf_dma {
 	struct dma_buf_io_ctx		*ctx;
-	struct file			*target_file;
 };
 
 static inline struct io_buf_dma *io_priv_to_dmabuf(void *data)
@@ -63,11 +62,10 @@ static inline int io_import_dmabuf_map(struct io_kiocb *req,
 				struct io_mapped_ubuf *imu,
 				size_t len, size_t offset)
 {
-	struct io_buf_dma *bd = io_buf_to_dmabuf(imu);
-
 	if (!IS_ENABLED(CONFIG_DMA_SHARED_BUFFER))
 		return -EOPNOTSUPP;
-	if (req->file != bd->target_file)
+	if (unlikely(!dma_buf_io_file_compatible(io_buf_to_dmabuf(imu)->ctx,
+						 req->file)))
 		return -EBADF;
 	req->flags |= REQ_F_DMABUF;
 	iov_iter_dmabuf_map(iter, ddir, NULL, offset, len);
-- 
2.54.0

Re: [PATCH 4/5] io_uring/dma-buf: switch to tag file filtering

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Critical] io_uring/dma-buf: unpinned gendisk tag after fput() in __io_register_dmabuf()
- [Critical] io_uring/dma-buf: uninitialized f_dma_io_tag in init_file() allows stale match
- [High] block: use-after-free of bdev in blkdev_open() after blkdev_put_no_open()
--

commit f539898d6b6343a21d00edd1090b62db7611212c
Author: Pavel Begunkov <asml.silence@gmail.com>

io_uring/dma-buf: switch to tag file filtering

This commit switches io_uring registered dma-buf buffers from matching the
exact registered target_file pointer to checking file compatibility via
dma_buf_io_file_compatible(), and drops target_file immediately upon
registration.

> diff --git a/io_uring/dma-buf.c b/io_uring/dma-buf.c
> index 6f1d21c0f558f..f25749f7da22b 100644
> --- a/io_uring/dma-buf.c
> +++ b/io_uring/dma-buf.c
> @@ -4,7 +4,6 @@ static inline void io_release_dmabuf(void *priv)
>  {
>  	struct io_buf_dma *bd = io_priv_to_dmabuf(priv);
>  
> -	fput(bd->target_file);
>  	dma_buf_io_ctx_release(bd->ctx);
>  	kfree(bd);
>  }
> @@ -52,13 +51,13 @@ int __io_register_dmabuf(struct io_ring_ctx *ctx,
>  	if (ret)
>  		goto err;
>  
> -	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);
> +	fput(target_file);

[Severity: Critical]
Does dropping target_file in __io_register_dmabuf() instead of holding it
until io_release_dmabuf() leave the struct gendisk pointer stored in
ctx->importer_tag unpinned, allowing ABA tag aliasing if the disk is freed?

When a block device is opened, blkdev_open() sets filp->f_dma_io_tag to the
raw struct gendisk pointer:

block/fops.c:blkdev_open() {
	...
	filp->f_dma_io_tag = (unsigned long)bdev->bd_disk;
}

During registration, dma_buf_io_ctx_create() saves target_file->f_dma_io_tag
into ctx->importer_tag, while nvme_init_dma_buf_io_ctx() ->
nvme_init_io_ctx_locked() only calls nvme_get_ctrl(ctrl) without taking a
reference on target_file, bdev, struct gendisk, or struct nvme_ns:

drivers/nvme/host/pci-dmabuf.c:nvme_init_io_ctx_locked() {
	...
	nvme_get_ctrl(ctrl);
	list_add(&nvme_ctx->list, &buf_list->ctxs);
}

Previously, keeping bd->target_file until io_release_dmabuf() pinned bdev and
bdev->bd_disk for the lifetime of the registered buffer. With fput(target_file)
called immediately in __io_register_dmabuf(), closing target_fd and removing
the namespace (via nvme_ns_remove() -> del_gendisk() -> nvme_put_ns() ->
nvme_free_ns() -> put_disk(ns->disk)) can free the struct gendisk while
bd->ctx remains active.

If a new struct gendisk is later allocated at the same address for a
different block device (such as another NVMe controller/IOMMU domain or a
non-NVMe driver like loop, scsi, virtio_blk, or dm), opening that device will
assign the same f_dma_io_tag address. Subsequent IORING_OP_READ_FIXED or
IORING_OP_WRITE_FIXED requests on the new file will then pass
dma_buf_io_file_compatible() in io_import_dmabuf_map() and submit REQ_DMABUF
bios (where bio->bi_dmabuf_map aliases bio->bi_io_vec with bi_max_vecs == 0)
to an incompatible device or driver.

> diff --git a/io_uring/dma-buf.h b/io_uring/dma-buf.h
> index a97412ebbb3fb..9bd7c8c643976 100644
> --- a/io_uring/dma-buf.h
> +++ b/io_uring/dma-buf.h

[ ... ]

> @@ -63,11 +62,10 @@ static inline int io_import_dmabuf_map(struct io_kiocb *req,
>  				struct io_mapped_ubuf *imu,
>  				size_t len, size_t offset)
>  {
> -	struct io_buf_dma *bd = io_buf_to_dmabuf(imu);
> -
>  	if (!IS_ENABLED(CONFIG_DMA_SHARED_BUFFER))
>  		return -EOPNOTSUPP;
> -	if (req->file != bd->target_file)
> +	if (unlikely(!dma_buf_io_file_compatible(io_buf_to_dmabuf(imu)->ctx,
> +						 req->file)))
>  		return -EBADF;

[Severity: Critical]
Can dma_buf_io_file_compatible() in io_import_dmabuf_map() match unrelated
files due to uninitialized f_dma_io_tag in init_file()?

In fs/file_table.c, alloc_empty_file() and alloc_empty_file_noaccount()
allocate struct file objects from filp_cache (which uses SLAB_TYPESAFE_BY_RCU
without __GFP_ZERO or a slab constructor), and init_file() initializes fields
individually without clearing f->f_dma_io_tag:

fs/file_table.c:init_file() {
	...
	f->f_iocb_flags = 0;
	f->f_pos	= 0;
	f->f_wb_err	= 0;
	f->f_sb_err	= 0;

	file_ref_init(&f->f_ref, 1);
	return 0;
}

When a block device file is closed (including target_file after
__io_register_dmabuf() calls fput(target_file)), its struct file is returned
to filp_cache still holding its stale f_dma_io_tag value. If that struct file
is recycled for a non-block file (for example, an O_DIRECT regular file on a
filesystem backed by loop, scsi, virtio_blk, dm, or another NVMe controller),
req->file->f_dma_io_tag will still equal ctx->importer_tag.

Wouldn't dma_buf_io_file_compatible() then return true in
io_import_dmabuf_map(), allowing ITER_DMABUF_MAP / REQ_DMABUF bios (with
bio->bi_io_vec aliased to struct dma_buf_io_map *) to be submitted to
incompatible filesystems and block drivers?

[Severity: High]
Also related to how f_dma_io_tag is populated for dma_buf_io_file_compatible()
in this series, in block/fops.c:blkdev_open():

block/fops.c:blkdev_open() {
	...
	ret = bdev_open(bdev, mode, filp->private_data, NULL, filp);
	if (ret)
		blkdev_put_no_open(bdev);
	filp->f_dma_io_tag = (unsigned long)bdev->bd_disk;
	return ret;
}

When bdev_open() fails, blkdev_put_no_open(bdev) drops the reference on bdev
acquired by blkdev_get_no_open(). Doesn't setting filp->f_dma_io_tag =
(unsigned long)bdev->bd_disk right after blkdev_put_no_open(bdev) cause a
use-after-free read of bdev?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1793751001.git.asml.silence@gmail.com?part=4

[PATCH 5/5] io_uring/dma-buf: get rid of struct io_buf_dma

[Pavel Begunkov]

Now struct io_buf_dma only stores dma_buf_io_ctx, and we can use the
latter directly.

Fixes: 62e4bf5b25d44 ("io_uring/rsrc: add dmabuf backed registered buffers")
Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>
---
 io_uring/dma-buf.c | 16 ++++------------
 io_uring/dma-buf.h | 16 ++++++----------
 2 files changed, 10 insertions(+), 22 deletions(-)

diff --git a/io_uring/dma-buf.c b/io_uring/dma-buf.c
index f25749f7da22..a1fc34c0c044 100644
--- a/io_uring/dma-buf.c
+++ b/io_uring/dma-buf.c
@@ -2,17 +2,14 @@
 
 static inline void io_release_dmabuf(void *priv)
 {
-	struct io_buf_dma *bd = io_priv_to_dmabuf(priv);
-
-	dma_buf_io_ctx_release(bd->ctx);
-	kfree(bd);
+	dma_buf_io_ctx_release(io_priv_to_dmabuf(priv));
 }
 
 int __io_register_dmabuf(struct io_ring_ctx *ctx,
 			struct io_uring_regbuf_desc *desc,
 			struct io_mapped_ubuf *imu)
 {
-	struct io_buf_dma *bd = NULL;
+	struct dma_buf_io_ctx *dmabuf_ctx;
 	struct file *target_file = NULL;
 	struct dma_buf *dmabuf = NULL;
 	int ret;
@@ -27,10 +24,6 @@ int __io_register_dmabuf(struct io_ring_ctx *ctx,
 	if (desc->uaddr || desc->size)
 		return -EINVAL;
 
-	bd = kzalloc(sizeof(*bd), GFP_KERNEL);
-	if (!bd)
-		return -ENOMEM;
-
 	target_file = fget(desc->target_fd);
 	if (!target_file) {
 		ret = -EBADF;
@@ -47,20 +40,19 @@ int __io_register_dmabuf(struct io_ring_ctx *ctx,
 		goto err;
 
 	ret = dma_buf_io_ctx_create(target_file, dmabuf, DMA_BIDIRECTIONAL,
-				    &bd->ctx);
+				    &dmabuf_ctx);
 	if (ret)
 		goto err;
 
 	imu->len = dmabuf->size;
 	imu->release = io_release_dmabuf;
-	imu->priv = bd;
+	imu->priv = dmabuf_ctx;
 	imu->flags = IO_REGBUF_F_DMABUF;
 	imu->dir = IO_BUF_DEST | IO_BUF_SOURCE;
 	dma_buf_put(dmabuf);
 	fput(target_file);
 	return 0;
 err:
-	kfree(bd);
 	if (target_file)
 		fput(target_file);
 	if (dmabuf)
diff --git a/io_uring/dma-buf.h b/io_uring/dma-buf.h
index 9bd7c8c64397..c622347fbd76 100644
--- a/io_uring/dma-buf.h
+++ b/io_uring/dma-buf.h
@@ -7,16 +7,12 @@
 
 #include "rsrc.h"
 
-struct io_buf_dma {
-	struct dma_buf_io_ctx		*ctx;
-};
-
-static inline struct io_buf_dma *io_priv_to_dmabuf(void *data)
+static inline struct dma_buf_io_ctx *io_priv_to_dmabuf(void *data)
 {
 	return data;
 }
 
-static inline struct io_buf_dma *io_buf_to_dmabuf(struct io_mapped_ubuf *imu)
+static inline struct dma_buf_io_ctx *io_buf_to_dmabuf(struct io_mapped_ubuf *imu)
 {
 	return io_priv_to_dmabuf(imu->priv);
 }
@@ -40,16 +36,16 @@ static inline int io_attach_dmabuf_map(struct io_kiocb *req,
 					unsigned issue_flags)
 {
 	bool nowait = issue_flags & IO_URING_F_NONBLOCK;
+	struct io_mapped_ubuf *imu;
 	struct dma_buf_io_map *map;
-	struct io_buf_dma *bd;
 
 	if (!(req->flags & REQ_F_DMABUF))
 		return 0;
 	if (!IS_ENABLED(CONFIG_DMA_SHARED_BUFFER))
 		return -EOPNOTSUPP;
 
-	bd = io_buf_to_dmabuf(req->buf_node->buf);
-	map = dma_buf_io_get_map(bd->ctx, nowait);
+	imu = req->buf_node->buf;
+	map = dma_buf_io_get_map(io_buf_to_dmabuf(imu), nowait);
 	if (unlikely(IS_ERR(map)))
 		return PTR_ERR(map);
 	req->dmabuf_map = map;
@@ -64,7 +60,7 @@ static inline int io_import_dmabuf_map(struct io_kiocb *req,
 {
 	if (!IS_ENABLED(CONFIG_DMA_SHARED_BUFFER))
 		return -EOPNOTSUPP;
-	if (unlikely(!dma_buf_io_file_compatible(io_buf_to_dmabuf(imu)->ctx,
+	if (unlikely(!dma_buf_io_file_compatible(io_buf_to_dmabuf(imu),
 						 req->file)))
 		return -EBADF;
 	req->flags |= REQ_F_DMABUF;
-- 
2.54.0

Re: [PATCH 0/5] allow dma-buf-io ctx reuse across files

[Pavel Begunkov]

On 11/4/26 00:18, Pavel Begunkov wrote:
> Currently, dma-buf maps can only be used by the file they were
> registered with. This means that we can't open a new file, even if it
> comes from the same subsystem and point to the same block device and
> use it with an old dma-buf registration. That would be too restrictive
> for upcoming filsystem support, so lift it now and allow reusing
> contexts between files pointing to the same NVMe, even if they refer
> to different partitions.
> 
> To do that, we add a tag representing for each file, save it from
> the file passed during registration and use it to reject files
> which tags don't match it. Tags are set by subsystems creating
> files, should be unique and represent the instance of the
> subsystem.

Fwiw, sashiko raised good points, I'll need to address
them in v2.

-- 
Pavel Begunkov