[PATCH 0/2] mremap: Fix newaddr hint with MREMAP_DONTUNMAP

[PATCH 0/2] mremap: Fix newaddr hint with MREMAP_DONTUNMAP

[Brian Geffon]

mmap(2) allows for a destination address to be specified without
MAP_FIXED and in this situation it's a hint to get_unmapped_area().
This address need not be page aligned because get_unmapped_area() will
align the hint.

In the case of mremap(2) with MREMAP_DONTUNMAP it shares a code path
with MREMAP_FIXED in mremap_to(), which means this function can be
called in 3 different scenarios: MREMAP_FIXED only, MREMAP_DONTUNMAP
only, or MREMAP_FIXED | MREMAP_DONTUNMAP. In the second case when only
MREMAP_DONTUNMAP is specified we don't need to do alignment or size
checks on newaddr because they will be passed to get_unmapped_area() and
dealt with appropriately.

This patch corrects that behavior to match what non-MREMAP_DONTUNMAP
mremap(2) and mmap(2) do. This odd behavioral difference was reported by
Marco Vanotti. Additionally, I've included a self test to validate this
behavior.

Brian Geffon (2):
  mremap: Fix new_addr being used as a hint with MREMAP_DONTUNMAP
  selftests: mm: Add a new MREMAP_DONTUNMAP self test

 mm/mremap.c                                   | 26 ++++++++----
 tools/testing/selftests/mm/mremap_dontunmap.c | 41 ++++++++++++++++++-
 2 files changed, 59 insertions(+), 8 deletions(-)

-- 
2.47.0.338.g60cca15819-goog

[PATCH 1/2] mremap: Fix new_addr being used as a hint with MREMAP_DONTUNMAP

[Brian Geffon]

Two non-mutually exclusive paths can land in mremap_to, MREMAP_FIXED
and MREMAP_DONTUNMAP which are called from mremap(). In the case of
MREMAP_FIXED we must validate the new_addr to ensure that the new
address is valid. In the case of MREMAP_DONTUNMAP without MREMAP_FIXED
a new address is specified as a hint, just like it would be in the
case of mmap. In this second case we don't need to perform any checks
because get_unmapped_area() will align new_addr, just like it would in
the case of mmap.

Signed-off-by: Brian Geffon <bgeffon@google.com>
Reported-by: Marco Vanotti <mvanotti@google.com>
---
 mm/mremap.c | 26 +++++++++++++++++++-------
 1 file changed, 19 insertions(+), 7 deletions(-)

