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

[cxg <evers_chen@163.com>]

Hi Andrew,

Thanks for improving the changelog and picking up the patch.

I noticed that the original madv_populate test was introduced by commit 
e5bfac53e310. Would it make sense to add the following Fixes tag?

Fixes: e5bfac53e310 ("selftests/vm: add test for 
MADV_POPULATE_(READ|WRITE)")

I also tested an alternative implementation using the existing procmap 
helpers, as suggested by Sashiko. It passes all 17 tests on RISC-V with 
Linux 7.2.0.

However, since PROCMAP_QUERY requires a relatively recent kernel, 
keeping the current /proc/self/maps implementation may be preferable for 
compatibility with older kernels.

Thanks,
Xionggui


On 11/13/26 11:19, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential
> issues to consider:
> - [Low] selftests/mm: custom /proc/self/maps parser in
>    mapping_is_readable()
> - [Low] selftests/mm: missing Fixes trailer for test_prot_write()
>    failure
> --
>
> commit 3592bfa7e9663819c643afaf7d6c337f6b102d15
> Author: Xionggui Chen <chenxionggui@lanxincomputing.com>
>
>> selftests/mm: Handle implicit read permissions in madv_populate
>>
>> 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.
>>
>> Signed-off-by: Xionggui Chen <chenxionggui@lanxincomputing.com>
> [Severity: Low]
> Should this commit include a Fixes: tag for the test it fixes?
>
>    Fixes: e5bfac53e310 ("selftests/vm: add test for MADV_POPULATE_(READ|WRITE)")
>
>> diff --git a/tools/testing/selftests/mm/madv_populate.c b/tools/testing/selftests/mm/madv_populate.c
>> index 7fce5d0b622be..818f99ee20041 100644
>> --- a/tools/testing/selftests/mm/madv_populate.c
>> +++ b/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;
>> +}
> [Severity: Low]
> Could mapping_is_readable() reuse the shared VMA helpers in vm_util.h
> instead of open-coding a /proc/self/maps parser?
>
> madv_populate.c already includes vm_util.h and links against vm_util.c,
> which provide open_self_procmap(), find_vma_procmap(), and close_procmap()
> (exposing PROCMAP_QUERY_VMA_READABLE in procmap.query.vma_flags) as well as
> check_vmflag(addr, "rd").  Reusing those avoids duplicating VMA lookup logic
> and avoids line truncation on mappings with paths longer than 256 bytes.
>