Re:Re: [PATCH] selftests/mm: Handle implicit read permissions in madv_populate

["陈熊贵 " <evers_chen@163.com>]

[-- Attachment #1: Type: text/plain, Size: 5760 bytes --]

Thanks for the review!

You're right. The changelog is too broad, and the distinction between RISC-V PTE permissions and VMA permissions needs to be made clearer.

I'll revisit the wording to describe the RISC-V-specific behavior more precisely, without implying that readable PTE permissions necessarily mean VM_READ is set.

Thanks,
Xionggui












At 2026-11-13 03:32:31, "Andrew Morton" <akpm@linux-foundation.org> wrote:
>On Thu, 12 Nov 2026 03:18:11 +0000 Xionggui Chen <evers_chen@163.com> wrote:
>
>> The madv_populate selftest assumes that a mapping created with
>> PROT_WRITE cannot be read, and expects MADV_POPULATE_READ to fail
>> with EINVAL.
>> 
>> However, some architectures, including RISC-V, may implicitly grant
>> read permission to PROT_WRITE mappings. On such architectures,
>> MADV_POPULATE_READ succeeds, causing the test to fail incorrectly.
>> 
>> Check the actual VMA permissions through /proc/self/maps and adjust
>> the expected result accordingly.
>> 
>> Add a PROT_NONE test to verify that MADV_POPULATE_READ still fails
>> with EINVAL for mappings without read permission.
>> 
>> On RISC-V with Linux 7.2.0, the original test fails 1 out of 16
>> tests, while the updated test passes all 17 tests.
>
>Code looks good, thanks.
>
>The changelog is a little imprecise.  
>
>"some architectures" is too broad - this is purely riscv.
>
>The wording suggests that a readable hardware PTE alone determines the
>result.  riscv can have readable writable PTEs without setting VM_READ,
>and this test still expects EINVAL on those systems.  
>
>
>I altered the second paragraph, please review.
>
>
>From: Xionggui Chen <chenxionggui@lanxincomputing.com>
>Subject: selftests/mm: handle implicit read permissions in madv_populate
>Date: Thu, 12 Nov 2026 03:18:11 +0000
>
>The madv_populate selftest assumes that mmap(PROT_WRITE) creates a VMA
>without VM_READ and expects MADV_POPULATE_READ to fail with EINVAL.
>
>On RISC-V, commit 6c7559f22b6f ("riscv/mm: ensure PROT_WRITE leads to
>VM_READ | VM_WRITE") makes the kernel set VM_READ for mappings requested
>with PROT_WRITE.  MADV_POPULATE_READ checks this VMA permission, so it
>succeeds and the test fails incorrectly.  Whether the hardware can read a
>writable PTE does not, by itself, determine the madvise() result.
>
>Check the VMA permissions through /proc/self/maps and adjust the expected
>result accordingly.
>
>Add a PROT_NONE test to verify that MADV_POPULATE_READ still fails with
>EINVAL for mappings without read permission.
>
>On RISC-V with Linux 7.2.0, the original test fails 1 out of 16 tests,
>while the updated test passes all 17 tests.
>
>Link: https://lore.kernel.org/20261112031811.56500-1-evers_chen@163.com
>Signed-off-by: Xionggui Chen <chenxionggui@lanxincomputing.com>
>Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
>Cc: David Hildenbrand <david@kernel.org>
>Cc: Shuah Khan <shuah@kernel.org>
>Cc: Lorenzo Stoakes <ljs@kernel.org>
>---
>
> tools/testing/selftests/mm/madv_populate.c |   67 +++++++++++++++++--
> 1 file changed, 63 insertions(+), 4 deletions(-)
>
>--- a/tools/testing/selftests/mm/madv_populate.c~selftests-mm-handle-implicit-read-permissions-in-madv_populate
>+++ a/tools/testing/selftests/mm/madv_populate.c
>@@ -69,10 +69,38 @@ static void test_prot_read(void)
> 	munmap(addr, SIZE);
> }
> 
>+static int mapping_is_readable(void *addr)
>+{
>+	FILE *maps;
>+	char line[256];
>+	unsigned long start, end;
>+	char perms[5];
>+	int readable = -1;
>+
>+	maps = fopen("/proc/self/maps", "r");
>+	if (!maps)
>+		return -1;
>+
>+	while (fgets(line, sizeof(line), maps)) {
>+		if (sscanf(line, "%lx-%lx %4s",
>+			   &start, &end, perms) != 3)
>+			continue;
>+
>+		if (start <= (unsigned long)addr &&
>+		    (unsigned long)addr < end) {
>+			readable = perms[0] == 'r';
>+			break;
>+		}
>+	}
>+
>+	fclose(maps);
>+	return readable;
>+}
>+
> static void test_prot_write(void)
> {
> 	char *addr;
>-	int ret;
>+	int ret, readable;
> 
> 	ksft_print_msg("[RUN] %s\n", __func__);
> 
>@@ -80,9 +108,19 @@ static void test_prot_write(void)
> 	if (addr == MAP_FAILED)
> 		ksft_exit_fail_msg("mmap failed\n");
> 
>+	readable = mapping_is_readable(addr);
>+	if (readable < 0)
>+		ksft_exit_fail_msg("Cannot determine VMA permissions\n");
>+
>+	errno = 0;
> 	ret = madvise(addr, SIZE, MADV_POPULATE_READ);
>-	ksft_test_result(ret == -1 && errno == EINVAL,
>-			 "MADV_POPULATE_READ with PROT_WRITE\n");
>+
>+	if (readable)
>+		ksft_test_result(ret == 0,
>+				 "MADV_POPULATE_READ with readable PROT_WRITE mapping\n");
>+	else
>+		ksft_test_result(ret == -1 && errno == EINVAL,
>+				 "MADV_POPULATE_READ with write-only mapping\n");
> 
> 	ret = madvise(addr, SIZE, MADV_POPULATE_WRITE);
> 	ksft_test_result(!ret, "MADV_POPULATE_WRITE with PROT_WRITE\n");
>@@ -90,6 +128,26 @@ static void test_prot_write(void)
> 	munmap(addr, SIZE);
> }
> 
>+static void test_prot_none(void)
>+{
>+	char *addr;
>+	int ret;
>+
>+	ksft_print_msg("[RUN] %s\n", __func__);
>+
>+	addr = mmap(0, SIZE, PROT_NONE,
>+		    MAP_ANONYMOUS | MAP_PRIVATE, 0, 0);
>+	if (addr == MAP_FAILED)
>+		ksft_exit_fail_msg("mmap failed\n");
>+
>+	errno = 0;
>+	ret = madvise(addr, SIZE, MADV_POPULATE_READ);
>+	ksft_test_result(ret == -1 && errno == EINVAL,
>+			 "MADV_POPULATE_READ with PROT_NONE\n");
>+
>+	munmap(addr, SIZE);
>+}
>+
> static void test_holes(void)
> {
> 	char *addr;
>@@ -266,7 +324,7 @@ static void test_softdirty(void)
> 
> int main(int argc, char **argv)
> {
>-	int nr_tests = 16;
>+	int nr_tests = 17;
> 	int err;
> 
> 	pagesize = getpagesize();
>@@ -280,6 +338,7 @@ int main(int argc, char **argv)
> 	sense_support();
> 	test_prot_read();
> 	test_prot_write();
>+	test_prot_none();
> 	test_holes();
> 	test_populate_read();
> 	test_populate_write();
>_

[-- Attachment #2: Type: text/html, Size: 6883 bytes --]