Linaro-mm-sig

linaro-mm-sig@lists.linaro.org

18 participants
3296 discussions

[PATCH 1/2] dma-buf: improve sg_table debugging hack v2

by Christian König

This debugging hack is important to enforce the rule that importers should *never* touch the underlying struct page of the exporter. Instead of just mangling the page link create a copy of the sg_table but only copy over the DMA addresses and not the pages. This will cause a NULL pointer de-reference if the importer tries to touch the struct page. Still quite a hack but this at least allows the exporter to properly keeps it's sg_table intact while allowing the DMA-buf maintainer to find and fix misbehaving importers and finally switch over to using a different data structure in the future. v2: improve the hack further by using a wrapper structure and explaining the background a bit more in the commit message. Signed-off-by: Christian König <christian.koenig(a)amd.com> Reviewed-by: Michael J. Ruhl <michael.j.ruhl(a)intel.com> (v1) --- drivers/dma-buf/dma-buf.c | 72 +++++++++++++++++++++++++++++++-------- 1 file changed, 58 insertions(+), 14 deletions(-) diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c index 2305bb2cc1f1..8c4afd360b72 100644 --- a/drivers/dma-buf/dma-buf.c +++ b/drivers/dma-buf/dma-buf.c @@ -35,6 +35,12 @@ #include "dma-buf-sysfs-stats.h" +/* Wrapper to hide the sg_table page link from the importer */ +struct dma_buf_sg_table_wrapper { + struct sg_table *original; + struct sg_table wrapper; +}; + static inline int is_dma_buf_file(struct file *); static DEFINE_MUTEX(dmabuf_list_mutex); @@ -828,21 +834,57 @@ void dma_buf_put(struct dma_buf *dmabuf) } EXPORT_SYMBOL_NS_GPL(dma_buf_put, "DMA_BUF"); -static void mangle_sg_table(struct sg_table *sg_table) +static int dma_buf_mangle_sg_table(struct sg_table **sg_table) { -#ifdef CONFIG_DMABUF_DEBUG - int i; - struct scatterlist *sg; - - /* To catch abuse of the underlying struct page by importers mix - * up the bits, but take care to preserve the low SG_ bits to - * not corrupt the sgt. The mixing is undone on unmap - * before passing the sgt back to the exporter. + struct scatterlist *to_sg, *from_sg; + struct sg_table *from = *sg_table; + struct dma_buf_sg_table_wrapper *to; + int i, ret; + + if (!IS_ENABLED(CONFIG_DMABUF_DEBUG)) + return 0; + + /* + * To catch abuse of the underlying struct page by importers copy the + * sg_table without copying the page_link and give only the copy back to + * the importer. */ - for_each_sgtable_sg(sg_table, sg, i) - sg->page_link ^= ~0xffUL; -#endif + to = kzalloc(sizeof(*to), GFP_KERNEL); + if (!to) + return -ENOMEM; + + ret = sg_alloc_table(&to->wrapper, from->nents, GFP_KERNEL); + if (ret) + goto free_to; + + to_sg = to->wrapper.sgl; + for_each_sgtable_dma_sg(from, from_sg, i) { + sg_set_page(to_sg, NULL, 0, 0); + sg_dma_address(to_sg) = sg_dma_address(from_sg); + sg_dma_len(to_sg) = sg_dma_len(from_sg); + to_sg = sg_next(to_sg); + } + to->original = from; + *sg_table = &to->wrapper; + return 0; + +free_to: + kfree(to); + return ret; +} + +static void dma_buf_demangle_sg_table(struct sg_table **sg_table) +{ + struct dma_buf_sg_table_wrapper *copy; + + if (!IS_ENABLED(CONFIG_DMABUF_DEBUG)) + return; + + copy = container_of(*sg_table, typeof(*copy), wrapper); + *sg_table = copy->original; + sg_free_table(&copy->wrapper); + kfree(copy); } static inline bool @@ -1139,7 +1181,9 @@ struct sg_table *dma_buf_map_attachment(struct dma_buf_attachment *attach, if (ret < 0) goto error_unmap; } - mangle_sg_table(sg_table); + ret = dma_buf_mangle_sg_table(&sg_table); + if (ret) + goto error_unmap; if (IS_ENABLED(CONFIG_DMA_API_DEBUG)) { struct scatterlist *sg; @@ -1220,7 +1264,7 @@ void dma_buf_unmap_attachment(struct dma_buf_attachment *attach, dma_resv_assert_held(attach->dmabuf->resv); - mangle_sg_table(sg_table); + dma_buf_demangle_sg_table(&sg_table); attach->dmabuf->ops->unmap_dma_buf(attach, sg_table, direction); if (dma_buf_pin_on_map(attach)) -- 2.43.0