diff --git a/mm/mremap.c b/mm/mremap.c
index 60473413836b..286ffdb883df 100644
--- a/mm/mremap.c
+++ b/mm/mremap.c
@@ -912,15 +912,27 @@ static unsigned long mremap_to(unsigned long addr, unsigned long old_len,
 	unsigned long ret;
 	unsigned long map_flags = 0;
 
-	if (offset_in_page(new_addr))
-		return -EINVAL;
+	/*
+	 * Two non-mutually exclusive paths can land in mremap_to, MREMAP_FIXED
+	 * and MREMAP_DONTUNMAP which are called from mremap(). In the case of
+	 * MREMAP_FIXED we must validate the new_addr to ensure that the new
+	 * address is valid. In the case of MREMAP_DONTUNMAP without MREMAP_FIXED
+	 * a new address is specified as a hint, just like it would be in the
+	 * case of mmap. In this second case we don't need to perform any checks
+	 * because get_unmapped_area() will align new_addr, just like it would in
+	 * the case of mmap.
+	 */
+	if (flags & MREMAP_FIXED) {
+		if (offset_in_page(new_addr))
+			return -EINVAL;
 
-	if (new_len > TASK_SIZE || new_addr > TASK_SIZE - new_len)
-		return -EINVAL;
+		if (new_len > TASK_SIZE || new_addr > TASK_SIZE - new_len)
+			return -EINVAL;
 
-	/* Ensure the old/new locations do not overlap */
-	if (addr + old_len > new_addr && new_addr + new_len > addr)
-		return -EINVAL;
+		/* Ensure the old/new locations do not overlap */
+		if (addr + old_len > new_addr && new_addr + new_len > addr)
+			return -EINVAL;
+	}
 
 	/*
 	 * move_vma() need us to stay 4 maps below the threshold, otherwise
-- 
2.47.0.338.g60cca15819-goog

[PATCH 2/2] selftests: mm: Add a new MREMAP_DONTUNMAP self test

[Brian Geffon]

Add a new selftest which validates that a new_addr as a hint behaves
in the same way as mmap.

Signed-off-by: Brian Geffon <bgeffon@google.com>
---
 tools/testing/selftests/mm/mremap_dontunmap.c | 41 ++++++++++++++++++-
 1 file changed, 40 insertions(+), 1 deletion(-)

diff --git a/tools/testing/selftests/mm/mremap_dontunmap.c b/tools/testing/selftests/mm/mremap_dontunmap.c
index 1d75084b9ca5..ccce97c68019 100644
--- a/tools/testing/selftests/mm/mremap_dontunmap.c
+++ b/tools/testing/selftests/mm/mremap_dontunmap.c
@@ -224,6 +224,44 @@ static void mremap_dontunmap_simple_fixed()
 	ksft_test_result_pass("%s\n", __func__);
 }
 
+// This test validates MREMAP_DONTUNMAP using a newaddr hint without
+// MREMAP_FIXED.
+static void mremap_dontunmap_simple_newaddr_hint()
+{
+	unsigned long num_pages = 5;
+
+	// This dest hint is intentionally not aligned.
+	void *new_addr_hint = (void*)0x999900010;
+
+	void *source_mapping =
+	    mmap(NULL, num_pages * page_size, PROT_READ | PROT_WRITE,
+		 MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
+	BUG_ON(source_mapping == MAP_FAILED, "mmap");
+	memset(source_mapping, 'a', num_pages * page_size);
+
+	void *remapped_mapping =
+	    mremap(source_mapping, num_pages * page_size, num_pages * page_size,
+		   MREMAP_DONTUNMAP | MREMAP_MAYMOVE,
+		   new_addr_hint);
+	BUG_ON(remapped_mapping == MAP_FAILED, "mremap");
+
+	// And the source mapping will have had its ptes dropped.
+	BUG_ON(check_region_contains_byte
+	       (source_mapping, num_pages * page_size, 0) != 0,
+	       "source should have no ptes");
+
+	// And the remapped area will be filled with 'a's.
+	BUG_ON(check_region_contains_byte
+	       (remapped_mapping, num_pages * page_size, 'a') != 0,
+	       "dest should have remapped content");
+
+	BUG_ON(munmap(source_mapping, num_pages * page_size) == -1,
+	       "unable to unmap source mapping");
+	BUG_ON(munmap(remapped_mapping, num_pages * page_size) == -1,
+	       "unable to unmap source mapping");
+	ksft_test_result_pass("%s\n", __func__);
+}
+
 // This test validates that we can MREMAP_DONTUNMAP for a portion of an
 // existing mapping.
 static void mremap_dontunmap_partial_mapping()
@@ -348,7 +386,7 @@ int main(void)
 		ksft_finished();
 	}
 
-	ksft_set_plan(5);
+	ksft_set_plan(6);
 
 	// Keep a page sized buffer around for when we need it.
 	page_buffer =
@@ -359,6 +397,7 @@ int main(void)
 	mremap_dontunmap_simple();
 	mremap_dontunmap_simple_shmem();
 	mremap_dontunmap_simple_fixed();
+	mremap_dontunmap_simple_newaddr_hint();
 	mremap_dontunmap_partial_mapping();
 	mremap_dontunmap_partial_mapping_overwrite();
 
-- 
2.47.0.338.g60cca15819-goog

Re: [PATCH 1/2] mremap: Fix new_addr being used as a hint with MREMAP_DONTUNMAP

[Marco Vanotti]

[-- Attachment #1: Type: text/plain, Size: 2817 bytes --]

On Fri, Dec 6, 2024 at 12:20 PM Brian Geffon <bgeffon@google.com> wrote:
>
> Two non-mutually exclusive paths can land in mremap_to, MREMAP_FIXED
> and MREMAP_DONTUNMAP which are called from mremap(). In the case of
> MREMAP_FIXED we must validate the new_addr to ensure that the new
> address is valid. In the case of MREMAP_DONTUNMAP without MREMAP_FIXED
> a new address is specified as a hint, just like it would be in the
> case of mmap. In this second case we don't need to perform any checks
> because get_unmapped_area() will align new_addr, just like it would in
> the case of mmap.
>
> Signed-off-by: Brian Geffon <bgeffon@google.com>
> Reported-by: Marco Vanotti <mvanotti@google.com>
> ---
>  mm/mremap.c | 26 +++++++++++++++++++-------
>  1 file changed, 19 insertions(+), 7 deletions(-)
>
> diff --git a/mm/mremap.c b/mm/mremap.c
> index 60473413836b..286ffdb883df 100644
> --- a/mm/mremap.c
> +++ b/mm/mremap.c
> @@ -912,15 +912,27 @@ static unsigned long mremap_to(unsigned long addr, unsigned long old_len,
>         unsigned long ret;
>         unsigned long map_flags = 0;
>
> -       if (offset_in_page(new_addr))
> -               return -EINVAL;
> +       /*
> +        * Two non-mutually exclusive paths can land in mremap_to, MREMAP_FIXED
> +        * and MREMAP_DONTUNMAP which are called from mremap(). In the case of
> +        * MREMAP_FIXED we must validate the new_addr to ensure that the new
> +        * address is valid. In the case of MREMAP_DONTUNMAP without MREMAP_FIXED
> +        * a new address is specified as a hint, just like it would be in the
> +        * case of mmap. In this second case we don't need to perform any checks
> +        * because get_unmapped_area() will align new_addr, just like it would in
> +        * the case of mmap.
> +        */
A few lines below we also check for MREMAP_FIXED before calling
do_unmap, can't we do the validation there?
> +       if (flags & MREMAP_FIXED) {
> +               if (offset_in_page(new_addr))
> +                       return -EINVAL;
>
> -       if (new_len > TASK_SIZE || new_addr > TASK_SIZE - new_len)
> -               return -EINVAL;
> +               if (new_len > TASK_SIZE || new_addr > TASK_SIZE - new_len)
> +                       return -EINVAL;
>
> -       /* Ensure the old/new locations do not overlap */
> -       if (addr + old_len > new_addr && new_addr + new_len > addr)
> -               return -EINVAL;
> +               /* Ensure the old/new locations do not overlap */
> +               if (addr + old_len > new_addr && new_addr + new_len > addr)
> +                       return -EINVAL;
> +       }
>
>         /*
>          * move_vma() need us to stay 4 maps below the threshold, otherwise
> --
> 2.47.0.338.g60cca15819-goog
>

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 5294 bytes --]

Re: [PATCH 1/2] mremap: Fix new_addr being used as a hint with MREMAP_DONTUNMAP

[Brian Geffon]

On Fri, Dec 6, 2024 at 10:35 AM Marco Vanotti <mvanotti@google.com> wrote:
>
> On Fri, Dec 6, 2024 at 12:20 PM Brian Geffon <bgeffon@google.com> wrote:
> >
> > Two non-mutually exclusive paths can land in mremap_to, MREMAP_FIXED
> > and MREMAP_DONTUNMAP which are called from mremap(). In the case of
> > MREMAP_FIXED we must validate the new_addr to ensure that the new
> > address is valid. In the case of MREMAP_DONTUNMAP without MREMAP_FIXED
> > a new address is specified as a hint, just like it would be in the
> > case of mmap. In this second case we don't need to perform any checks
> > because get_unmapped_area() will align new_addr, just like it would in
> > the case of mmap.
> >
> > Signed-off-by: Brian Geffon <bgeffon@google.com>
> > Reported-by: Marco Vanotti <mvanotti@google.com>
> > ---
> >  mm/mremap.c | 26 +++++++++++++++++++-------
> >  1 file changed, 19 insertions(+), 7 deletions(-)
> >
> > diff --git a/mm/mremap.c b/mm/mremap.c
> > index 60473413836b..286ffdb883df 100644
> > --- a/mm/mremap.c
> > +++ b/mm/mremap.c
> > @@ -912,15 +912,27 @@ static unsigned long mremap_to(unsigned long addr, unsigned long old_len,
> >         unsigned long ret;
> >         unsigned long map_flags = 0;
> >
> > -       if (offset_in_page(new_addr))
> > -               return -EINVAL;
> > +       /*
> > +        * Two non-mutually exclusive paths can land in mremap_to, MREMAP_FIXED
> > +        * and MREMAP_DONTUNMAP which are called from mremap(). In the case of
> > +        * MREMAP_FIXED we must validate the new_addr to ensure that the new
> > +        * address is valid. In the case of MREMAP_DONTUNMAP without MREMAP_FIXED
> > +        * a new address is specified as a hint, just like it would be in the
> > +        * case of mmap. In this second case we don't need to perform any checks
> > +        * because get_unmapped_area() will align new_addr, just like it would in
> > +        * the case of mmap.
> > +        */
> A few lines below we also check for MREMAP_FIXED before calling
> do_unmap, can't we do the validation there?

I don't have a strong preference either way. I can mail a v2 with this
suggestion.

> > +       if (flags & MREMAP_FIXED) {
> > +               if (offset_in_page(new_addr))
> > +                       return -EINVAL;
> >
> > -       if (new_len > TASK_SIZE || new_addr > TASK_SIZE - new_len)
> > -               return -EINVAL;
> > +               if (new_len > TASK_SIZE || new_addr > TASK_SIZE - new_len)
> > +                       return -EINVAL;
> >
> > -       /* Ensure the old/new locations do not overlap */
> > -       if (addr + old_len > new_addr && new_addr + new_len > addr)
> > -               return -EINVAL;
> > +               /* Ensure the old/new locations do not overlap */
> > +               if (addr + old_len > new_addr && new_addr + new_len > addr)
> > +                       return -EINVAL;
> > +       }
> >
> >         /*
> >          * move_vma() need us to stay 4 maps below the threshold, otherwise
> > --
> > 2.47.0.338.g60cca15819-goog
> >

Re: [PATCH 0/2] mremap: Fix newaddr hint with MREMAP_DONTUNMAP

[Jann Horn]

+mmap maintainers (maybe mm/mremap.c should be added to the file
pattern for "MEMORY MAPPING" in "MAINTAINERS"? I'm not sure)

On Fri, Dec 6, 2024 at 4:20 PM Brian Geffon <bgeffon@google.com> wrote:
> mmap(2) allows for a destination address to be specified without
> MAP_FIXED and in this situation it's a hint to get_unmapped_area().
> This address need not be page aligned because get_unmapped_area() will
> align the hint.
>
> In the case of mremap(2) with MREMAP_DONTUNMAP it shares a code path
> with MREMAP_FIXED in mremap_to(), which means this function can be
> called in 3 different scenarios: MREMAP_FIXED only, MREMAP_DONTUNMAP
> only, or MREMAP_FIXED | MREMAP_DONTUNMAP. In the second case when only
> MREMAP_DONTUNMAP is specified we don't need to do alignment or size
> checks on newaddr because they will be passed to get_unmapped_area() and
> dealt with appropriately.
>
> This patch corrects that behavior to match what non-MREMAP_DONTUNMAP
> mremap(2) and mmap(2) do. This odd behavioral difference was reported by
> Marco Vanotti. Additionally, I've included a self test to validate this
> behavior.

Marco pointed me to this; I had no idea mremap() had this undocumented
behavior where it takes a hint address. The mremap() manpage is
currently wrong about this, it sort of implies that the new_address
argument is only used if MREMAP_FIXED is set.

Marco also noticed that upstream glibc now assumes this behavior:
https://sourceware.org/git/?p=glibc.git;a=commit;h=6c40cb0e9f893d49dc7caee580a055de53562206

Debian also has a test that explicitly checks for this behavior:
https://sources.debian.org/src/glibc/2.40-4/debian/patches/git-updates.diff/?hl=22820#L22818

I guess it's too late to remove that behavior at this point, and the
right thing to do is to update the manpage?

Re: [PATCH 0/2] mremap: Fix newaddr hint with MREMAP_DONTUNMAP

[Lorenzo Stoakes]

On Fri, Dec 06, 2024 at 07:42:51PM +0100, Jann Horn wrote:
> +mmap maintainers (maybe mm/mremap.c should be added to the file
> pattern for "MEMORY MAPPING" in "MAINTAINERS"? I'm not sure)

Yeah I think it's actually right to group together _all_ VMA-related operations
under the VMA entry, because we have interaction between them all mprotect,
mlock, etc. etc. etc.

I will send a patch in a second for this, because we do keep getting bitten by
this.

>
> On Fri, Dec 6, 2024 at 4:20 PM Brian Geffon <bgeffon@google.com> wrote:
> > mmap(2) allows for a destination address to be specified without
> > MAP_FIXED and in this situation it's a hint to get_unmapped_area().
> > This address need not be page aligned because get_unmapped_area() will
> > align the hint.
> >
> > In the case of mremap(2) with MREMAP_DONTUNMAP it shares a code path
> > with MREMAP_FIXED in mremap_to(), which means this function can be
> > called in 3 different scenarios: MREMAP_FIXED only, MREMAP_DONTUNMAP
> > only, or MREMAP_FIXED | MREMAP_DONTUNMAP. In the second case when only
> > MREMAP_DONTUNMAP is specified we don't need to do alignment or size
> > checks on newaddr because they will be passed to get_unmapped_area() and
> > dealt with appropriately.
> >
> > This patch corrects that behavior to match what non-MREMAP_DONTUNMAP
> > mremap(2) and mmap(2) do. This odd behavioral difference was reported by
> > Marco Vanotti. Additionally, I've included a self test to validate this
> > behavior.

Yeah if this is user-facing - I don't think we can change this. Can we do any v2
as an RFC for now until we can get a chance to look at this? And please cc- the
VMA/mmap maintainers on future revisions (sorry this wasn't at all clear, we
need to update MAINTAINERS here).

Thanks!

>
> Marco pointed me to this; I had no idea mremap() had this undocumented
> behavior where it takes a hint address. The mremap() manpage is
> currently wrong about this, it sort of implies that the new_address
> argument is only used if MREMAP_FIXED is set.
>
> Marco also noticed that upstream glibc now assumes this behavior:
> https://sourceware.org/git/?p=glibc.git;a=commit;h=6c40cb0e9f893d49dc7caee580a055de53562206
>
> Debian also has a test that explicitly checks for this behavior:
> https://sources.debian.org/src/glibc/2.40-4/debian/patches/git-updates.diff/?hl=22820#L22818
>
> I guess it's too late to remove that behavior at this point, and the
> right thing to do is to update the manpage?

Yeah, if user-facing we can't fundamentally change behaviour even if it's
strange I'd say.

Re: [PATCH 0/2] mremap: Fix newaddr hint with MREMAP_DONTUNMAP

[Brian Geffon]

On Fri, Dec 6, 2024 at 10:52 AM Lorenzo Stoakes
<lorenzo.stoakes@oracle.com> wrote:
>
> On Fri, Dec 06, 2024 at 07:42:51PM +0100, Jann Horn wrote:
> > +mmap maintainers (maybe mm/mremap.c should be added to the file
> > pattern for "MEMORY MAPPING" in "MAINTAINERS"? I'm not sure)
>
> Yeah I think it's actually right to group together _all_ VMA-related operations
> under the VMA entry, because we have interaction between them all mprotect,
> mlock, etc. etc. etc.
>
> I will send a patch in a second for this, because we do keep getting bitten by
> this.
>
> >
> > On Fri, Dec 6, 2024 at 4:20 PM Brian Geffon <bgeffon@google.com> wrote:
> > > mmap(2) allows for a destination address to be specified without
> > > MAP_FIXED and in this situation it's a hint to get_unmapped_area().
> > > This address need not be page aligned because get_unmapped_area() will
> > > align the hint.
> > >
> > > In the case of mremap(2) with MREMAP_DONTUNMAP it shares a code path
> > > with MREMAP_FIXED in mremap_to(), which means this function can be
> > > called in 3 different scenarios: MREMAP_FIXED only, MREMAP_DONTUNMAP
> > > only, or MREMAP_FIXED | MREMAP_DONTUNMAP. In the second case when only
> > > MREMAP_DONTUNMAP is specified we don't need to do alignment or size
> > > checks on newaddr because they will be passed to get_unmapped_area() and
> > > dealt with appropriately.
> > >
> > > This patch corrects that behavior to match what non-MREMAP_DONTUNMAP
> > > mremap(2) and mmap(2) do. This odd behavioral difference was reported by
> > > Marco Vanotti. Additionally, I've included a self test to validate this
> > > behavior.
>
> Yeah if this is user-facing - I don't think we can change this. Can we do any v2
> as an RFC for now until we can get a chance to look at this? And please cc- the
> VMA/mmap maintainers on future revisions (sorry this wasn't at all clear, we
> need to update MAINTAINERS here).

Sure, I'll mail the next series as an RFC in the next few days. This
behavior was not introduced intentionally.

>
> Thanks!
>
> >
> > Marco pointed me to this; I had no idea mremap() had this undocumented
> > behavior where it takes a hint address. The mremap() manpage is
> > currently wrong about this, it sort of implies that the new_address
> > argument is only used if MREMAP_FIXED is set.
> >
> > Marco also noticed that upstream glibc now assumes this behavior:
> > https://sourceware.org/git/?p=glibc.git;a=commit;h=6c40cb0e9f893d49dc7caee580a055de53562206
> >
> > Debian also has a test that explicitly checks for this behavior:
> > https://sources.debian.org/src/glibc/2.40-4/debian/patches/git-updates.diff/?hl=22820#L22818
> >
> > I guess it's too late to remove that behavior at this point, and the
> > right thing to do is to update the manpage?
>
> Yeah, if user-facing we can't fundamentally change behaviour even if it's
> strange I'd say.

Definitely, no matter what happens we'll need a man page update. I
think to make things consistent we'll probably want to consider
allowing all variants of mremap(2) (without MREMAP_FIXED) to use
newaddr as a hint, like mmap(2). But I'll mail the RFC with much more
detail in the cover letter about the history and impact.

Brian