> Subject: Re: [PATCH v2] dma-buf/udmabuf: Disable the size limit by
> default
>
> On 28.07.26 22:20, Kasireddy, Vivek wrote:
> > Hi Robert,
> >
> >> Subject: [PATCH v2] dma-buf/udmabuf: Disable the size limit by
> default
> >>
> >> As udmabuf increasingly enjoys popularity - being used in projects
> like
> >> libcamera, Gstreamer, Mesa, KWin and Weston - users more
> frequently
> >> encounter cases where the current default size limit of 64MB is too
> low.
> >> Examples include allocating video buffers at a 8K resolution - and
> even
> >> 4K
> >> is affected when using non-subsampled video formats and high bit
> >> depths.
> >>
> >> In its current form the size limit for individual buffers does not seem
> to
> >> provide any additional level of protection - such as limiting the
> amount
> >> of
> >> memory a process can pin - as the later can just allocate multiple
> >> buffers.
> >> If additional guardrails are desired, they would likely require some
> kind
> >> accounting not limited to individual buffers.
> >>
> >> Therefor let's disable the size limit by default by setting it to the
> >> maximal possible value, INT_MAX.
> >>
> >> Signed-off-by: Robert Mader <robert.mader(a)collabora.com>
> >>
> >> ---
> >>
> >> Changes in V2:
> >> - Use INT_MAX instead of 0 in order to not change behavior
> otherwise.
> >>
> >> See
> >> https://lore.kernel.org/dri-devel/20260711144814.8205-1-
> >> robert.mader(a)collabora.com/
> >> for a previous attempt to make the value configurable via kconfig -
> and
> >> in particular
> >> https://lore.kernel.org/dri-devel/6764ca6f-b4d8-4baa-9d27-
> >> 2ca867ac2d41(a)amd.com/
> >> for the suggestion and discussion to remove the default limit.
> >> ---
> >> drivers/dma-buf/udmabuf.c | 4 ++--
> >> 1 file changed, 2 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/dma-buf/udmabuf.c b/drivers/dma-buf/udmabuf.c
> >> index bced421c0d65..639e93704924 100644
> >> --- a/drivers/dma-buf/udmabuf.c
> >> +++ b/drivers/dma-buf/udmabuf.c
> >> @@ -20,9 +20,9 @@ static int list_limit = 1024;
> >> module_param(list_limit, int, 0644);
> >> MODULE_PARM_DESC(list_limit, "udmabuf_create_list->count limit.
> >> Default is 1024.");
> >>
> >> -static int size_limit_mb = 64;
> >> +static int size_limit_mb = INT_MAX;
> >> module_param(size_limit_mb, int, 0644);
> >> -MODULE_PARM_DESC(size_limit_mb, "Max size of a dmabuf, in
> >> megabytes. Default is 64.");
> >> +MODULE_PARM_DESC(size_limit_mb, "Max size of a dmabuf, in
> >> megabytes. Default is INT_MAX.");
> > Acked-by: Vivek Kasireddy <vivek.kasireddy(a)intel.com>
> >
> > If there are no further concerns/questions from anyone, I'll push it to
> > drm-misc-next soon.
>
> FTR., we already have a first duplicate :P
>
> https://lore.kernel.org/dri-devel/20260803144120.11524-1-
> xaver.hugl(a)kde.org/
Yeah, looks like this change seems to be desirable in many use-cases.
I have pushed it to drm-misc-next today.
Thanks,
Vivek
>
> >
> > Thanks,
> > Vivek
> >> struct udmabuf {
> >> pgoff_t pagecount;
> >> --
> >> 2.55.0
>
> --
> Robert Mader
> Consultant Software Developer
>
> Collabora Ltd.
> Platinum Building, St John's Innovation Park, Cambridge CB4 0DS, UK
> Registered in England & Wales, no. 5513718
On Fri, 31 Jul 2026 15:01:01 -0700 Bobby Eshleman wrote:
> Meanwhile, do we want drivers/net/hw/config to workaround by carrying
> something like SYNC_FILE or DMABUF_HEAPS, or wait and see about the
> udmabuf kconfig change?
Let's enable SYNC_FILE for now and pursue the UDMABUF change in
parallel. It will land in another tree if at all, so we'll have
to wait a while before it propagates to us.
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
>