Re: [PATCH 2/2] tools: testing: add unmapped_area() tests

[Lorenzo Stoakes <lorenzo.stoakes@oracle.com>]

Given I wonder if the patch is even correct, probably a bit superfluous to
review the test but, just in case :)

On Mon, Jan 27, 2025 at 07:55:27AM +0000, Wei Yang wrote:
> Leverage the userland vma tests to verify unmapped_area{_topdown}
> behavior.

This is really not an acceptable commit message. You are adding tests for a
very, very specific scenario, you're not testing these functions in general
_at all_.

If you have indeed found an issue, then these tests must be majorly
refactored to:

a. Be vastly, vastly better commented. Like it's just borderline
   incomprehensible at the moment to me.
b. Assert and explicitly the alleged regression in an individual test named
   as such with a comment explaining what's going on.
c. Rename the 'general' tests to explicitly check _stack_ behaviour with
   comments to that effect.

Right now this is way off what's required.

>
> Signed-off-by: Wei Yang <richard.weiyang@gmail.com>
> CC: Liam R. Howlett <Liam.Howlett@Oracle.com>
> CC: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
> CC: Vlastimil Babka <vbabka@suse.cz>
> CC: Jann Horn <jannh@google.com>
> ---
>  tools/testing/vma/vma.c          | 177 +++++++++++++++++++++++++++++++
>  tools/testing/vma/vma_internal.h |   2 +-
>  2 files changed, 178 insertions(+), 1 deletion(-)
>
> diff --git a/tools/testing/vma/vma.c b/tools/testing/vma/vma.c
> index 6c31118d5fe5..3c1817b01ac8 100644
> --- a/tools/testing/vma/vma.c
> +++ b/tools/testing/vma/vma.c
> @@ -1735,6 +1735,182 @@ static bool test_mmap_region_basic(void)
>  	return true;
>  }
>
> +static bool test_unmapped_area(void)
> +{

You are not doing a general test, you are testing a VM_GROWSDOWN for legacy
mmap or an arch that does upward growing stacks.

The test should explicitly spell this out.

> +	struct mm_struct mm = {};
> +	struct vm_unmapped_area_info info = {};
> +	unsigned long addr, addr1, addr2, addr3, addr4;
> +	unsigned long low = mmap_min_addr + 0x2000, size = 0x1000, gap = 0x1000;
> +	unsigned long stack_guard_gap_old = stack_guard_gap;
> +
> +	VMA_ITERATOR(vmi, &mm, 0);
> +
> +	current->mm = &mm;
> +
> +	/* adjust guard gap for test */
> +	stack_guard_gap = gap;
> +
> +	/*
> +	 * Prepare a range like this:
> +	 *
> +	 * 0123456789abcdef
> +	 *   m  m    m  m
> +	 *   ma m    m am         start_gap = 0
> +	 *   m am    m am         start_gap = 0x1000
> +	 *   m  m  aam  m         start_gap = 0x2000
> +	 */

OK this diagram is cute but I have NO IDEA what is going on here.

Also here you use 'a' not 'A' as in the commit message for the actual
patch? We need a key dude honestly.

> +	addr1 = low;
> +	__mmap_region(NULL, addr1, size,
> +		      VM_READ | VM_WRITE | VM_MAYREAD | VM_MAYWRITE,
> +		      addr1 >> PAGE_SHIFT, NULL);

Ignoring errors? Bypassing the mmap_region() logic?

> +	addr2 = addr1 + size + (gap * 2);
> +	__mmap_region(NULL, addr2, size,
> +		      VM_READ | VM_WRITE | VM_MAYREAD | VM_MAYWRITE,
> +		      addr2 >> PAGE_SHIFT, NULL);
> +	addr3 = addr2 + size + (gap * 4);
> +	__mmap_region(NULL, addr3, size,
> +		      VM_READ | VM_WRITE | VM_MAYREAD | VM_MAYWRITE,
> +		      addr3 >> PAGE_SHIFT, NULL);
> +	addr4 = addr3 + size + (gap * 2);
> +	__mmap_region(NULL, addr4, size,
> +		      VM_READ | VM_WRITE | VM_MAYREAD | VM_MAYWRITE,
> +		      addr4 >> PAGE_SHIFT, NULL);
> +
> +	/* start_gap == 0 */
> +	info.length = size;
> +	info.low_limit = low;
> +	info.high_limit = addr4 + size;
> +	info.start_gap = 0;
> +	addr = unmapped_area(&info);
> +	ASSERT_EQ(addr, addr1 + size);
> +	addr = unmapped_area_topdown(&info);
> +	ASSERT_EQ(addr, addr4 - size);
> +
> +	/* start_gap == 0x1000 */
> +	info.start_gap = gap;
> +	addr = unmapped_area(&info);
> +	ASSERT_EQ(addr, addr1 + size + info.start_gap);
> +	addr = unmapped_area_topdown(&info);
> +	ASSERT_EQ(addr, addr4 - size);
> +
> +	/* start_gap == 0x2000 */

How can it be 0x2000? It's only ever 0 or 0x1000 (and only 0x1000 for
x86-64)?

