[PATCH 0/2] vma: fix unmapped_area()

[PATCH 0/2] vma: fix unmapped_area()

[Wei Yang]

The gap check in unmapped_area() seems not correct.

Add test cases to verify the behavior.

Wei Yang (2):
  mm/vma: fix gap check for unmapped_area with VM_GROWSDOWN
  tools: testing: add unmapped_area() tests

 mm/vma.c                         |   2 +-
 tools/testing/vma/vma.c          | 177 +++++++++++++++++++++++++++++++
 tools/testing/vma/vma_internal.h |   2 +-
 3 files changed, 179 insertions(+), 2 deletions(-)

-- 
2.34.1

[PATCH 1/2] mm/vma: fix gap check for unmapped_area with VM_GROWSDOWN

[Wei Yang]

Current unmapped_area() may fail to get the available area.

For example, we have a vma range like below:

    0123456789abcdef
      m  m  A m  m

Let's assume start_gap is 0x2000 and stack_guard_gap is 0x1000. And now
we are looking for free area with size 0x1000 within [0x2000, 0xd000].

The unmapped_area_topdown() could find address at 0x8000, while
unmapped_area() fails.

In original code before commit 3499a13168da ("mm/mmap: use maple tree
for unmapped_area{_topdown}"), the logic is:

  * find a gap with total length, including alignment
  * adjust start address with alignment

What we do now is:

  * find a gap with total length, including alignment
  * adjust start address with alignment
  * then compare the left range with total length

This is not necessary to compare with total length again after start
address adjusted.

Also, it is not correct to minus 1 here. This may not trigger an issue
in real world, since address are usually aligned with page size.

Fixes: 58c5d0d6d522 ("mm/mmap: regression fix for unmapped_area{_topdown}")
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>
CC: Rick Edgecombe <rick.p.edgecombe@intel.com>
Cc: <stable@vger.kernel.org>
---
 mm/vma.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/mm/vma.c b/mm/vma.c
index 3f45a245e31b..d82fdbc710b0 100644
--- a/mm/vma.c
+++ b/mm/vma.c
@@ -2668,7 +2668,7 @@ unsigned long unmapped_area(struct vm_unmapped_area_info *info)
 	gap += (info->align_offset - gap) & info->align_mask;
 	tmp = vma_next(&vmi);
 	if (tmp && (tmp->vm_flags & VM_STARTGAP_FLAGS)) { /* Avoid prev check if possible */
-		if (vm_start_gap(tmp) < gap + length - 1) {
+		if (vm_start_gap(tmp) < gap + info->length) {
 			low_limit = tmp->vm_end;
 			vma_iter_reset(&vmi);
 			goto retry;
-- 
2.34.1

Re: [PATCH 1/2] mm/vma: fix gap check for unmapped_area with VM_GROWSDOWN

[Lorenzo Stoakes]

You have a subject line of 'fix gap check for unmapped_area with
VM_GROWSDOWN'. I'm not sure this is quite accurate.

I don't really have time to do a deep dive (again, this is why it's so
important to give a decent commit message - explaining under what _real
world_ circumstances this will be used etc.).

But anyway, it seems it will only be the case if MMF_TOPDOWN is not set in
the mm flags, which usually requires an mmap compatibility mode to achieve
unless the arch otherwise forces it.

And these arches would be ones where the stack grows UP, right? Or at least
ones where this is possible?

So already we're into specifics - either arches that grow the stack up, or
ones that intentionally use the old mmap compatibility mode are affected.

This happens in:

[ pretty much all unmapped area callers ]
-> vm_unmapped_area()
-> unmapped_area() (if !(info->flags & VM_UNMAPPED_AREA_TOPDOWN)

Where VM_UNMAPPED_AREA_TOPDOWN is only not set in the circumstances
mentioned above.

So, for this issue you claim is the case to happen, you have to:

1. Either be using a stack grows up arch, or enabling an mmap()
compatibility mode.
2. Also set MAP_GROWSDOWN on the mmap() call, which is translated to
VM_GROWSDOWN.

We are already far from 'fix gap check for VM_GROWSDOWN' right? I mean I
don't have the time to absolutely dive into the guts of this, but I assume
this is correct right?

I'm not saying we shouldn't address this, but it's VITAL to clarify what
exactly it is you're tackling.

On Mon, Jan 27, 2025 at 07:55:26AM +0000, Wei Yang wrote:
> Current unmapped_area() may fail to get the available area.
>
> For example, we have a vma range like below:
>
>     0123456789abcdef
>       m  m  A m  m

I don't understand this diagram at all. What is going on here?  What is 'm'
what is 'A' what are these values? page offsets * 0x1000?

is that a page of memory allocated at each 'm'? Is A somehow an address
under consideration?

You _have_ to add a key and explanation here, my mind reading skills are
much diminished in my old age... :P

>
> Let's assume start_gap is 0x2000 and stack_guard_gap is 0x1000. And now
> we are looking for free area with size 0x1000 within [0x2000, 0xd000].

How can start_gap be 0x2000 when it is only ever 0x1000 at most and only
applicable in x86-64 anyway?

>

It'd be good if you'd shown this on the diagram somehow?

Like this:

  <--------->
0123456789abcdef
  m  m  A m  m

But then I'm confused as to what A is once again.

Ideally you'd actually provide what the struct vm_unmapped_area_info fields
actually are with other parameters and _work through_ an example.

Also you're now talking about a stack but you haven't mentioned the word
'stack' anywhere in any part of this series afaict.

'Fix case where the arch grows stacks upwards or we are in legacy mmap mode
but still want to map a grows-down stack' is a LOT more specific than 'fix
unmapped_area()'.

> The unmapped_area_topdown() could find address at 0x8000, while
> unmapped_area() fails.

OK you need to WORK THROUGH why this is. You're putting all the work on me
as a reviewer to go check to see if this is indeed the case. It's not a
fair distribution of work.

>
> In original code before commit 3499a13168da ("mm/mmap: use maple tree
> for unmapped_area{_topdown}"), the logic is:
>
>   * find a gap with total length, including alignment
>   * adjust start address with alignment
>
> What we do now is:
>
>   * find a gap with total length, including alignment
>   * adjust start address with alignment
>   * then compare the left range with total length

What is 'left range'? This explanation is really hard to follow.

>
> This is not necessary to compare with total length again after start
> address adjusted.
>
> Also, it is not correct to minus 1 here. This may not trigger an issue
> in real world, since address are usually aligned with page size.

You aren't saying why.

Also the VMA's start is _always_ page-aligned.

Presumably the minus 1 is intentionally to amke it an inclusive value once
offset by length?

>
> Fixes: 58c5d0d6d522 ("mm/mmap: regression fix for unmapped_area{_topdown}")

Fixes it how?

> 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>
> CC: Rick Edgecombe <rick.p.edgecombe@intel.com>
> Cc: <stable@vger.kernel.org>
> ---
>  mm/vma.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/mm/vma.c b/mm/vma.c
> index 3f45a245e31b..d82fdbc710b0 100644
> --- a/mm/vma.c
> +++ b/mm/vma.c
> @@ -2668,7 +2668,7 @@ unsigned long unmapped_area(struct vm_unmapped_area_info *info)
>  	gap += (info->align_offset - gap) & info->align_mask;
>  	tmp = vma_next(&vmi);
>  	if (tmp && (tmp->vm_flags & VM_STARTGAP_FLAGS)) { /* Avoid prev check if possible */
> -		if (vm_start_gap(tmp) < gap + length - 1) {
> +		if (vm_start_gap(tmp) < gap + info->length) {

Have already spent all morning on this :) Sigh.

OK so let's expand this (again - this is the kind of thing you should do
for a tricky change like this).

info->start_gap is set based on stack_guard_placement() and is either
PAGE_SIZE (0x1000) if VM_SHADOW_STACK is set or 0. This is only applicable
in x86-64.

The align mask is likely to be PAGE_SIZE - 1 but can vary.

length = info->length + align_mask + start_gap

This takes into account the worst possible alignment overhead accounting for any following VMA also.

gap is equal to the current start of the range under consideration (via
vma_iter_addr) and as well institutes the appropriate alignment and
accounts for start_gap for any _prior_ VMA.

Then we try to find the vm_start_gap() for a candidate _next_ VMA, which if
a stack, uses stack_guard_gap() to SUBTRACT the stack guard gap from
vma->vm_start or account for the shadow stack.

Then we finally have:

if (next->vm_start - stack gap < start_of_range + [preceeding gap/alignment requirements] + [worst case length] - 1) {
   Try again
}

Or:

start_of_range >= next->vm_start - stack gap + [preceeding gap/alignment requirements] + [worst case length] + 1

start of range
  v
  |[preceeding gap][ VMA, worst length ][following gap]<stack gap>

Surely the + 1 (which is the -1 in the original calculation) is simply
accounting for the fact that the start of the range is _inclusive_?

You're proposing eliminating entirely the after-this-VMA requirements... why?

This just seems incorrect to me?

You really need to argue the case better if this has some validity, otherwise I think it's just wrong?

>  			low_limit = tmp->vm_end;
>  			vma_iter_reset(&vmi);
>  			goto retry;
> --
> 2.34.1
>

Re: [PATCH 1/2] mm/vma: fix gap check for unmapped_area with VM_GROWSDOWN

[Wei Yang]

On Mon, Jan 27, 2025 at 12:08:04PM +0000, Lorenzo Stoakes wrote:
>You have a subject line of 'fix gap check for unmapped_area with
>VM_GROWSDOWN'. I'm not sure this is quite accurate.
>
>I don't really have time to do a deep dive (again, this is why it's so
>important to give a decent commit message - explaining under what _real
>world_ circumstances this will be used etc.).
>
>But anyway, it seems it will only be the case if MMF_TOPDOWN is not set in
>the mm flags, which usually requires an mmap compatibility mode to achieve
>unless the arch otherwise forces it.
>
>And these arches would be ones where the stack grows UP, right? Or at least
>ones where this is possible?
>
>So already we're into specifics - either arches that grow the stack up, or
>ones that intentionally use the old mmap compatibility mode are affected.
>
>This happens in:
>
>[ pretty much all unmapped area callers ]
>-> vm_unmapped_area()
>-> unmapped_area() (if !(info->flags & VM_UNMAPPED_AREA_TOPDOWN)
>
>Where VM_UNMAPPED_AREA_TOPDOWN is only not set in the circumstances
>mentioned above.
>
>So, for this issue you claim is the case to happen, you have to:
>
>1. Either be using a stack grows up arch, or enabling an mmap()
>compatibility mode.
>2. Also set MAP_GROWSDOWN on the mmap() call, which is translated to
>VM_GROWSDOWN.
>
>We are already far from 'fix gap check for VM_GROWSDOWN' right? I mean I
>don't have the time to absolutely dive into the guts of this, but I assume
>this is correct right?
>
>I'm not saying we shouldn't address this, but it's VITAL to clarify what
>exactly it is you're tackling.
>

Thanks for taking a look.

If my understanding is correct, your concern here is the case here never
happen in real world.

  We are searching a gap bottom-up, while the vma wants top-down.

This maybe possible to me. Here is my understanding, (but maybe not correct).

We have two separate flags affecting the search:

  * mm->flags:      MMF_TOPDOWN  or not
  * vma->vm_flags:  VM_GROWSDOWN or VM_GROWSUP

To me, they are independent.

For mm->flags, arch_pick_mmap_layout() could set/clear MMF_TOPDOWN it based on
the result of mmap_is_legacy(). Even we provide a sysctl file
/proc/sys/vm/legacy_vm_layout for configuration.


For vma->vm_flags, for general, VM_STACK is set to VM_GROWSDOWN by default.
And we use the flag in __bprm_mm_init() and setup_arg_pages().

So to me the case is real and not a very specific one.

But maybe I missed some background. Would you mind telling me the miss part,
if it is not too time wasting?

-- 
Wei Yang
Help you, Help me

Re: [PATCH 1/2] mm/vma: fix gap check for unmapped_area with VM_GROWSDOWN

[Lorenzo Stoakes]

On Tue, Jan 28, 2025 at 03:30:30AM +0000, Wei Yang wrote:
> On Mon, Jan 27, 2025 at 12:08:04PM +0000, Lorenzo Stoakes wrote:
> >You have a subject line of 'fix gap check for unmapped_area with
> >VM_GROWSDOWN'. I'm not sure this is quite accurate.
> >
> >I don't really have time to do a deep dive (again, this is why it's so
> >important to give a decent commit message - explaining under what _real
> >world_ circumstances this will be used etc.).
> >
> >But anyway, it seems it will only be the case if MMF_TOPDOWN is not set in
> >the mm flags, which usually requires an mmap compatibility mode to achieve
> >unless the arch otherwise forces it.
> >
> >And these arches would be ones where the stack grows UP, right? Or at least
> >ones where this is possible?
> >
> >So already we're into specifics - either arches that grow the stack up, or
> >ones that intentionally use the old mmap compatibility mode are affected.
> >
> >This happens in:
> >
> >[ pretty much all unmapped area callers ]
> >-> vm_unmapped_area()
> >-> unmapped_area() (if !(info->flags & VM_UNMAPPED_AREA_TOPDOWN)
> >
> >Where VM_UNMAPPED_AREA_TOPDOWN is only not set in the circumstances
> >mentioned above.
> >
> >So, for this issue you claim is the case to happen, you have to:
> >
> >1. Either be using a stack grows up arch, or enabling an mmap()
> >compatibility mode.
> >2. Also set MAP_GROWSDOWN on the mmap() call, which is translated to
> >VM_GROWSDOWN.
> >
> >We are already far from 'fix gap check for VM_GROWSDOWN' right? I mean I
> >don't have the time to absolutely dive into the guts of this, but I assume
> >this is correct right?
> >
> >I'm not saying we shouldn't address this, but it's VITAL to clarify what
> >exactly it is you're tackling.
> >
>
> Thanks for taking a look.
>
> If my understanding is correct, your concern here is the case here never
> happen in real world.

No, it's not, re-read what I've said.

I'm saying you have completely failed to be specific in your commit message
about how, what where and why the alleged issue happens, forcing me to spend my
entire morning to figure out what on earth it is you're trying to do.

I also (but you've clipped it) say I think your solution is just wrong and that
there isn't an issue here.

I also (but you've clipped it) say you utterly fail to explain what on earth you
are doing.

I also (but you've clipped it) say you assume the start_gap can be 0x2000 even
though it can only ever be 0 or 0x1000.

I have taken a great deal of time to specifically critique this even though
you've given me little to work on.

I simply do not have time to do this and in future if your commit messages are
this bad I will just reject your series.

>
>   We are searching a gap bottom-up, while the vma wants top-down.
>
> This maybe possible to me. Here is my understanding, (but maybe not correct).
>
> We have two separate flags affecting the search:
>
>   * mm->flags:      MMF_TOPDOWN  or not
>   * vma->vm_flags:  VM_GROWSDOWN or VM_GROWSUP
>
> To me, they are independent.

You are simply reiterating things I've told you but you failed to mention in
your series.

>
> For mm->flags, arch_pick_mmap_layout() could set/clear MMF_TOPDOWN it based on
> the result of mmap_is_legacy(). Even we provide a sysctl file
> /proc/sys/vm/legacy_vm_layout for configuration.

Yes I know.

>
>
> For vma->vm_flags, for general, VM_STACK is set to VM_GROWSDOWN by default.
> And we use the flag in __bprm_mm_init() and setup_arg_pages().

Yes I know.

>
> So to me the case is real and not a very specific one.

It's literally very specific to the point where you have to enable a
compatibility mode. As you've just said.

Sorry, this is unacceptable for _core_ mm work. This is not an area where you
can provide nebulous and vague commit messages.

>
> But maybe I missed some background. Would you mind telling me the miss part,
> if it is not too time wasting?
>
> --
> Wei Yang
> Help you, Help me

You have clipped the part where I point out that I think your 'solution' to the
alleged 'problem' is incorrect.

Honestly, drop this series Wei. The finding unmapped area code is necessarily
conservative and fuzzy as it accounts for -worst case- alignment and providing
appropriate gaps.

It failing to find a gap in very specific and awkward circumstances across the
vastness of the virtual address space isn't a bug.

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

[Wei Yang]

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(-)

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)
-- 
2.34.1

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

[Lorenzo Stoakes]

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
>
>

Re: [PATCH 0/2] vma: fix unmapped_area()

[Lorenzo Stoakes]

Hi Wei,

I seem to recall us having a very recent converation about holding off on
patches like these for a little while to which you agreed, and then you
sent this pretty much the very next day? And during the merge window?
Honestly not _hugely_ impressed with that.

In my view this patch should have instead started as a query to Liam about
the gap calculation, this would have been far more civil and would have
allowed us to determine for sure if the approach you've taken here is
valid.

Given your history of sending entirely trivial patches which we keep asking
you not to send (mixed in with the occasional actually useful patch) it is
KEY to communicate to ensure we're on the same page.

If you send meaningful commits, we want to merge them. Arbitrarily sending
something like this, at this point in time, when you've been asked not to -
does not help achieve this aim.

On Mon, Jan 27, 2025 at 07:55:25AM +0000, Wei Yang wrote:
> The gap check in unmapped_area() seems not correct.
>
> Add test cases to verify the behavior.

This is an -entirely- unacceptable cover letter. It's two lines dude. Give
some details. You're actually tackling a very, very specific aspect and
scenario in some of the most sensitive code in all of mm.

You really, really need to be clear on what it is you're doing, why, what
workload you were doing to hit this, what testing you've done, what real
life things this interacts with etc. etc.

It makes our lives easier as maintainers. Right now I see this as 'another
trivial Wei patch', you need to provide details to prove otherwise, if that
is indeed, not the case.

Also your subject line here is horrible - 'fix unmapped_area()' - actually
you seem to be (in your view) correcting the calculation with respect to
upward-growing stacks. Correct me if I'm wrong. I mean even your patch 1/2
has a better message... It needs to be more specific to what you're doing.

>
> Wei Yang (2):
>   mm/vma: fix gap check for unmapped_area with VM_GROWSDOWN
>   tools: testing: add unmapped_area() tests
>
>  mm/vma.c                         |   2 +-
>  tools/testing/vma/vma.c          | 177 +++++++++++++++++++++++++++++++
>  tools/testing/vma/vma_internal.h |   2 +-
>  3 files changed, 179 insertions(+), 2 deletions(-)
>
> --
> 2.34.1
>
>

Re: [PATCH 0/2] vma: fix unmapped_area()

[Wei Yang]

On Mon, Jan 27, 2025 at 10:50:17AM +0000, Lorenzo Stoakes wrote:
>Hi Wei,
>
>I seem to recall us having a very recent converation about holding off on
>patches like these for a little while to which you agreed, and then you
>sent this pretty much the very next day? And during the merge window?
>Honestly not _hugely_ impressed with that.
>

Yes I remember your suggestion. I send this because it is a bug fix to me. 
Per my understanding on your word, it is ok to send a fix.

If I misunderstand, I apologize.

>In my view this patch should have instead started as a query to Liam about
>the gap calculation, this would have been far more civil and would have
>allowed us to determine for sure if the approach you've taken here is
>valid.
>

You are right. I will try to be better next time.

As you mentioned a query before sending a patch, this is preferred, right?
Hope I don't mess this again.

>Given your history of sending entirely trivial patches which we keep asking
>you not to send (mixed in with the occasional actually useful patch) it is
>KEY to communicate to ensure we're on the same page.
>
>If you send meaningful commits, we want to merge them. Arbitrarily sending
>something like this, at this point in time, when you've been asked not to -
>does not help achieve this aim.
>

Thanks, I would be more considerate next time.

>On Mon, Jan 27, 2025 at 07:55:25AM +0000, Wei Yang wrote:
>> The gap check in unmapped_area() seems not correct.
>>
>> Add test cases to verify the behavior.
>
>This is an -entirely- unacceptable cover letter. It's two lines dude. Give
>some details. You're actually tackling a very, very specific aspect and
>scenario in some of the most sensitive code in all of mm.
>
>You really, really need to be clear on what it is you're doing, why, what
>workload you were doing to hit this, what testing you've done, what real
>life things this interacts with etc. etc.
>
>It makes our lives easier as maintainers. Right now I see this as 'another
>trivial Wei patch', you need to provide details to prove otherwise, if that
>is indeed, not the case.
>
>Also your subject line here is horrible - 'fix unmapped_area()' - actually
>you seem to be (in your view) correcting the calculation with respect to
>upward-growing stacks. Correct me if I'm wrong. I mean even your patch 1/2
>has a better message... It needs to be more specific to what you're doing.
>

Thanks to you and Liam. I will try to do better to not waste your time.

>>
>> Wei Yang (2):
>>   mm/vma: fix gap check for unmapped_area with VM_GROWSDOWN
>>   tools: testing: add unmapped_area() tests
>>
>>  mm/vma.c                         |   2 +-
>>  tools/testing/vma/vma.c          | 177 +++++++++++++++++++++++++++++++
>>  tools/testing/vma/vma_internal.h |   2 +-
>>  3 files changed, 179 insertions(+), 2 deletions(-)
>>
>> --
>> 2.34.1
>>
>>

-- 
Wei Yang
Help you, Help me

Re: [PATCH 0/2] vma: fix unmapped_area()

[Lorenzo Stoakes]

On Tue, Jan 28, 2025 at 01:56:19AM +0000, Wei Yang wrote:
> On Mon, Jan 27, 2025 at 10:50:17AM +0000, Lorenzo Stoakes wrote:
> >Hi Wei,
> >
> >I seem to recall us having a very recent converation about holding off on
> >patches like these for a little while to which you agreed, and then you
> >sent this pretty much the very next day? And during the merge window?
> >Honestly not _hugely_ impressed with that.
> >
>
> Yes I remember your suggestion. I send this because it is a bug fix to me.
> Per my understanding on your word, it is ok to send a fix.

This is not a bug. A bug is something that breaks the kernel.

Having to use a different gap in the vastness of virtual address space is
not a bug.

This is you misunderstanding unmapped_area() as a function that for some
reason in your view must function identically in being minimally
conservative whether top-down or bottom-up including scenarios in which
start_gap is an impossible value.

As far as I can tell the function is behaving completely correctly, it is
just conservative in accounting for worst-case alignment and front and rear
gaps of appropriate size.

You could have more civilly tested out this theory, rather than wasting
presumably a LOT of your time on this (meanwhile taking -no- time to write
commit messages), simply asking Liam about it on-list.

Communication is key.

>
> If I misunderstand, I apologize.
>
> >In my view this patch should have instead started as a query to Liam about
> >the gap calculation, this would have been far more civil and would have
> >allowed us to determine for sure if the approach you've taken here is
> >valid.
> >
>
> You are right. I will try to be better next time.
>
> As you mentioned a query before sending a patch, this is preferred, right?
> Hope I don't mess this again.
>
> >Given your history of sending entirely trivial patches which we keep asking
> >you not to send (mixed in with the occasional actually useful patch) it is
> >KEY to communicate to ensure we're on the same page.
> >
> >If you send meaningful commits, we want to merge them. Arbitrarily sending
> >something like this, at this point in time, when you've been asked not to -
> >does not help achieve this aim.
> >
>
> Thanks, I would be more considerate next time.
>
> >On Mon, Jan 27, 2025 at 07:55:25AM +0000, Wei Yang wrote:
> >> The gap check in unmapped_area() seems not correct.
> >>
> >> Add test cases to verify the behavior.
> >
> >This is an -entirely- unacceptable cover letter. It's two lines dude. Give
> >some details. You're actually tackling a very, very specific aspect and
> >scenario in some of the most sensitive code in all of mm.
> >
> >You really, really need to be clear on what it is you're doing, why, what
> >workload you were doing to hit this, what testing you've done, what real
> >life things this interacts with etc. etc.
> >
> >It makes our lives easier as maintainers. Right now I see this as 'another
> >trivial Wei patch', you need to provide details to prove otherwise, if that
> >is indeed, not the case.
> >
> >Also your subject line here is horrible - 'fix unmapped_area()' - actually
> >you seem to be (in your view) correcting the calculation with respect to
> >upward-growing stacks. Correct me if I'm wrong. I mean even your patch 1/2
> >has a better message... It needs to be more specific to what you're doing.
> >
>
> Thanks to you and Liam. I will try to do better to not waste your time.
>
> >>
> >> Wei Yang (2):
> >>   mm/vma: fix gap check for unmapped_area with VM_GROWSDOWN
> >>   tools: testing: add unmapped_area() tests
> >>
> >>  mm/vma.c                         |   2 +-
> >>  tools/testing/vma/vma.c          | 177 +++++++++++++++++++++++++++++++
> >>  tools/testing/vma/vma_internal.h |   2 +-
> >>  3 files changed, 179 insertions(+), 2 deletions(-)
> >>
> >> --
> >> 2.34.1
> >>
> >>
>
> --
> Wei Yang
> Help you, Help me
>

Re: [PATCH 0/2] vma: fix unmapped_area()

[Liam R. Howlett]

* Wei Yang <richard.weiyang@gmail.com> [250127 02:55]:
> The gap check in unmapped_area() seems not correct.
> 

This is not an acceptable cover letter.

You must have spent a significant amount of time trying to figure it out
and spent about 2 seconds on explaining what you think is going on here.

I'm not convinced there's an issue here at all, and I'm not going to
spend time on it after this cover letter.  It seems like you are also
not sure there's a problem.

If it's important enough to fix then it's important enough to explain
what isn't correct.

If it's not important enough to explain then don't bother fixing it.

Thanks,
Liam

Re: [PATCH 0/2] vma: fix unmapped_area()

[Wei Yang]

On Mon, Jan 27, 2025 at 09:19:24AM -0500, Liam R. Howlett wrote:
>* Wei Yang <richard.weiyang@gmail.com> [250127 02:55]:
>> The gap check in unmapped_area() seems not correct.
>> 
>
>This is not an acceptable cover letter.
>
>You must have spent a significant amount of time trying to figure it out
>and spent about 2 seconds on explaining what you think is going on here.
>

Sometimes I am not sure what should be put into the cover letter.

Will adjust next time.

>I'm not convinced there's an issue here at all, and I'm not going to
>spend time on it after this cover letter.  It seems like you are also
>not sure there's a problem.
>
>If it's important enough to fix then it's important enough to explain
>what isn't correct.
>
>If it's not important enough to explain then don't bother fixing it.
>
>Thanks,
>Liam

-- 
Wei Yang
Help you, Help me