1 month, 3 weeks

Re: [RFC v2 01/11] file: add callback for pre-mapping dmabuf

by Christian König

On 11/23/25 23:51, Pavel Begunkov wrote: > Add a file callback that maps a dmabuf for the given file and returns > an opaque token of type struct dma_token representing the mapping. I'm really scratching my head what you mean with that? And why the heck would we need to pass a DMA-buf to a struct file? Regards, Christian. > The > implementation details are hidden from the caller, and the implementors > are normally expected to extend the structure. > > The callback callers will be able to pass the token with an IO request, > which implemented in following patches as a new iterator type. The user > should release the token once it's not needed by calling the provided > release callback via appropriate helpers. > > Signed-off-by: Pavel Begunkov <asml.silence(a)gmail.com> > --- > include/linux/dma_token.h | 35 +++++++++++++++++++++++++++++++++++ > include/linux/fs.h | 4 ++++ > 2 files changed, 39 insertions(+) > create mode 100644 include/linux/dma_token.h > > diff --git a/include/linux/dma_token.h b/include/linux/dma_token.h > new file mode 100644 > index 000000000000..9194b34282c2 > --- /dev/null > +++ b/include/linux/dma_token.h > @@ -0,0 +1,35 @@ > +/* SPDX-License-Identifier: GPL-2.0 */ > +#ifndef _LINUX_DMA_TOKEN_H > +#define _LINUX_DMA_TOKEN_H > + > +#include <linux/dma-buf.h> > + > +struct dma_token_params { > + struct dma_buf *dmabuf; > + enum dma_data_direction dir; > +}; > + > +struct dma_token { > + void (*release)(struct dma_token *); > +}; > + > +static inline void dma_token_release(struct dma_token *token) > +{ > + token->release(token); > +} > + > +static inline struct dma_token * > +dma_token_create(struct file *file, struct dma_token_params *params) > +{ > + struct dma_token *res; > + > + if (!file->f_op->dma_map) > + return ERR_PTR(-EOPNOTSUPP); > + res = file->f_op->dma_map(file, params); > + > + WARN_ON_ONCE(!IS_ERR(res) && !res->release); > + > + return res; > +} > + > +#endif > diff --git a/include/linux/fs.h b/include/linux/fs.h > index c895146c1444..0ce9a53fabec 100644 > --- a/include/linux/fs.h > +++ b/include/linux/fs.h > @@ -2262,6 +2262,8 @@ struct dir_context { > struct iov_iter; > struct io_uring_cmd; > struct offset_ctx; > +struct dma_token; > +struct dma_token_params; > > typedef unsigned int __bitwise fop_flags_t; > > @@ -2309,6 +2311,8 @@ 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 *); > + struct dma_token *(*dma_map)(struct file *, > + struct dma_token_params *); > } __randomize_layout; > > /* Supports async buffered reads */

1 month, 3 weeks

Re: [RFC v2 05/11] block: add infra to handle dmabuf tokens

by Christoph Hellwig

On Sun, Nov 23, 2025 at 10:51:25PM +0000, Pavel Begunkov wrote: > +struct dma_token *blkdev_dma_map(struct file *file, > + struct dma_token_params *params) Given that this is a direct file operation instance it should be in block/fops.c. If we do want a generic helper below it, it should take a struct block_device instead. But we can probably defer that until a user for that shows up.

1 month, 3 weeks

Re: [RFC v2 07/11] nvme-pci: implement dma_token backed requests

by Christoph Hellwig