> +	info.start_gap = gap * 2;
> +	addr = unmapped_area(&info);
> +	ASSERT_EQ(addr, addr2 + size + info.start_gap);
> +	addr = unmapped_area_topdown(&info);
> +	ASSERT_EQ(addr, addr3 - size);
> +

I honestly have zero idea what you're testing here. You really have to add
some comments.

> +	cleanup_mm(&mm, &vmi);
> +
> +	/*
> +	 * Prepare a range like this with VM_GROWSDOWN set:
> +	 *
> +	 * 0123456789abcdef
> +	 *   m  m    m  m
> +	 *   ma m    ma m         start_gap = 0
> +	 *   m  m aa m  m         start_gap = 0x1000
> +	 *   m  m  a m  m         start_gap = 0x2000

Another diagram that is cute but where I have no idea what is going on...

> +	 */
> +	addr1 = low;
> +	__mmap_region(NULL, addr1, size,
> +		      VM_READ | VM_WRITE | VM_MAYREAD | VM_MAYWRITE | VM_GROWSDOWN,
> +		      addr1 >> PAGE_SHIFT, NULL);
> +	addr2 = addr1 + size + (gap * 2);
> +	__mmap_region(NULL, addr2, size,
> +		      VM_READ | VM_WRITE | VM_MAYREAD | VM_MAYWRITE | VM_GROWSDOWN,
> +		      addr2 >> PAGE_SHIFT, NULL);
> +	addr3 = addr2 + size + (gap * 4);
> +	__mmap_region(NULL, addr3, size,
> +		      VM_READ | VM_WRITE | VM_MAYREAD | VM_MAYWRITE | VM_GROWSDOWN,
> +		      addr3 >> PAGE_SHIFT, NULL);
> +	addr4 = addr3 + size + (gap * 2);
> +	__mmap_region(NULL, addr4, size,
> +		      VM_READ | VM_WRITE | VM_MAYREAD | VM_MAYWRITE | VM_GROWSDOWN,
> +		      addr4 >> PAGE_SHIFT, NULL);

You're explicitly testing stack behaviour, again you should document this
by naming the test appropriately or doing it in another test.

> +
> +	/* start_gap == 0 */
> +	info.length = size;
> +	info.low_limit = low;
> +	info.high_limit = addr4 + size;
> +	info.start_gap = 0;
> +	addr = unmapped_area(&info);
> +	ASSERT_EQ(addr, addr1 + size);
> +	addr = unmapped_area_topdown(&info);
> +	ASSERT_EQ(addr, addr4 - stack_guard_gap - size);
> +
> +	/* start_gap == 0x1000 */
> +	info.start_gap = gap;
> +	addr = unmapped_area(&info);
> +	ASSERT_EQ(addr, addr2 + size + info.start_gap);
> +	addr = unmapped_area_topdown(&info);
> +	ASSERT_EQ(addr, addr3 - stack_guard_gap - size);
> +
> +	/* start_gap == 0x2000 */
> +	info.start_gap = gap * 2;
> +	addr = unmapped_area(&info);
> +	ASSERT_EQ(addr, addr2 + size + info.start_gap);
> +	addr = unmapped_area_topdown(&info);
> +	ASSERT_EQ(addr, addr3 - stack_guard_gap - size);
> +

Again I really have no idea what it is you're testing here or what's going
on. These tests are really hard to follow and it's not clear you're
actually asserting that the alleged regression is resolved?

