| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <a76fc252-0fe3-4d4b-a9a1-4a2895c2680d@lucifer.local>
Date: Thu, 5 Jun 2025 17:00:49 +0100
From: Lorenzo Stoakes <lorenzo.stoakes@...cle.com>
To: Mark Brown <broonie@...nel.org>
Cc: Andrew Morton <akpm@...ux-foundation.org>, Shuah Khan <shuah@...nel.org>,
David Hildenbrand <david@...hat.com>, linux-mm@...ck.org,
linux-kselftest@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 4/4] selftests/mm: Fix test result reporting in
gup_longterm
This seems to be causing tests to fail rather than be skipped if hugetlb
isn't configured. I bisected the problem to this patch so it's definitely
changed how things are handled (though of course it might just be
_revealing_ some previously existing bug in this test...).
Using a couple of tests as an example:
Before this patch:
# [RUN] R/O longterm GUP-fast pin in MAP_PRIVATE file mapping ... with memfd hugetlb (2048 kB)
ok 39 # SKIP memfd_create() failed (Cannot allocate memory)
# [RUN] R/O longterm GUP-fast pin in MAP_PRIVATE file mapping ... with memfd hugetlb (1048576 kB)
ok 40 # SKIP memfd_create() failed (Cannot allocate memory)
After:
# [RUN] R/O longterm GUP-fast pin in MAP_PRIVATE file mapping ... with memfd hugetlb (2048 kB)
# memfd_create() failed (Cannot allocate memory)
not ok 39 R/O longterm GUP-fast pin in MAP_PRIVATE file mapping ... with memfd hugetlb (2048 kB)
# [RUN] R/O longterm GUP-fast pin in MAP_PRIVATE file mapping ... with memfd hugetlb (1048576 kB)
# memfd_create() failed (Cannot allocate memory)
not ok 40 R/O longterm GUP-fast pin in MAP_PRIVATE file mapping ... with memfd hugetlb (1048576 kB)
See below, I think I've found where it happens...
On Tue, May 27, 2025 at 05:04:48PM +0100, Mark Brown wrote:
> The kselftest framework uses the string logged when a test result is
> reported as the unique identifier for a test, using it to track test
> results between runs. The gup_longterm test fails to follow this
> pattern, it runs a single test function repeatedly with various
> parameters but each result report is a string logging an error message
> which is fixed between runs.
>
> Since the code already logs each test uniquely before it starts refactor
> to also print this to a buffer, then use that name as the test result.
> This isn't especially pretty but is relatively straightforward and is a
> great help to tooling.
>
> Signed-off-by: Mark Brown <broonie@...nel.org>
> ---
> tools/testing/selftests/mm/gup_longterm.c | 150 +++++++++++++++++++-----------
> 1 file changed, 94 insertions(+), 56 deletions(-)
>
> diff --git a/tools/testing/selftests/mm/gup_longterm.c b/tools/testing/selftests/mm/gup_longterm.c
> index e60e62809186..f84ea97c2543 100644
> --- a/tools/testing/selftests/mm/gup_longterm.c
> +++ b/tools/testing/selftests/mm/gup_longterm.c
> @@ -93,33 +93,48 @@ static void do_test(int fd, size_t size, enum test_type type, bool shared)
> __fsword_t fs_type = get_fs_type(fd);
> bool should_work;
> char *mem;
> + int result = KSFT_PASS;
> int ret;
>
> + if (fd < 0) {
> + result = KSFT_FAIL;
> + goto report;
> + }
> +
> if (ftruncate(fd, size)) {
> if (errno == ENOENT) {
> skip_test_dodgy_fs("ftruncate()");
> } else {
> - ksft_test_result_fail("ftruncate() failed (%s)\n", strerror(errno));
> + ksft_print_msg("ftruncate() failed (%s)\n",
> + strerror(errno));
> + result = KSFT_FAIL;
> + goto report;
> }
> return;
> }
>
> if (fallocate(fd, 0, 0, size)) {
> - if (size == pagesize)
> - ksft_test_result_fail("fallocate() failed (%s)\n", strerror(errno));
> - else
> - ksft_test_result_skip("need more free huge pages\n");
> - return;
> + if (size == pagesize) {
> + ksft_print_msg("fallocate() failed (%s)\n", strerror(errno));
> + result = KSFT_FAIL;
> + } else {
> + ksft_print_msg("need more free huge pages\n");
> + result = KSFT_SKIP;
> + }
> + goto report;
> }
>
> mem = mmap(NULL, size, PROT_READ | PROT_WRITE,
> shared ? MAP_SHARED : MAP_PRIVATE, fd, 0);
> if (mem == MAP_FAILED) {
> - if (size == pagesize || shared)
> - ksft_test_result_fail("mmap() failed (%s)\n", strerror(errno));
> - else
> - ksft_test_result_skip("need more free huge pages\n");
> - return;
> + if (size == pagesize || shared) {
> + ksft_print_msg("mmap() failed (%s)\n", strerror(errno));
> + result = KSFT_FAIL;
> + } else {
> + ksft_print_msg("need more free huge pages\n");
> + result = KSFT_SKIP;
> + }
> + goto report;
> }
>
> /* Fault in the page such that GUP-fast can pin it directly. */
> @@ -134,7 +149,8 @@ static void do_test(int fd, size_t size, enum test_type type, bool shared)
> */
> ret = mprotect(mem, size, PROT_READ);
> if (ret) {
> - ksft_test_result_fail("mprotect() failed (%s)\n", strerror(errno));
> + ksft_print_msg("mprotect() failed (%s)\n", strerror(errno));
> + result = KSFT_FAIL;
> goto munmap;
> }
> /* FALLTHROUGH */
> @@ -147,12 +163,14 @@ static void do_test(int fd, size_t size, enum test_type type, bool shared)
> type == TEST_TYPE_RW_FAST;
>
> if (gup_fd < 0) {
> - ksft_test_result_skip("gup_test not available\n");
> + ksft_print_msg("gup_test not available\n");
> + result = KSFT_SKIP;
> break;
> }
>
> if (rw && shared && fs_is_unknown(fs_type)) {
> - ksft_test_result_skip("Unknown filesystem\n");
> + ksft_print_msg("Unknown filesystem\n");
> + result = KSFT_SKIP;
> return;
> }
> /*
> @@ -169,14 +187,19 @@ static void do_test(int fd, size_t size, enum test_type type, bool shared)
> args.flags |= rw ? PIN_LONGTERM_TEST_FLAG_USE_WRITE : 0;
> ret = ioctl(gup_fd, PIN_LONGTERM_TEST_START, &args);
> if (ret && errno == EINVAL) {
> - ksft_test_result_skip("PIN_LONGTERM_TEST_START failed (EINVAL)n");
> + ksft_print_msg("PIN_LONGTERM_TEST_START failed (EINVAL)n");
> + result = KSFT_SKIP;
> break;
> } else if (ret && errno == EFAULT) {
> - ksft_test_result(!should_work, "Should have failed\n");
> + if (should_work)
> + result = KSFT_FAIL;
> + else
> + result = KSFT_PASS;
> break;
> } else if (ret) {
> - ksft_test_result_fail("PIN_LONGTERM_TEST_START failed (%s)\n",
> - strerror(errno));
> + ksft_print_msg("PIN_LONGTERM_TEST_START failed (%s)\n",
> + strerror(errno));
> + result = KSFT_FAIL;
> break;
> }
>
> @@ -189,7 +212,10 @@ static void do_test(int fd, size_t size, enum test_type type, bool shared)
> * some previously unsupported filesystems, we might want to
> * perform some additional tests for possible data corruptions.
> */
> - ksft_test_result(should_work, "Should have worked\n");
> + if (should_work)
> + result = KSFT_PASS;
> + else
> + result = KSFT_FAIL;
> break;
> }
> #ifdef LOCAL_CONFIG_HAVE_LIBURING
> @@ -199,8 +225,9 @@ static void do_test(int fd, size_t size, enum test_type type, bool shared)
>
> /* io_uring always pins pages writable. */
> if (shared && fs_is_unknown(fs_type)) {
> - ksft_test_result_skip("Unknown filesystem\n");
> - return;
> + ksft_print_msg("Unknown filesystem\n");
> + result = KSFT_SKIP;
> + goto report;
> }
> should_work = !shared ||
> fs_supports_writable_longterm_pinning(fs_type);
> @@ -208,8 +235,9 @@ static void do_test(int fd, size_t size, enum test_type type, bool shared)
> /* Skip on errors, as we might just lack kernel support. */
> ret = io_uring_queue_init(1, &ring, 0);
> if (ret < 0) {
> - ksft_test_result_skip("io_uring_queue_init() failed (%s)\n",
> - strerror(-ret));
> + ksft_print_msg("io_uring_queue_init() failed (%s)\n",
> + strerror(-ret));
> + result = KSFT_SKIP;
> break;
> }
> /*
> @@ -222,17 +250,28 @@ static void do_test(int fd, size_t size, enum test_type type, bool shared)
> /* Only new kernels return EFAULT. */
> if (ret && (errno == ENOSPC || errno == EOPNOTSUPP ||
> errno == EFAULT)) {
> - ksft_test_result(!should_work, "Should have failed (%s)\n",
> - strerror(errno));
> + if (should_work) {
> + ksft_print_msg("Should have failed (%s)\n",
> + strerror(errno));
> + result = KSFT_FAIL;
> + } else {
> + result = KSFT_PASS;
> + }
> } else if (ret) {
> /*
> * We might just lack support or have insufficient
> * MEMLOCK limits.
> */
> - ksft_test_result_skip("io_uring_register_buffers() failed (%s)\n",
> - strerror(-ret));
> + ksft_print_msg("io_uring_register_buffers() failed (%s)\n",
> + strerror(-ret));
> + result = KSFT_SKIP;
> } else {
> - ksft_test_result(should_work, "Should have worked\n");
> + if (should_work) {
> + result = KSFT_PASS;
> + } else {
> + ksft_print_msg("Should have worked\n");
> + result = KSFT_FAIL;
> + }
> io_uring_unregister_buffers(&ring);
> }
>
> @@ -246,6 +285,8 @@ static void do_test(int fd, size_t size, enum test_type type, bool shared)
>
> munmap:
> munmap(mem, size);
> +report:
> + log_test_result(result);
> }
>
> typedef void (*test_fn)(int fd, size_t size);
> @@ -254,13 +295,11 @@ static void run_with_memfd(test_fn fn, const char *desc)
> {
> int fd;
>
> - ksft_print_msg("[RUN] %s ... with memfd\n", desc);
> + log_test_start("%s ... with memfd", desc);
>
> fd = memfd_create("test", 0);
> - if (fd < 0) {
> - ksft_test_result_fail("memfd_create() failed (%s)\n", strerror(errno));
> - return;
> - }
> + if (fd < 0)
> + ksft_print_msg("memfd_create() failed (%s)\n", strerror(errno));
>
> fn(fd, pagesize);
> close(fd);
> @@ -271,23 +310,23 @@ static void run_with_tmpfile(test_fn fn, const char *desc)
> FILE *file;
> int fd;
>
> - ksft_print_msg("[RUN] %s ... with tmpfile\n", desc);
> + log_test_start("%s ... with tmpfile", desc);
>
> file = tmpfile();
> if (!file) {
> - ksft_test_result_fail("tmpfile() failed (%s)\n", strerror(errno));
> - return;
> - }
> -
> - fd = fileno(file);
> - if (fd < 0) {
> - ksft_test_result_fail("fileno() failed (%s)\n", strerror(errno));
> - goto close;
> + ksft_print_msg("tmpfile() failed (%s)\n", strerror(errno));
> + fd = -1;
> + } else {
> + fd = fileno(file);
> + if (fd < 0) {
> + ksft_print_msg("fileno() failed (%s)\n", strerror(errno));
> + }
> }
>
> fn(fd, pagesize);
> -close:
> - fclose(file);
> +
> + if (file)
> + fclose(file);
> }
>
> static void run_with_local_tmpfile(test_fn fn, const char *desc)
> @@ -295,22 +334,22 @@ static void run_with_local_tmpfile(test_fn fn, const char *desc)
> char filename[] = __FILE__"_tmpfile_XXXXXX";
> int fd;
>
> - ksft_print_msg("[RUN] %s ... with local tmpfile\n", desc);
> + log_test_start("%s ... with local tmpfile", desc);
>
> fd = mkstemp(filename);
> - if (fd < 0) {
> - ksft_test_result_fail("mkstemp() failed (%s)\n", strerror(errno));
> - return;
> - }
> + if (fd < 0)
> + ksft_print_msg("mkstemp() failed (%s)\n", strerror(errno));
>
> if (unlink(filename)) {
> - ksft_test_result_fail("unlink() failed (%s)\n", strerror(errno));
> - goto close;
> + ksft_print_msg("unlink() failed (%s)\n", strerror(errno));
> + close(fd);
> + fd = -1;
> }
>
> fn(fd, pagesize);
> -close:
> - close(fd);
> +
> + if (fd >= 0)
> + close(fd);
> }
>
> static void run_with_memfd_hugetlb(test_fn fn, const char *desc,
> @@ -319,15 +358,14 @@ static void run_with_memfd_hugetlb(test_fn fn, const char *desc,
> int flags = MFD_HUGETLB;
> int fd;
>
> - ksft_print_msg("[RUN] %s ... with memfd hugetlb (%zu kB)\n", desc,
> + log_test_start("%s ... with memfd hugetlb (%zu kB)", desc,
> hugetlbsize / 1024);
>
> flags |= __builtin_ctzll(hugetlbsize) << MFD_HUGE_SHIFT;
>
> fd = memfd_create("test", flags);
> if (fd < 0) {
> - ksft_test_result_skip("memfd_create() failed (%s)\n", strerror(errno));
> - return;
> + ksft_print_msg("memfd_create() failed (%s)\n", strerror(errno));
I think it's this change that does it (probably).
I wonder if it's worth checking for errno == ENOMEM and skipping in this
case? Or is that too general?
> }
>
> fn(fd, hugetlbsize);
>
> --
> 2.39.5
>
Powered by blists - more mailing lists