Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [Medium] The regression test silently reports a false pass if the ioctl fails for a reason other than the expected -EFAULT. - [Low] A file descriptor is leaked on the test's failure path if the ioctl unexpectedly succeeds. --
--- Patch [4]: [PATCH v6 4/4] selftests: dmabuf-heaps: add fd-leak-on-EFAULT regression test --- commit 2ee174de4cce870677ff17aa1714b40304934392 Author: Baineng Shou <[email protected]> selftests: dmabuf-heaps: add fd-leak-on-EFAULT regression test This commit adds 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. It triggers the failure by placing the ioctl argument in a private anonymous page and changing it to PROT_READ between the copy_from_user() and copy_to_user() calls. > 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 [ ... ] > + 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); > + [Severity: Low] Does this code leak the newly allocated file descriptor if the ioctl unexpectedly succeeds? Since the test unmaps the req structure immediately after the ioctl, the returned file descriptor stored in req->fd is lost. If the ioctl returned 0, the fd cannot be read and closed before the test jumps to the error path. > + if (ret != -1) { [Severity: Medium] Will this silently report a false pass if the ioctl fails for a reason other than the expected EFAULT? The test checks that the return value is -1, but doesn't verify that errno == EFAULT. If the ioctl fails early (for example, returning -EINVAL or -ENOMEM) before reaching the copy_to_user() step, the fd count remains unchanged and the test passes, even though the EFAULT 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/[email protected]?part=4