> +static void nvme_sync_dma(struct nvme_dev *nvme_dev, struct request *req, > + enum dma_data_direction dir) > +{ > + struct blk_mq_dma_map *map = req->dma_map; > + int length = blk_rq_payload_bytes(req); > + bool for_cpu = dir == DMA_FROM_DEVICE; > + struct device *dev = nvme_dev->dev; > + dma_addr_t *dma_list = map->private; > + struct bio *bio = req->bio; > + int offset, map_idx; > + > + offset = bio->bi_iter.bi_bvec_done; > + map_idx = offset / NVME_CTRL_PAGE_SIZE; > + length += offset & (NVME_CTRL_PAGE_SIZE - 1); > + > + while (length > 0) { > + u64 dma_addr = dma_list[map_idx++]; > + > + if (for_cpu) > + __dma_sync_single_for_cpu(dev, dma_addr, > + NVME_CTRL_PAGE_SIZE, dir); > + else > + __dma_sync_single_for_device(dev, dma_addr, > + NVME_CTRL_PAGE_SIZE, dir); > + length -= NVME_CTRL_PAGE_SIZE; > + } This looks really inefficient. Usually the ranges in the dmabuf should be much larger than a controller page. > +static void nvme_unmap_premapped_data(struct nvme_dev *dev, > + struct request *req) > +{ > + struct nvme_iod *iod = blk_mq_rq_to_pdu(req); > + > + if (rq_data_dir(req) == READ) > + nvme_sync_dma(dev, req, DMA_FROM_DEVICE); > + if (!(iod->flags & IOD_SINGLE_SEGMENT)) > + nvme_free_descriptors(req); > +} This doesn't really unmap anything :) Also the dma ownership rules say that you always need to call the sync_to_device helpers before I/O and the sync_to_cpu helpers after I/O, no matters if it is a read or write. The implementations then makes them a no-op where possible. > + > + offset = bio->bi_iter.bi_bvec_done; > + map_idx = offset / NVME_CTRL_PAGE_SIZE; > + offset &= (NVME_CTRL_PAGE_SIZE - 1); > + > + prp1_dma = dma_list[map_idx++] + offset; > + > + length -= (NVME_CTRL_PAGE_SIZE - offset); > + if (length <= 0) { > + prp2_dma = 0; Urgg, why is this building PRPs instead of SGLs? Yes, SGLs are an optional feature, but for devices where you want to micro-optimize like this I think we should simply require them. This should cut down on both the memory use and the amount of special mapping code.

1 month, 3 weeks

Re: [RFC v2 06/11] nvme-pci: add support for dmabuf reggistration

by Christoph Hellwig

Splitting this trivial stub from the substantial parts in the next patch feels odd. Please merge them. (and better commit logs and comments really would be useful for others to understand what you've done). > +const struct dma_buf_attach_ops nvme_dmabuf_importer_ops = { > + .move_notify = nvme_dmabuf_move_notify, > + .allow_peer2peer = true, > +}; Tab-align the =, please. > +static int nvme_init_dma_token(struct request_queue *q, > + struct blk_mq_dma_token *token) > +{ > + struct dma_buf_attachment *attach; > + struct nvme_ns *ns = q->queuedata; > + struct nvme_dev *dev = to_nvme_dev(ns->ctrl); > + struct dma_buf *dmabuf = token->dmabuf; > + > + if (dmabuf->size % NVME_CTRL_PAGE_SIZE) > + return -EINVAL; Why do you care about alignment to the controller page size? > + for_each_sgtable_dma_sg(sgt, sg, tmp) { > + dma_addr_t dma = sg_dma_address(sg); > + unsigned long sg_len = sg_dma_len(sg); > + > + while (sg_len) { > + dma_list[i++] = dma; > + dma += NVME_CTRL_PAGE_SIZE; > + sg_len -= NVME_CTRL_PAGE_SIZE; > + } > + } Why does this build controller pages sized chunks?

1 month, 3 weeks

Re: [RFC v2 05/11] block: add infra to handle dmabuf tokens

by Christoph Hellwig

On Sun, Nov 23, 2025 at 10:51:25PM +0000, Pavel Begunkov wrote: > Add blk-mq infrastructure to handle dmabuf tokens. There are two main Please spell out infrastructure in the subject as well. > +struct dma_token *blkdev_dma_map(struct file *file, > + struct dma_token_params *params) > +{ > + struct request_queue *q = bdev_get_queue(file_bdev(file)); > + > + if (!(file->f_flags & O_DIRECT)) > + return ERR_PTR(-EINVAL); Shouldn't the O_DIRECT check be in the caller? > +++ b/block/blk-mq-dma-token.c Missing SPDX and Copyright statement. > @@ -0,0 +1,236 @@ > +#include <linux/blk-mq-dma-token.h> > +#include <linux/dma-resv.h> > + > +struct blk_mq_dma_fence { > + struct dma_fence base; > + spinlock_t lock; > +}; And a high-level comment explaining the fencing logic would be nice as well. > + struct blk_mq_dma_map *map = container_of(ref, struct blk_mq_dma_map, refs); Overly long line. > +static struct blk_mq_dma_map *blk_mq_alloc_dma_mapping(struct blk_mq_dma_token *token) Another one. Also kinda inconsistent between _map in the data structure and _mapping in the function name. > +static inline > +struct blk_mq_dma_map *blk_mq_get_token_map(struct blk_mq_dma_token *token) Really odd return value / scope formatting. > +{ > + struct blk_mq_dma_map *map; > + > + guard(rcu)(); > + > + map = rcu_dereference(token->map); > + if (unlikely(!map || !percpu_ref_tryget_live_rcu(&map->refs))) > + return NULL; > + return map; Please use good old rcu_read_unlock to make this readable. > + guard(mutex)(&token->mapping_lock); Same. > + > + map = blk_mq_get_token_map(token); > + if (map) > + return map; > + > + map = blk_mq_alloc_dma_mapping(token); > + if (IS_ERR(map)) > + return NULL; > + > + dma_resv_lock(dmabuf->resv, NULL); > + ret = dma_resv_wait_timeout(dmabuf->resv, DMA_RESV_USAGE_BOOKKEEP, > + true, MAX_SCHEDULE_TIMEOUT); > + ret = ret ? ret : -ETIME; if (!ret) ret = -ETIME; > +blk_status_t blk_rq_assign_dma_map(struct request *rq, > + struct blk_mq_dma_token *token) > +{ > + struct blk_mq_dma_map *map; > + > + map = blk_mq_get_token_map(token); > + if (map) > + goto complete; > + > + if (rq->cmd_flags & REQ_NOWAIT) > + return BLK_STS_AGAIN; > + > + map = blk_mq_create_dma_map(token); > + if (IS_ERR(map)) > + return BLK_STS_RESOURCE; Having a few comments, that say this is creating the map lazily would probably helper the reader. Also why not keep the !map case in the branch, as the map case should be the fast path and thus usually be straight line in the function? > +void blk_mq_dma_map_move_notify(struct blk_mq_dma_token *token) > +{ > + blk_mq_dma_map_remove(token); > +} Is there a good reason for having this blk_mq_dma_map_move_notify wrapper? > + if (bio_flagged(bio, BIO_DMA_TOKEN)) { > + struct blk_mq_dma_token *token; > + blk_status_t ret; > + > + token = dma_token_to_blk_mq(bio->dma_token); > + ret = blk_rq_assign_dma_map(rq, token); > + if (ret) { > + if (ret == BLK_STS_AGAIN) { > + bio_wouldblock_error(bio); > + } else { > + bio->bi_status = BLK_STS_RESOURCE; > + bio_endio(bio); > + } > + goto queue_exit; > + } > + } Any reason to not just keep the dma_token_to_blk_mq? Also why is this overriding non-BLK_STS_AGAIN errors with BLK_STS_RESOURCE? (I really wish we could make all BLK_STS_AGAIN errors be quiet without the explicit setting of BIO_QUIET, which is a bit annoying, but that's not for this patch). > +static inline > +struct blk_mq_dma_token *dma_token_to_blk_mq(struct dma_token *token) More odd formatting.

1 month, 3 weeks

Re: [RFC v2 04/11] block: introduce dma token backed bio type

by Christoph Hellwig

> diff --git a/block/bio.c b/block/bio.c > index 7b13bdf72de0..8793f1ee559d 100644 > --- a/block/bio.c > +++ b/block/bio.c > @@ -843,6 +843,11 @@ static int __bio_clone(struct bio *bio, struct bio *bio_src, gfp_t gfp) > bio_clone_blkg_association(bio, bio_src); > } > > + if (bio_flagged(bio_src, BIO_DMA_TOKEN)) { > + bio->dma_token = bio_src->dma_token; > + bio_set_flag(bio, BIO_DMA_TOKEN); > + } Historically __bio_clone itself does not clone the payload, just the bio. But we got rid of the callers that want to clone a bio but not the payload long time ago. I'd suggest a prep patch that moves assigning bi_io_vec from bio_alloc_clone and bio_init_clone into __bio_clone, and given that they are the same field that'll take carw of the dma token as well. Alternatively do it in an if/else that the compiler will hopefully optimize away. > @@ -1349,6 +1366,10 @@ int bio_iov_iter_get_pages(struct bio *bio, struct iov_iter *iter, > bio_iov_bvec_set(bio, iter); > iov_iter_advance(iter, bio->bi_iter.bi_size); > return 0; > + } else if (iov_iter_is_dma_token(iter)) { No else after an return please. > +++ b/block/blk-merge.c > @@ -328,6 +328,29 @@ int bio_split_io_at(struct bio *bio, const struct queue_limits *lim, > unsigned nsegs = 0, bytes = 0, gaps = 0; > struct bvec_iter iter; > > + if (bio_flagged(bio, BIO_DMA_TOKEN)) { Please split the dmabuf logic into a self-contained helper here. > + int offset = offset_in_page(bio->bi_iter.bi_bvec_done); > + > + nsegs = ALIGN(bio->bi_iter.bi_size + offset, PAGE_SIZE); > + nsegs >>= PAGE_SHIFT; Why are we hardcoding PAGE_SIZE based "segments" here? > + > + if (offset & lim->dma_alignment || bytes & len_align_mask) > + return -EINVAL; > + > + if (bio->bi_iter.bi_size > max_bytes) { > + bytes = max_bytes; > + nsegs = (bytes + offset) >> PAGE_SHIFT; > + goto split; > + } else if (nsegs > lim->max_segments) { No else after a goto either.

1 month, 3 weeks

Re: [RFC v2 03/11] block: move around bio flagging helpers

by Christoph Hellwig

On Sun, Nov 23, 2025 at 10:51:23PM +0000, Pavel Begunkov wrote: > We'll need bio_flagged() earlier in bio.h in the next patch, move it > together with all related helpers, and mark the bio_flagged()'s bio > argument as const. > > Signed-off-by: Pavel Begunkov <asml.silence(a)gmail.com> Looks good: Reviewed-by: Christoph Hellwig <hch(a)lst.de> Maybe ask Jens to queue it up ASAP to get it out of the way?

1 month, 3 weeks

Re: [RFC v2 02/11] iov_iter: introduce iter type for pre-registered dma

by Christoph Hellwig

On Sun, Nov 23, 2025 at 10:51:22PM +0000, Pavel Begunkov wrote: > diff --git a/include/linux/uio.h b/include/linux/uio.h > index 5b127043a151..1b22594ca35b 100644 > --- a/include/linux/uio.h > +++ b/include/linux/uio.h > @@ -29,6 +29,7 @@ enum iter_type { > ITER_FOLIOQ, > ITER_XARRAY, > ITER_DISCARD, > + ITER_DMA_TOKEN, Please use DMABUF/dmabuf naming everywhere, this is about dmabufs and not dma in general. Otherwise this looks good.

1 month, 3 weeks

Re: [RFC v2 01/11] file: add callback for pre-mapping dmabuf

by Christoph Hellwig

On Sun, Nov 23, 2025 at 10:51:21PM +0000, Pavel Begunkov wrote: > +static inline struct dma_token * > +dma_token_create(struct file *file, struct dma_token_params *params) > +{ > + struct dma_token *res; > + > + if (!file->f_op->dma_map) > + return ERR_PTR(-EOPNOTSUPP); > + res = file->f_op->dma_map(file, params); Calling the file operation ->dmap_map feels really misleading. create_token as in the function name is already much better, but it really is not just dma, but dmabuf related, and that should really be encoded in the name. Also why not pass the dmabuf and direction directly instead of wrapping it in the odd params struct making the whole thing hard to follow?

1 month, 3 weeks

← Newer
1
...
6
7
8
9
10
11
12
...
330
Older →

Jump to page:

2026

2025

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

2012

2011

Linaro-mm-sig