| Message ID | 20250127075527.16614-3-richard.weiyang@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | vma: fix unmapped_area() | expand |
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 > >
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) +{ + 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 + */ + addr1 = low; + __mmap_region(NULL, addr1, size, + VM_READ | VM_WRITE | VM_MAYREAD | VM_MAYWRITE, + addr1 >> PAGE_SHIFT, NULL); + 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 */ + 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); + + /* + * 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 + */ + 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); + + /* 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); + + 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 + */ + 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); + + /* 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); + + /* restore stack_guard_gap */ + stack_guard_gap = stack_guard_gap_old; + 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 #define VM_ACCESS_FLAGS (VM_READ | VM_WRITE | VM_EXEC) #define VM_SPECIAL (VM_IO | VM_DONTEXPAND | VM_PFNMAP | VM_MIXEDMAP)
Leverage the userland vma tests to verify unmapped_area{_topdown} behavior. 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(-)