> +	cleanup_mm(&mm, &vmi);
> +
> +	/*
> +	 * Prepare a range like this with VM_GROWSUP set:
> +	 *
> +	 * 0123456789abcdef
> +	 *   m  m    m  m
> +	 *   m am    m am         start_gap = 0
> +	 *   m am    m am         start_gap = 0x1000
> +	 *   m  m  aam  m         start_gap = 0x2000

Same comments as before.

> +	 */
> +	addr1 = low;
> +	__mmap_region(NULL, addr1, size,
> +		      VM_READ | VM_WRITE | VM_MAYREAD | VM_MAYWRITE | VM_GROWSUP,
> +		      addr1 >> PAGE_SHIFT, NULL);
> +	addr2 = addr1 + size + (gap * 2);
> +	__mmap_region(NULL, addr2, size,
> +		      VM_READ | VM_WRITE | VM_MAYREAD | VM_MAYWRITE | VM_GROWSUP,
> +		      addr2 >> PAGE_SHIFT, NULL);
> +	addr3 = addr2 + size + (gap * 4);
> +	__mmap_region(NULL, addr3, size,
> +		      VM_READ | VM_WRITE | VM_MAYREAD | VM_MAYWRITE | VM_GROWSUP,
> +		      addr3 >> PAGE_SHIFT, NULL);
> +	addr4 = addr3 + size + (gap * 2);
> +	__mmap_region(NULL, addr4, size,
> +		      VM_READ | VM_WRITE | VM_MAYREAD | VM_MAYWRITE | VM_GROWSUP,
> +		      addr4 >> PAGE_SHIFT, NULL);

Same comments as before.

> +
> +	/* start_gap == 0 */
> +	info.length = size;
> +	info.low_limit = low;
> +	info.high_limit = addr4 + size;
> +	info.start_gap = 0;
> +	addr = unmapped_area(&info);
> +	ASSERT_EQ(addr, addr1 + size + stack_guard_gap);
> +	addr = unmapped_area_topdown(&info);
> +	ASSERT_EQ(addr, addr4 - size);
> +
> +	/* start_gap == 0x1000 */
> +	info.start_gap = gap;
> +	addr = unmapped_area(&info);
> +	ASSERT_EQ(addr, addr1 + size + stack_guard_gap);
> +	addr = unmapped_area_topdown(&info);
> +	ASSERT_EQ(addr, addr4 - size);
> +
> +	/* start_gap == 0x2000 */
> +	info.start_gap = gap * 2;
> +	addr = unmapped_area(&info);
> +	ASSERT_EQ(addr, addr2 + size + info.start_gap);
> +	addr = unmapped_area_topdown(&info);
> +	ASSERT_EQ(addr, addr3 - size);
> +
> +	cleanup_mm(&mm, &vmi);

Same comments as before.

> +
> +	/* restore stack_guard_gap */
> +	stack_guard_gap = stack_guard_gap_old;

Be nice to have test bring up/tear down. But anyway.

> +	return true;
> +}
> +
>  int main(void)
>  {
>  	int num_tests = 0, num_fail = 0;
> @@ -1769,6 +1945,7 @@ int main(void)
>  	TEST(expand_only_mode);
>
>  	TEST(mmap_region_basic);
> +	TEST(unmapped_area);
>
>  #undef TEST
>
> diff --git a/tools/testing/vma/vma_internal.h b/tools/testing/vma/vma_internal.h
> index 1eae23039854..37b47fb85a3b 100644
> --- a/tools/testing/vma/vma_internal.h
> +++ b/tools/testing/vma/vma_internal.h
> @@ -65,7 +65,7 @@ extern unsigned long dac_mmap_min_addr;
>  #define VM_SHADOW_STACK	VM_NONE
>  #define VM_SOFTDIRTY	0
>  #define VM_ARCH_1	0x01000000	/* Architecture-specific flag */
> -#define VM_GROWSUP	VM_NONE
> +#define VM_GROWSUP	VM_ARCH_1

Not sure I love this, maybe would prefer as a variable but I guess it lets
you choose the flag.

>
>  #define VM_ACCESS_FLAGS (VM_READ | VM_WRITE | VM_EXEC)
>  #define VM_SPECIAL (VM_IO | VM_DONTEXPAND | VM_PFNMAP | VM_MIXEDMAP)
> --
> 2.34.1
>
>