| 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: <CACw3F531DPDtQe5PJmH091n9RhK57FhUYR1L45FdjrYBMSUOtQ@mail.gmail.com>
Date: Tue, 16 Jan 2024 09:07:05 -0800
From: Jiaqi Yan <jiaqiyan@...gle.com>
To: Muhammad Usama Anjum <usama.anjum@...labora.com>
Cc: Andrew Morton <akpm@...ux-foundation.org>, Shuah Khan <shuah@...nel.org>, kernel@...labora.com,
linux-mm@...ck.org, linux-kselftest@...r.kernel.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 5/7] selftests/mm: hugetlb-read-hwpoison: conform test
to TAP format output
On Sun, Jan 14, 2024 at 11:33 PM Muhammad Usama Anjum
<usama.anjum@...labora.com> wrote:
>
> Conform the layout, informational and status messages to TAP. No
> functional change is intended other than the layout of output messages.
>
> Signed-off-by: Muhammad Usama Anjum <usama.anjum@...labora.com>
> ---
> Changes in v3:
> - Use ksft_perror as short hand instead of missing strerror(errno) at
> one place
Minor thing: I think we should preserve previous changelogs, right?
>
> Tested this by reverting the patch a08c7193e4f18dc8508f2d07d0de2c5b94cb39a3
> ("mm/filemap: remove hugetlb special casing in filemap.c") as it has
> broken the test. The bug report can be found at [1].
>
> Tested with proposed fix as well [2].
>
> [1] https://lore.kernel.org/all/079335ab-190f-41f7-b832-6ffe7528fd8b@collabora.com
> [2] https://lore.kernel.org/all/a20e7bdb-7344-306d-e8f5-5ee69af7d5ea@oracle.com
> ---
> .../selftests/mm/hugetlb-read-hwpoison.c | 116 ++++++++----------
> 1 file changed, 54 insertions(+), 62 deletions(-)
>
> diff --git a/tools/testing/selftests/mm/hugetlb-read-hwpoison.c b/tools/testing/selftests/mm/hugetlb-read-hwpoison.c
> index ba6cc6f9cabc..23b41b88c6af 100644
> --- a/tools/testing/selftests/mm/hugetlb-read-hwpoison.c
> +++ b/tools/testing/selftests/mm/hugetlb-read-hwpoison.c
> @@ -58,8 +58,8 @@ static bool verify_chunk(char *buf, size_t len, char val)
>
> for (i = 0; i < len; ++i) {
> if (buf[i] != val) {
> - printf(PREFIX ERROR_PREFIX "check fail: buf[%lu] = %u != %u\n",
> - i, buf[i], val);
> + ksft_print_msg(PREFIX ERROR_PREFIX "check fail: buf[%lu] = %u != %u\n",
> + i, buf[i], val);
> return false;
> }
> }
> @@ -75,21 +75,21 @@ static bool seek_read_hugepage_filemap(int fd, size_t len, size_t wr_chunk_size,
> ssize_t total_ret_count = 0;
> char val = offset / wr_chunk_size + offset % wr_chunk_size;
>
> - printf(PREFIX PREFIX "init val=%u with offset=0x%lx\n", val, offset);
> - printf(PREFIX PREFIX "expect to read 0x%lx bytes of data in total\n",
> - expected);
> + ksft_print_msg(PREFIX PREFIX "init val=%u with offset=0x%lx\n", val, offset);
> + ksft_print_msg(PREFIX PREFIX "expect to read 0x%lx bytes of data in total\n",
> + expected);
> if (lseek(fd, offset, SEEK_SET) < 0) {
> - perror(PREFIX ERROR_PREFIX "seek failed");
> + ksft_perror(PREFIX ERROR_PREFIX "seek failed");
> return false;
> }
>
> while (offset + total_ret_count < len) {
> ret_count = read(fd, buf, wr_chunk_size);
> if (ret_count == 0) {
> - printf(PREFIX PREFIX "read reach end of the file\n");
> + ksft_print_msg(PREFIX PREFIX "read reach end of the file\n");
> break;
> } else if (ret_count < 0) {
> - perror(PREFIX ERROR_PREFIX "read failed");
> + ksft_perror(PREFIX ERROR_PREFIX "read failed");
> break;
> }
> ++val;
> @@ -98,8 +98,8 @@ static bool seek_read_hugepage_filemap(int fd, size_t len, size_t wr_chunk_size,
>
> total_ret_count += ret_count;
> }
> - printf(PREFIX PREFIX "actually read 0x%lx bytes of data in total\n",
> - total_ret_count);
> + ksft_print_msg(PREFIX PREFIX "actually read 0x%lx bytes of data in total\n",
> + total_ret_count);
>
> return total_ret_count == expected;
> }
> @@ -112,15 +112,15 @@ static bool read_hugepage_filemap(int fd, size_t len,
> ssize_t total_ret_count = 0;
> char val = 0;
>
> - printf(PREFIX PREFIX "expect to read 0x%lx bytes of data in total\n",
> - expected);
> + ksft_print_msg(PREFIX PREFIX "expect to read 0x%lx bytes of data in total\n",
> + expected);
> while (total_ret_count < len) {
> ret_count = read(fd, buf, wr_chunk_size);
> if (ret_count == 0) {
> - printf(PREFIX PREFIX "read reach end of the file\n");
> + ksft_print_msg(PREFIX PREFIX "read reach end of the file\n");
> break;
> } else if (ret_count < 0) {
> - perror(PREFIX ERROR_PREFIX "read failed");
> + ksft_perror(PREFIX ERROR_PREFIX "read failed");
> break;
> }
> ++val;
> @@ -129,8 +129,8 @@ static bool read_hugepage_filemap(int fd, size_t len,
>
> total_ret_count += ret_count;
> }
> - printf(PREFIX PREFIX "actually read 0x%lx bytes of data in total\n",
> - total_ret_count);
> + ksft_print_msg(PREFIX PREFIX "actually read 0x%lx bytes of data in total\n",
> + total_ret_count);
>
> return total_ret_count == expected;
> }
> @@ -142,14 +142,14 @@ test_hugetlb_read(int fd, size_t len, size_t wr_chunk_size)
> char *filemap = NULL;
>
> if (ftruncate(fd, len) < 0) {
> - perror(PREFIX ERROR_PREFIX "ftruncate failed");
> + ksft_perror(PREFIX ERROR_PREFIX "ftruncate failed");
> return status;
> }
>
> filemap = mmap(NULL, len, PROT_READ | PROT_WRITE,
> MAP_SHARED | MAP_POPULATE, fd, 0);
> if (filemap == MAP_FAILED) {
> - perror(PREFIX ERROR_PREFIX "mmap for primary mapping failed");
> + ksft_perror(PREFIX ERROR_PREFIX "mmap for primary mapping failed");
> goto done;
> }
>
> @@ -162,7 +162,7 @@ test_hugetlb_read(int fd, size_t len, size_t wr_chunk_size)
> munmap(filemap, len);
> done:
> if (ftruncate(fd, 0) < 0) {
> - perror(PREFIX ERROR_PREFIX "ftruncate back to 0 failed");
> + ksft_perror(PREFIX ERROR_PREFIX "ftruncate back to 0 failed");
> status = TEST_FAILED;
> }
>
> @@ -179,14 +179,14 @@ test_hugetlb_read_hwpoison(int fd, size_t len, size_t wr_chunk_size,
> const unsigned long pagesize = getpagesize();
>
> if (ftruncate(fd, len) < 0) {
> - perror(PREFIX ERROR_PREFIX "ftruncate failed");
> + ksft_perror(PREFIX ERROR_PREFIX "ftruncate failed");
> return status;
> }
>
> filemap = mmap(NULL, len, PROT_READ | PROT_WRITE,
> MAP_SHARED | MAP_POPULATE, fd, 0);
> if (filemap == MAP_FAILED) {
> - perror(PREFIX ERROR_PREFIX "mmap for primary mapping failed");
> + ksft_perror(PREFIX ERROR_PREFIX "mmap for primary mapping failed");
> goto done;
> }
>
> @@ -201,7 +201,7 @@ test_hugetlb_read_hwpoison(int fd, size_t len, size_t wr_chunk_size,
> */
> hwp_addr = filemap + len / 2 + pagesize;
> if (madvise(hwp_addr, pagesize, MADV_HWPOISON) < 0) {
> - perror(PREFIX ERROR_PREFIX "MADV_HWPOISON failed");
> + ksft_perror(PREFIX ERROR_PREFIX "MADV_HWPOISON failed");
> goto unmap;
> }
>
> @@ -228,7 +228,7 @@ test_hugetlb_read_hwpoison(int fd, size_t len, size_t wr_chunk_size,
> munmap(filemap, len);
> done:
> if (ftruncate(fd, 0) < 0) {
> - perror(PREFIX ERROR_PREFIX "ftruncate back to 0 failed");
> + ksft_perror(PREFIX ERROR_PREFIX "ftruncate back to 0 failed");
> status = TEST_FAILED;
> }
>
> @@ -240,27 +240,32 @@ static int create_hugetlbfs_file(struct statfs *file_stat)
> int fd;
>
> fd = memfd_create("hugetlb_tmp", MFD_HUGETLB);
> - if (fd < 0) {
> - perror(PREFIX ERROR_PREFIX "could not open hugetlbfs file");
> - return -1;
> - }
> + if (fd < 0)
> + ksft_exit_fail_msg(PREFIX ERROR_PREFIX "could not open hugetlbfs file: %s\n",
> + strerror(errno));
>
> memset(file_stat, 0, sizeof(*file_stat));
> +
> if (fstatfs(fd, file_stat)) {
> - perror(PREFIX ERROR_PREFIX "fstatfs failed");
> - goto close;
> + close(fd);
> + ksft_exit_fail_msg(PREFIX ERROR_PREFIX "fstatfs failed: %s\n", strerror(errno));
> }
> if (file_stat->f_type != HUGETLBFS_MAGIC) {
> - printf(PREFIX ERROR_PREFIX "not hugetlbfs file\n");
> - goto close;
> + close(fd);
> + ksft_exit_fail_msg(PREFIX ERROR_PREFIX "not hugetlbfs file\n");
> }
>
> return fd;
> -close:
> - close(fd);
> - return -1;
> }
>
> +#define KSFT_PRINT_MSG(status, fmt, ...) \
> + do { \
> + if (status == TEST_SKIPPED) \
> + ksft_test_result_skip(fmt, __VA_ARGS__); \
> + else \
> + ksft_test_result(status == TEST_PASSED, fmt, __VA_ARGS__); \
> + } while (0)
> +
> int main(void)
> {
> int fd;
> @@ -273,50 +278,37 @@ int main(void)
> };
> size_t i;
>
> + ksft_print_header();
> + ksft_set_plan(12);
Minor: can this number be calculated, or at least defined as a macro
with documents? That would make it easier for reading.
> +
> for (i = 0; i < ARRAY_SIZE(wr_chunk_sizes); ++i) {
> - printf("Write/read chunk size=0x%lx\n",
> - wr_chunk_sizes[i]);
> + ksft_print_msg("Write/read chunk size=0x%lx\n",
> + wr_chunk_sizes[i]);
>
> fd = create_hugetlbfs_file(&file_stat);
> - if (fd < 0)
> - goto create_failure;
> - printf(PREFIX "HugeTLB read regression test...\n");
> + ksft_print_msg(PREFIX "HugeTLB read regression test...\n");
> status = test_hugetlb_read(fd, file_stat.f_bsize,
> wr_chunk_sizes[i]);
> - printf(PREFIX "HugeTLB read regression test...%s\n",
> - status_to_str(status));
> + KSFT_PRINT_MSG(status, PREFIX "HugeTLB read regression test...%s\n",
> + status_to_str(status));
> close(fd);
> - if (status == TEST_FAILED)
> - return -1;
>
> fd = create_hugetlbfs_file(&file_stat);
> - if (fd < 0)
> - goto create_failure;
> - printf(PREFIX "HugeTLB read HWPOISON test...\n");
> + ksft_print_msg(PREFIX "HugeTLB read HWPOISON test...\n");
> status = test_hugetlb_read_hwpoison(fd, file_stat.f_bsize,
> wr_chunk_sizes[i], false);
> - printf(PREFIX "HugeTLB read HWPOISON test...%s\n",
> - status_to_str(status));
> + KSFT_PRINT_MSG(status, PREFIX "HugeTLB read HWPOISON test..%s\n",
> + status_to_str(status));
> close(fd);
> - if (status == TEST_FAILED)
> - return -1;
>
> fd = create_hugetlbfs_file(&file_stat);
> - if (fd < 0)
> - goto create_failure;
> - printf(PREFIX "HugeTLB seek then read HWPOISON test...\n");
> + ksft_print_msg(PREFIX "HugeTLB seek then read HWPOISON test...\n");
> status = test_hugetlb_read_hwpoison(fd, file_stat.f_bsize,
> wr_chunk_sizes[i], true);
> - printf(PREFIX "HugeTLB seek then read HWPOISON test...%s\n",
> - status_to_str(status));
> + KSFT_PRINT_MSG(status, PREFIX "HugeTLB seek then read HWPOISON test...%s\n",
> + status_to_str(status));
> close(fd);
> - if (status == TEST_FAILED)
> - return -1;
> }
>
> - return 0;
> -
> -create_failure:
> - printf(ERROR_PREFIX "Abort test: failed to create hugetlbfs file\n");
> - return -1;
> + ksft_finished();
> }
> --
> 2.42.0
>
>
This version looks good to me. Maybe someone else need to take another
look, just add mine:
Reviewed-by: Jiaqi Yan <jiaqiyan@...gle.com>
Powered by blists - more mailing lists