Hi Anshuman

Thanks for your revirew.

On 2025/1/13 12:26, Anshuman Khandual wrote:

Hello Hao,

On 1/13/25 08:58, Hao Ge wrote:

...

From: Hao Ge gehao@kylinos.cn

In the run_with_memfd_hugetlb function, some error handle have passed incorrect parameters. It should be "smem", but it was mistakenly written as "mem".

I guess there are couple of more instances where the returned address 'smem' is not getting tested for MAP_FAILED. Hence the commit message here needs to be bit more generic rather than run_with_memfd_hugetlb() specific.

Thank you for bringing it to my attention. Upon reviewing, I confirm that you are correct.

--- a/tools/testing/selftests/mm/cow.c +++ b/tools/testing/selftests/mm/cow.c @@ -1482,7 +1482,7 @@ static void run_with_zeropage(non_anon_test_fn fn, const char *desc) } smem = mmap(NULL, pagesize, PROT_READ, MAP_PRIVATE | MAP_ANON, -1, 0);

•   if (mem == MAP_FAILED) {
  

•   if (smem == MAP_FAILED) {
             ksft_test_result_fail("mmap() failed\n");
             goto munmap;
     }
  

@@ -1583,7 +1583,7 @@ static void run_with_memfd(non_anon_test_fn fn, const char *desc) goto close; } smem = mmap(NULL, pagesize, PROT_READ, MAP_SHARED, fd, 0);

•   if (mem == MAP_FAILED) {
  

•   if (smem == MAP_FAILED) {
             ksft_test_result_fail("mmap() failed\n");
             goto munmap;
     }
  
  
  

...

Let's fix it.

Fixes: baa489fabd01 ("selftests/vm: rename selftests/vm to selftests/mm")

This commit just renamed the directory from vm/ to mm/ directory. The following commit introduced the problem instead. Please update the Fixes: tag as required.

f8664f3c4a08f799 ("selftests/vm: cow: basic COW tests for non-anonymous pages")

Yes, I accidentally copied the wrong information. I will resend a v2  to address these issues. Thank you very much

Thanks

Best Regards

Hao

...

Signed-off-by: Hao Ge gehao@kylinos.cn

tools/testing/selftests/mm/cow.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/testing/selftests/mm/cow.c b/tools/testing/selftests/mm/cow.c index 32c6ccc2a6be..7a89680d1566 100644 --- a/tools/testing/selftests/mm/cow.c +++ b/tools/testing/selftests/mm/cow.c @@ -1684,7 +1684,7 @@ static void run_with_memfd_hugetlb(non_anon_test_fn fn, const char *desc, goto close; } smem = mmap(NULL, hugetlbsize, PROT_READ, MAP_SHARED, fd, 0);

• if (mem == MAP_FAILED) {

• if (smem == MAP_FAILED) { ksft_test_result_fail("mmap() failed\n"); goto munmap; }

@@ -1696,7 +1696,7 @@ static void run_with_memfd_hugetlb(non_anon_test_fn fn, const char *desc, fn(mem, smem, hugetlbsize); munmap: munmap(mem, hugetlbsize);

• if (mem != MAP_FAILED)

• if (smem != MAP_FAILED) munmap(smem, hugetlbsize); close: close(fd);

A small nit.

Please always send a new patch version separately as a new email thread instead - not part of an ongoing discussion.

On 1/13/25 10:39, Hao Ge wrote:

From: Hao Ge gehao@kylinos.cn

In the cow.c,some error handle have passed incorrect parameters. It should be "smem", but it was mistakenly written as "mem".

Let's fix it.

Fixes: f8664f3c4a08 ("selftests/vm: cow: basic COW tests for non-anonymous pages") Signed-off-by: Hao Ge gehao@kylinos.cn

---

v2: Anshuman pointed out that the error is not limited to the run_with_memfd_hugetlb function; there are other places where it occurs as well. Therefore, let's fix it all together. Similarly, Update the Fix tag to be accurate.

LGTM

Reviewed-by: Anshuman Khandual anshuman.khandual@arm.com

---

tools/testing/selftests/mm/cow.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/tools/testing/selftests/mm/cow.c b/tools/testing/selftests/mm/cow.c index 32c6ccc2a6be..c52c93996ba1 100644 --- a/tools/testing/selftests/mm/cow.c +++ b/tools/testing/selftests/mm/cow.c @@ -1482,7 +1482,7 @@ static void run_with_zeropage(non_anon_test_fn fn, const char *desc) } smem = mmap(NULL, pagesize, PROT_READ, MAP_PRIVATE | MAP_ANON, -1, 0);

• if (mem == MAP_FAILED) {

• if (smem == MAP_FAILED) { ksft_test_result_fail("mmap() failed\n"); goto munmap; }

@@ -1583,7 +1583,7 @@ static void run_with_memfd(non_anon_test_fn fn, const char *desc) goto close; } smem = mmap(NULL, pagesize, PROT_READ, MAP_SHARED, fd, 0);

• if (mem == MAP_FAILED) {

• if (smem == MAP_FAILED) { ksft_test_result_fail("mmap() failed\n"); goto munmap; }

@@ -1634,7 +1634,7 @@ static void run_with_tmpfile(non_anon_test_fn fn, const char *desc) goto close; } smem = mmap(NULL, pagesize, PROT_READ, MAP_SHARED, fd, 0);

• if (mem == MAP_FAILED) {

• if (smem == MAP_FAILED) { ksft_test_result_fail("mmap() failed\n"); goto munmap; }

@@ -1684,7 +1684,7 @@ static void run_with_memfd_hugetlb(non_anon_test_fn fn, const char *desc, goto close; } smem = mmap(NULL, hugetlbsize, PROT_READ, MAP_SHARED, fd, 0);

• if (mem == MAP_FAILED) {

• if (smem == MAP_FAILED) { ksft_test_result_fail("mmap() failed\n"); goto munmap; }

@@ -1696,7 +1696,7 @@ static void run_with_memfd_hugetlb(non_anon_test_fn fn, const char *desc, fn(mem, smem, hugetlbsize); munmap: munmap(mem, hugetlbsize);

• if (mem != MAP_FAILED)

• if (smem != MAP_FAILED) munmap(smem, hugetlbsize);

close: close(fd);