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

[sashiko-bot@kernel.org]

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.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261112031811.56500-1-evers_chen@163.com?part=1