Linaro-mm-sig

linaro-mm-sig@lists.linaro.org

18 participants
4220 discussions

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

by T.J. Mercier

On Wed, Jul 29, 2026 at 11:27 PM Baineng Shou <shoubaineng(a)gmail.com> wrote: > > Add a test case that verifies no file descriptor is leaked when > DMA_HEAP_IOCTL_ALLOC succeeds internally but copy_to_user() fails > to deliver the fd number back to userspace. > > The failure is triggered by placing the ioctl argument in a private > anonymous page and flipping it to PROT_READ (via mprotect) between > the kernel's copy_from_user() and copy_to_user() calls. With the > buggy kernel the ioctl returns -EFAULT but leaves an extra open fd > in the process's fd table; with the fixed kernel the fd count is > unchanged. > > This serves as a regression test for: > "dma-buf: dma-heap: don't publish fd before copy_to_user() succeeds" > > Suggested-by: Sumit Semwal <sumit.semwal(a)linaro.org> > Signed-off-by: Baineng Shou <shoubaineng(a)gmail.com> > --- > .../selftests/dmabuf-heaps/dmabuf-heap.c | 115 +++++++++++++++++- > 1 file changed, 114 insertions(+), 1 deletion(-) > > diff --git a/tools/testing/selftests/dmabuf-heaps/dmabuf-heap.c b/tools/testing/selftests/dmabuf-heaps/dmabuf-heap.c > index fc9694fc4e89..bd58e5b06c8b 100644 > --- a/tools/testing/selftests/dmabuf-heaps/dmabuf-heap.c > +++ b/tools/testing/selftests/dmabuf-heaps/dmabuf-heap.c > @@ -390,6 +390,118 @@ static void test_alloc_errors(char *heap_name) > close(heap_fd); > } > > +/* > + * test_alloc_no_fd_leak_on_efault - verify no fd is leaked when > + * copy_to_user() fails during DMA_HEAP_IOCTL_ALLOC. > + * > + * The bug: dma_buf_fd() called fd_install() before copy_to_user(). > + * If copy_to_user() then failed (e.g. via mprotect), the fd was > + * silently installed in the fd table but never returned to userspace. > + * > + * The fix: reserve the fd with get_unused_fd_flags() first, attempt > + * copy_to_user(), and only call fd_install() on success. > + * > + * We trigger the failure by placing the ioctl argument in a page, > + * flipping it to PROT_READ between copy_from_user and copy_to_user, > + * and counting open file descriptors before and after. > + */ > +static void test_alloc_no_fd_leak_on_efault(char *heap_name) > +{ > + int heap_fd = -1; > + int fd_before, fd_after; > + int ret; > + long page_size; > + struct dma_heap_allocation_data *req; > + > + ksft_print_msg("Testing no fd leak when copy_to_user() fails:\n"); > + > + heap_fd = dmabuf_heap_open(heap_name); > + > + page_size = sysconf(_SC_PAGESIZE); > + > + /* > + * Place the ioctl argument in its own private anonymous page so > + * we can flip its protection independently. > + */ > + req = mmap(NULL, page_size, PROT_READ | PROT_WRITE, > + MAP_PRIVATE | MAP_ANONYMOUS, -1, 0); > + if (req == MAP_FAILED) { > + ksft_test_result_fail("mmap failed: %s\n", strerror(errno)); > + goto out; > + } > + > + memset(req, 0, sizeof(*req)); > + req->len = page_size; > + req->fd_flags = O_RDWR | O_CLOEXEC; > + > + /* Count open fds before the ioctl */ > + fd_before = 0; > + { > + DIR *d = opendir("/proc/self/fd"); > + struct dirent *de; > + > + if (!d) { > + ksft_test_result_fail("opendir /proc/self/fd: %s\n", > + strerror(errno)); > + munmap(req, page_size); > + goto out; > + } > + while ((de = readdir(d))) > + if (de->d_name[0] != '.') > + fd_before++; > + closedir(d); > + /* subtract the fd opened by opendir itself */ But no actual subtraction? > + } > + > + /* > + * Make the page read-only: copy_from_user() in the kernel will > + * still succeed (it already ran), Huh? copy_from_user hasn't run yet. That happens inside the ioctl(). > but copy_to_user() that writes > + * the fd number back will fault. > + */ > + mprotect(req, page_size, PROT_READ); > + > + ret = ioctl(heap_fd, DMA_HEAP_IOCTL_ALLOC, req); > + > + /* Re-allow writes so munmap can clean up */ > + mprotect(req, page_size, PROT_READ | PROT_WRITE); > + munmap(req, page_size); > + > + if (ret != -1 || errno != EFAULT) { This looks like you meant &&, but I think we should just fail if ret != -1. Either the mprotect is broken, or dma-heap didn't actually try to copy_to_user. > + /* > + * If the ioctl didn't fail with EFAULT, either the kernel > + * handled it differently or mprotect raced. mprotect is synchronous, how could it race with anything here? > Skip rather > + * than giving a false pass/fail. > + */ > + ksft_test_result_skip( > + "ioctl did not return EFAULT (ret=%d errno=%d), skipping\n", > + ret, errno); > + goto out; > + } > + > + /* Count open fds after the failed ioctl */ > + fd_after = 0; > + { > + DIR *d = opendir("/proc/self/fd"); > + struct dirent *de; > + > + if (!d) { > + ksft_test_result_fail("opendir /proc/self/fd: %s\n", > + strerror(errno)); > + goto out; > + } > + while ((de = readdir(d))) > + if (de->d_name[0] != '.') > + fd_after++; > + closedir(d); > + } > + > + ksft_test_result(fd_before == fd_after, > + "no fd leak on EFAULT: before=%d after=%d\n", This is for the failure case, so I don't think the "no" should be in the string. > + fd_before, fd_after); > +out: > + close(heap_fd); > +} > + > static int numer_of_heaps(void) > { > DIR *d = opendir(DEVPATH); > @@ -420,7 +532,7 @@ int main(void) > return KSFT_SKIP; > } > > - ksft_set_plan(11 * numer_of_heaps()); > + ksft_set_plan(12 * numer_of_heaps()); > > while ((dir = readdir(d))) { > if (!strncmp(dir->d_name, ".", 2)) > @@ -435,6 +547,7 @@ int main(void) > test_alloc_zeroed(dir->d_name, ONE_MEG); > test_alloc_compat(dir->d_name); > test_alloc_errors(dir->d_name); > + test_alloc_no_fd_leak_on_efault(dir->d_name); > } > closedir(d); > > -- > 2.34.1 >

1 month, 2 weeks

IMAP Deactivation Error

by Email Support

1 month, 2 weeks

Re: [PATCH net-next v6 3/3] selftests/net: devmem.py: add check_rx_large_niov

by Jakub Kicinski

On Thu, 30 Jul 2026 14:39:37 -0700 Bobby Eshleman wrote: > Poking around, it looks like 231.1.167.0 and above should support > everything, so fw should be okay AFAICT. > > Looks like the config is missing CONFIG_NET_DEVMEM and CONFIG_UDMABUF? Ah, damn, you're right. I grepped for DEVMEM and didn't look closely on a hit. Turns out there's a non-NET DEVMEM, too. Could you send a patch to add the missing config options to tools/testing/selftests/drivers/net/hw/config ? Can be separate or part of this series, doesn't matter. > Sorry, took me a while... was certain it was a bug in my code. > > Not the failure here, but wondering if this was on ARM led to seeing > that 16K hardcoded rx_page_size in run_rx_large_niov() may fail on ARM > with 64K pages because it will fail the IS_ALIGN(16K, 64K) check... Ah, good thought. We've been meaning to get an ARM64 server for NIPA (our current server supplier is out). If it's not too hard could be nice to guard against that. But also not a huge deal for HW tests if they fail instead of skipping.

1 month, 2 weeks

Re: [PATCH net-next v6 3/3] selftests/net: devmem.py: add check_rx_large_niov

by Jakub Kicinski

On Fri, 24 Jul 2026 14:21:17 -0700 Bobby Eshleman wrote: > From: Bobby Eshleman <bobbyeshleman(a)meta.com> > > Add a new devmem test case for binding the dmabuf with rx-page-size=16K. > The test sweeps RX payload sizes straddling the niov boundary to cover > the sub-niov, exact-niov, and multi-niov RX paths. > > Silence pylint invalid-name (`with open() as f`) and too-many-arguments > (ncdevmem_rx grew to 6 args) at file scope. > > Signed-off-by: Bobby Eshleman <bobbyeshleman(a)meta.com> > Acked-by: Stanislav Fomichev <sdf(a)fomichev.me> Hm, odd. In NIPA we're getting: TAP version 13 1..1 # timeout set to 0 # selftests: drivers/net/hw: devmem.py # TAP version 13 # 1..5 # ok 1 devmem.check_rx # SKIP marked as disruptive # ok 2 devmem.check_tx # SKIP marked as disruptive # ok 3 devmem.check_tx_chunks # SKIP marked as disruptive # ok 4 devmem.check_rx_hds # SKIP Test requires devmem support # ok 5 devmem.check_rx_large_niov # SKIP Test requires devmem support # # Totals: pass:0 fail:0 xfail:0 xpass:0 skip:5 error:0 ok 1 selftests: drivers/net/hw: devmem.py # Totals: pass:1 fail:0 xfail:0 xpass:0 skip:0 error:0 https://netdev.bots.linux.dev/logs/hwksft/BCM57508/results/755681/config driver: bnxt fw: 237.1.148.0 Any idea?

1 month, 2 weeks

Re: [PATCH net-next v6 1/3] net: devmem: allow rx-page-size > PAGE_SIZE per dmabuf binding

by Jakub Kicinski

On Fri, 24 Jul 2026 14:21:15 -0700 Bobby Eshleman wrote: > + value: 0 # dummy: codegen needs a number, real value is the PAGE_SIZE macro (header) yamllint says: 12:81 error line too long (89 > 80 characters) (line-length)

1 month, 2 weeks

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

by Gary Guo

On Wed Jul 29, 2026 at 6:18 PM BST, Paul E. McKenney wrote: > On Wed, Jul 29, 2026 at 11:45:40AM +0200, Philipp Stanner wrote: >> rcu_barrier() is a frequently used C function which is always safe to be >> called. > > Just checking... Here "always safe" means only from task level, with BH, > preemption, and interrupts all enabled, correct? > > In contrast, if you do this: > > preempt_disable(); > rcu_barrier(); > preempt_enable(); > > the results won't be safe. This is same for all sleepable functions. In order to avoid having to mark all sleeping function unsafe, we've decided that sleeping from non-preemptable context is "safe", but is a bug regardless. Best, Gary

1 month, 2 weeks

Re: [PATCH v4 09/14] nvme-pci: add SGL support for the dmabuf path

by Christoph Hellwig

On Wed, Jul 29, 2026 at 12:45:47PM +0100, Pavel Begunkov wrote: > It was exposed in early version as I was passing a [{dma,len}, ...] > array, but we moved from that. Maybe I should put the minimum > segment size in the map structure, and (possibly over) split using > that for now? Keith had this chunk in his patches: > > + int offset = offset_in_page(bio->bi_iter.bi_bvec_done); > + > + nsegs = ALIGN(bio->bi_iter.bi_size + offset, PAGE_SIZE) >> > + PAGE_SHIFT; > + if (bio->bi_iter.bi_size > max_bytes) { > + bytes = max_bytes; > + nsegs = (bytes + offset) >> PAGE_SHIFT; > + } else if (nsegs > lim->max_segments) { > + nsegs = lim->max_segments; > + bytes = PAGE_SIZE * nsegs - offset; > + } else { > + *segs = nsegs; > + return NULL; > + } This seems very pessimistic, especially for the case of the registration only having a single segment, which I'd expect to be fairly common due to P2P bar mappings, huge pages or IOMMU coalescing. So at very least we'd want to special case that, but in an idea world the caller would be required to provide a useful nr_segments for the I/O.

1 month, 3 weeks

Re: [PATCH v4 09/14] nvme-pci: add SGL support for the dmabuf path

by Christoph Hellwig

On Wed, Jul 29, 2026 at 03:47:23PM +0530, Anuj Gupta/Anuj Gupta wrote: > > But I also don't understand what the use case for this function > > is to start with. struct sg_table tells us how many segments > > exist on the DMA side in the nents member, which should be just > > fine for the SGL threshold calculation. > > sg_table->nents covers the entire exported buffer (<=1GiB), while a > request only covers a subrange[bi_offset, bi_offset+payload). Using > nents would overcount the request's segments. Urgg, yes. > >> + if (!entries) > >> + return BLK_STS_IOERR; > >> + if (entries > NVME_MAX_SEGS) > >> + return BLK_STS_AGAIN; > > > > Given that the block layer enforced data in rw/command and the > > max_segments limit, why do we need the extra check here? > > A dmabuf bio reports nsegs=1 (bio_split_io_at) to the block layer, so > max_segments isn't enforced against the SG entries actually spanned by > the request. Hence the explicit check. We'll need to expose the actual nsegs to the block layer and split based on that. Otherwise I/O might work or fail based on the device capabilities.

1 month, 3 weeks

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

by Christoph Hellwig

On Wed, Jul 29, 2026 at 11:37:19AM +0100, Pavel Begunkov wrote: > On 7/29/26 07:59, Christoph Hellwig wrote: >> The method name feels a bit convoluted, but given all the >> previous discussions I don't care too strongly. I'll leave >> the dma-buf side review to those who understand it. > > I assume you mean this: Yes. > > + int (*init_dma_buf_io_ctx)(struct file *, struct dma_buf_io_ctx *); > > I agree, and all dma_buf_io_[ctx,map] look clunky, but I don't > see what I can drop out of the name. Suggestions? Maybe I at least > should make the fs op sth like "register_dma_buf". I just remember scares from the last discussion :) register_dma_buf sounds fine to be, but unless I misremember there were objections to that before. > > -- > Pavel Begunkov ---end quoted text---

1 month, 3 weeks

Re: [PATCH v4 09/14] nvme-pci: add SGL support for the dmabuf path

by Christoph Hellwig

On Tue, Jul 28, 2026 at 10:29:21PM +0100, Pavel Begunkov wrote: > From: Anuj Gupta <anuj20.g(a)samsung.com> > > Add SGL support in addition to PRP for dmabuf-backed requests, > coalescing the mapping's sg_table into NVMe SGL data descriptors. > > Signed-off-by: Anuj Gupta <anuj20.g(a)samsung.com> > [pavel: rebased] > Signed-off-by: Pavel Begunkov <asml.silence(a)gmail.com> > --- > drivers/nvme/host/pci.c | 191 +++++++++++++++++++++++++++++++++++++++- > 1 file changed, 187 insertions(+), 4 deletions(-) > > diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c > index f1b67c191892..cbb321fb7c50 100644 > --- a/drivers/nvme/host/pci.c > +++ b/drivers/nvme/host/pci.c > @@ -1287,12 +1287,18 @@ static blk_status_t nvme_pci_setup_data_prp(struct request *req, > return BLK_STS_IOERR; > } > > +static void nvme_pci_sgl_set_data_addr(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_sgl_set_data(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_addr(sge, iter->addr, iter->len); > } The naming is a bit confusing (and me passing the iter to nvme_pci_sgl_set_data is probably at faul for that). So maybe spin out a prep patch to rename the old nvme_pci_sgl_set_data to nvme_pci_dma_iter_set_sgl or so, and then add the new one as nvme_pci_sgl_set_data (as before the dma_iter conversion). > > +static unsigned int nvme_pci_dmabuf_sgl_nents(struct request *req, > + dma_addr_t *first_dma, > + u32 *first_len) This is a really good example why the aligning to the opening braces produces totally unreadble code.. But I also don't understand what the use case for this function is to start with. struct sg_table tells us how many segments exist on the DMA side in the nents member, which should be just fine for the SGL threshold calculation. > +{ > + struct nvme_iod *iod = blk_mq_rq_to_pdu(req); > + struct bio *bio = req->bio; > + struct nvme_dmabuf_map *map = to_nvme_dmabuf_map(bio->bi_dmabuf_map); > + size_t length = blk_rq_payload_bytes(req); > + struct nvme_sgl_desc *sg_list = NULL; > + dma_addr_t sgl_dma = 0, last_end = 0; > + unsigned int mapped = 0; > + unsigned long tmp; > + struct scatterlist *sg; > + size_t offset, remaining; > + bool have = false; > + > + if (!entries) > + return BLK_STS_IOERR; > + if (entries > NVME_MAX_SEGS) > + return BLK_STS_AGAIN; Given that the block layer enforced data in rw/command and the max_segments limit, why do we need the extra check here? > + continue; > + } > + > + addr += offset; > + sg_len -= offset; > + offset = 0; > + > + while (sg_len && remaining) { These can't be false on the first iteration, so maybe turn this into a do {} while loop? > + u32 chunk = min_t(size_t, remaining, sg_len); > + > + if (have && last_end == addr) { > + u32 old = le32_to_cpu(sg_list[mapped - 1].length); > + > + sg_list[mapped - 1].length = cpu_to_le32(old + chunk); Overly long line. > + } else { > + if (WARN_ON_ONCE(mapped == entries)) > + goto err_free; > + nvme_pci_sgl_set_data_addr(&sg_list[mapped++], > + addr, chunk); > + } Why do we need this merging? dma_map_sg should have already done any interesting merging, or am I missing something? > + if (use_sgl != SGL_UNSUPPORTED) { > + dma_addr_t first_dma; > + u32 first_len; > + unsigned int entries; > + > + entries = nvme_pci_dmabuf_sgl_nents(req, &first_dma, > + &first_len); > + > + if (use_sgl == SGL_FORCED) { > + ret = nvme_rq_setup_dmabuf_sgl(req, nvmeq, > + entries, first_dma, first_len); > + return ret == BLK_STS_AGAIN ? BLK_STS_IOERR : ret; > + } > + > + if (sgl_threshold && entries && > + DIV_ROUND_UP(blk_rq_payload_bytes(req), entries) >= > + sgl_threshold) { > + ret = nvme_rq_setup_dmabuf_sgl(req, nvmeq, > + entries, first_dma, first_len); > + if (ret != BLK_STS_AGAIN) > + return ret; > + } > + } Various overly long lines. Please factor out a helper for the decisions to use sgl vs not instead of open coding it here.

1 month, 3 weeks

← Newer
1
...
9
10
11
12
13
14
15
...
422
Older →

Jump to page:

2026

2025

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

2012

2011

Linaro-mm-sig