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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Medium] The test does not verify that `errno == EFAULT` when `ioctl` fails, which can lead to false positives if the allocation fails for other reasons.
- [Low] The return value of the first `mprotect` call is ignored.
- [Low] An unnecessary `mprotect` call is made to restore `PROT_WRITE` before `munmap`, accompanied by an inaccurate comment.
- [Low] If the `ioctl` unexpectedly succeeds, the newly allocated file descriptor is leaked.
--

--- Patch [4]: [PATCH v7 4/4] selftests: dmabuf-heaps: add fd-leak-on-EFAULT regression test ---
commit 1be53f13852770ff6e3074adac5bd4f146ea6110
Author: Baineng Shou <shoubaineng@gmail.com>

selftests: dmabuf-heaps: add fd-leak-on-EFAULT regression test

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.

> diff --git a/tools/testing/selftests/dmabuf-heaps/dmabuf-heap.c b/tools/testing/selftests/dmabuf-heaps/dmabuf-heap.c
> index fc9694fc4e89e..1d49df6719198 100644
> --- a/tools/testing/selftests/dmabuf-heaps/dmabuf-heap.c
> +++ b/tools/testing/selftests/dmabuf-heaps/dmabuf-heap.c
[ ... ]
> @@ -390,6 +390,116 @@ static void test_alloc_errors(char *heap_name)
[ ... ]
> +static void test_alloc_no_fd_leak_on_efault(char *heap_name)
> +{
[ ... ]
> +	/*
> +	 * Make the page read-only so copy_to_user() will fault.  The
> +	 * ioctl must fail with -1; if it returns success the test setup
> +	 * is broken (mprotect is synchronous, so there is no race).
> +	 */
> +	mprotect(req, page_size, PROT_READ);

[Severity: Low]
Does this code handle the case where mprotect() fails? If it fails, the memory
would remain writable, the ioctl would succeed, and the test would fail
confusingly instead of reporting a setup failure.

> +
> +	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);

[Severity: Low]
Is this mprotect() call necessary? munmap() manipulates the kernel's memory map
directly and doesn't require the underlying memory pages to have write
permissions to be unmapped.

> +	munmap(req, page_size);
> +
> +	if (ret != -1) {

[Severity: Low]
If the ioctl unexpectedly succeeds (for example if mprotect failed), does this
code leak the newly allocated file descriptor? By the time success is detected,
req has been unmapped, making it impossible to read req->fd to close it.

[Severity: Medium]
Does this code verify that the failure was actually caused by the expected
EFAULT? If the ioctl fails for unrelated reasons like -ENOMEM, ret will still
be -1, fd_before will equal fd_after, and the test will erroneously report PASS
even though the copy_to_user() fault path was never tested.

> +		ksft_test_result_fail("ioctl returned %d, expected -1 EFAULT\n",
> +				      ret);
> +		goto out;
> +	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260817050457.1005285-1-shoubaineng@gmail.com?part=4