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

[T.J. Mercier]

On Wed, Jul 29, 2026 at 11:27 PM Baineng Shou <shoubaineng@xxxxxxxxx> 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@xxxxxxxxxx>
> Signed-off-by: Baineng Shou <shoubaineng@xxxxxxxxx>
> ---
>  .../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
>