Re: [PATCH v3 2/2] mm: selftests: Adjust the MADV_COLLAPSE uffd-minor selftests

[David Hildenbrand (Arm)]

On 9/10/26 04:34, James Houghton wrote:
> The behavior of MADV_COLLAPSE with uffd-minor VMAs has changed:
> MADV_COLLAPSE will no longer install PTEs where none existed before.

Misleading: MADV_COLLAPSE installed a PMD not PTEs?

> Update the selftest to demonstrate this new behavior.
> 
> On an unpatched kernel, the test will hit "unexpected memory contents
> after collapse".
> 
> If the selftest is left unpatched but the kernel is patched, the
> selftest will SKIP when it gets EINVAL back from MADV_COLLAPSE.
> 
> Signed-off-by: James Houghton <jthoughton@xxxxxxxxxx>
> ---
>  tools/testing/selftests/mm/uffd-unit-tests.c | 75 ++++++++++++++++++--
>  1 file changed, 68 insertions(+), 7 deletions(-)
> 
> diff --git a/tools/testing/selftests/mm/uffd-unit-tests.c b/tools/testing/selftests/mm/uffd-unit-tests.c
> index ef9b3956bdcf..6f2360f9b75d 100644
> --- a/tools/testing/selftests/mm/uffd-unit-tests.c
> +++ b/tools/testing/selftests/mm/uffd-unit-tests.c
> @@ -518,19 +518,34 @@ static void uffd_wp_fork_pin_with_event_test(uffd_global_test_opts_t *gopts, uff
>  	uffd_wp_fork_pin_test_common(gopts, args, true);
>  }
>  
> -static void check_memory_contents(uffd_global_test_opts_t *gopts, char *p)
> +static int __check_memory_contents(unsigned long offset,
> +				   unsigned long nr_pages,
> +				   uffd_global_test_opts_t *gopts,
> +				   char *p)

Two tabs look better ;)

>  {
>  	unsigned long i, j;
>  	uint8_t expected_byte;
>  
> -	for (i = 0; i < gopts->nr_pages; ++i) {
> +	if (nr_pages + offset < nr_pages)
> +		err("overflow in memory check");
> +	if (nr_pages + offset > gopts->nr_pages)
> +		err("out of bounds memory check");
> +
> +	for (i = offset; i < offset + nr_pages; ++i) {
>  		expected_byte = ~((uint8_t)(i % ((uint8_t)-1)));
>  		for (j = 0; j < gopts->page_size; j++) {
>  			uint8_t v = *(uint8_t *)(p + (i * gopts->page_size) + j);
>  			if (v != expected_byte)
> -				err("unexpected page contents");
> +				return 1;
>  		}
>  	}
> +
> +	return 0;
> +}
> +
> +static int check_memory_contents(uffd_global_test_opts_t *gopts, char *p)
> +{
> +	return __check_memory_contents(0, gopts->nr_pages, gopts, p);
>  }
>  
>  static void uffd_minor_test_common(uffd_global_test_opts_t *gopts, bool test_collapse, bool test_wp)
> @@ -539,6 +554,8 @@ static void uffd_minor_test_common(uffd_global_test_opts_t *gopts, bool test_col
>  	pthread_t uffd_mon;
>  	char c = '\0';
>  	struct uffd_args args = { 0 };
> +	unsigned long checked = 0;
> +	bool bad_contents;

Through in an empty line. Or better

struct uffd_args args = { .gopts = gopts };

>  
>  	/*
> @@ -564,24 +581,65 @@ static void uffd_minor_test_common(uffd_global_test_opts_t *gopts, bool test_col
>  	if (pthread_create(&uffd_mon, NULL, uffd_poll_thread, &args))
>  		err("uffd_poll_thread create");
>  
> +	if (test_collapse) {
> +		/*
> +		 * Read just a single page and try collapsing. The collapse
> +		 * should either be rejected or be a no-op.
> +		 */
> +		if (__check_memory_contents(0, 1, gopts, gopts->area_dst_alias))
> +			err("unexpected memory contents before collapse");
> +
> +		/* MADV_COLLAPSE might return EINVAL for uffd-minor VMAs. */
> +		madvise(gopts->area_dst_alias, gopts->nr_pages * gopts->page_size,
> +			MADV_COLLAPSE);

Add an explicit check for expected errnos?

> +		/*
> +		 * If the above collapse mapped pages that were not explicitly
> +		 * CONTINUE'd, the below __check_memory_contents() will not
> +		 * fault on some pages, resulting in incorrect contents. The
> +		 * PTE for the first page may get retracted, so avoid checking
> +		 * that page, as we might take a second fault, flipping the
> +		 * contents a second time.
> +		 */
> +		checked = 1;

I'm not an expert on this code. But a trivial check to see whether there is
something in the page tables that shouldn't be there is through the help of
pagemap_is_populated().

Maybe that would help to simplify things, not sure.


-- 
Cheers,

David