In remap_move() we must account for both a single VMA case, where we are
permitted to move a single VMA regardless of multi-VMA move eligiblity, and
multiple VMAs which, of course, must be eligible for such an operation.
We determine this via vma_multi_allowed().
Currently, if the first VMA is not eligible, but others are, we will move
the first then return an error. This is not ideal, as we are performing an
operation which we don't need to do which has an impact on the memory
mapping.
We can very easily determine if this is a multi VMA move prior to the move
of the first VMA, by checking vma->vm_end vs. the specified end address.
Therefore this patch does so, and as a result eliminates unnecessary logic
around tracking whether the first VMA was permitted or not.
This is most useful for cases where a user attempts to erroneously move
mutliple VMAs which are not eligible for non-transient reasons - for
instance, UFFD-armed VMAs, or file-backed VMAs backed by a file system or
driver which specifies a custom f_op->get_unmapped_area.
In the less likely instance of a failure due to transient issues such as
out of memory or mapping limits being hit, the issue is already likely
fatal and so the fact the operation may be partially complete is
acceptable.
Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
---
mm/mremap.c | 20 ++++++++++++--------
1 file changed, 12 insertions(+), 8 deletions(-)
diff --git a/mm/mremap.c b/mm/mremap.c
index 46f9f3160dff..f61a9ea0b244 100644
--- a/mm/mremap.c
+++ b/mm/mremap.c
@@ -1816,10 +1816,11 @@ static unsigned long remap_move(struct vma_remap_struct *vrm)
unsigned long start = vrm->addr;
unsigned long end = vrm->addr + vrm->old_len;
unsigned long new_addr = vrm->new_addr;
- bool allowed = true, seen_vma = false;
unsigned long target_addr = new_addr;
unsigned long res = -EFAULT;
unsigned long last_end;
+ bool seen_vma = false;
+
VMA_ITERATOR(vmi, current->mm, start);
/*
@@ -1833,9 +1834,6 @@ static unsigned long remap_move(struct vma_remap_struct *vrm)
unsigned long len = min(end, vma->vm_end) - addr;
unsigned long offset, res_vma;
- if (!allowed)
- return -EFAULT;
-
/* No gap permitted at the start of the range. */
if (!seen_vma && start < vma->vm_start)
return -EFAULT;
@@ -1863,9 +1861,14 @@ static unsigned long remap_move(struct vma_remap_struct *vrm)
vrm->new_addr = target_addr + offset;
vrm->old_len = vrm->new_len = len;
- allowed = vma_multi_allowed(vma);
- if (seen_vma && !allowed)
- return -EFAULT;
+ if (!vma_multi_allowed(vma)) {
+ /* This is not the first VMA, abort immediately. */
+ if (seen_vma)
+ return -EFAULT;
+ /* This is the first, but there are more, abort. */
+ if (vma->vm_end < end)
+ return -EFAULT;
+ }
res_vma = check_prep_vma(vrm);
if (!res_vma)
@@ -1874,7 +1877,8 @@ static unsigned long remap_move(struct vma_remap_struct *vrm)
return res_vma;
if (!seen_vma) {
- VM_WARN_ON_ONCE(allowed && res_vma != new_addr);
+ VM_WARN_ON_ONCE(vma_multi_allowed(vma) &&
+ res_vma != new_addr);
res = res_vma;
}
--
2.50.1Hi Andrew,
Fixing a silly issue that syzbot picked up, I reuse vma incorrectly, very easy
fix, fix-patch below.
(Vlastimil had a look at this off-list).
Cheers, Lorenzo
----8<----
From 87fc8e42946938688d637f694cd6e80552a26667 Mon Sep 17 00:00:00 2001
From: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
Date: Sat, 16 Aug 2025 08:37:41 +0100
Subject: [PATCH] mm/mremap: do not incorrectly reference invalid VMA in
VM_WARN_ON_ONCE()
The VMA which is referenced here may have since been merged (which is the
entire point of the warning), and yet we still reference it.
Fix this by storing whether or not a multi move is permitted ahead of time
and have the VM_WARN_ON_ONCE() be predicated on this.
Reported-by: syzbot+4e221abf50259362f4f4@syzkaller.appspotmail.com
Closes: https://lore.kernel.org/linux-mm/689ff5f6.050a0220.e29e5.0030.GAE@google.com/
Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
Reviewed-by: Vlastimil Babka <vbabka@suse.cz>
---
mm/mremap.c | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/mm/mremap.c b/mm/mremap.c
index 18aa0b3b828f..33b642076205 100644
--- a/mm/mremap.c
+++ b/mm/mremap.c
@@ -1837,6 +1837,7 @@ static unsigned long remap_move(struct vma_remap_struct *vrm)
unsigned long addr = max(vma->vm_start, start);
unsigned long len = min(end, vma->vm_end) - addr;
unsigned long offset, res_vma;
+ bool multi_allowed;
/* No gap permitted at the start of the range. */
if (!seen_vma && start < vma->vm_start)
@@ -1865,7 +1866,8 @@ static unsigned long remap_move(struct vma_remap_struct *vrm)
vrm->new_addr = target_addr + offset;
vrm->old_len = vrm->new_len = len;
- if (!vma_multi_allowed(vma)) {
+ multi_allowed = vma_multi_allowed(vma);
+ if (!multi_allowed) {
/* This is not the first VMA, abort immediately. */
if (seen_vma)
return -EFAULT;
@@ -1881,8 +1883,7 @@ static unsigned long remap_move(struct vma_remap_struct *vrm)
return res_vma;
if (!seen_vma) {
- VM_WARN_ON_ONCE(vma_multi_allowed(vma) &&
- res_vma != new_addr);
+ VM_WARN_ON_ONCE(multi_allowed && res_vma != new_addr);
res = res_vma;
}
--
2.50.1
On 8/3/25 13:11, Lorenzo Stoakes wrote: > In remap_move() we must account for both a single VMA case, where we are > permitted to move a single VMA regardless of multi-VMA move eligiblity, and > multiple VMAs which, of course, must be eligible for such an operation. > > We determine this via vma_multi_allowed(). > > Currently, if the first VMA is not eligible, but others are, we will move > the first then return an error. This is not ideal, as we are performing an > operation which we don't need to do which has an impact on the memory > mapping. > > We can very easily determine if this is a multi VMA move prior to the move > of the first VMA, by checking vma->vm_end vs. the specified end address. > > Therefore this patch does so, and as a result eliminates unnecessary logic > around tracking whether the first VMA was permitted or not. > > This is most useful for cases where a user attempts to erroneously move > mutliple VMAs which are not eligible for non-transient reasons - for > instance, UFFD-armed VMAs, or file-backed VMAs backed by a file system or > driver which specifies a custom f_op->get_unmapped_area. > > In the less likely instance of a failure due to transient issues such as > out of memory or mapping limits being hit, the issue is already likely > fatal and so the fact the operation may be partially complete is > acceptable. > > Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com> With the updated commit log, Reviewed-by: Vlastimil Babka <vbabka@suse.cz>
Andrew, please adjust commit message as follows:
On Sun, Aug 03, 2025 at 12:11:22PM +0100, Lorenzo Stoakes wrote:
> In remap_move() we must account for both a single VMA case, where we are
> permitted to move a single VMA regardless of multi-VMA move eligiblity, and
> multiple VMAs which, of course, must be eligible for such an operation.
>
> We determine this via vma_multi_allowed().
>
> Currently, if the first VMA is not eligible, but others are, we will move
> the first then return an error. This is not ideal, as we are performing an
> operation which we don't need to do which has an impact on the memory
> mapping.
>
> We can very easily determine if this is a multi VMA move prior to the move
> of the first VMA, by checking vma->vm_end vs. the specified end address.
>
> Therefore this patch does so, and as a result eliminates unnecessary logic
> around tracking whether the first VMA was permitted or not.
>
> This is most useful for cases where a user attempts to erroneously move
> mutliple VMAs which are not eligible for non-transient reasons - for
> instance, UFFD-armed VMAs, or file-backed VMAs backed by a file system or
> driver which specifies a custom f_op->get_unmapped_area.
>
> In the less likely instance of a failure due to transient issues such as
> out of memory or mapping limits being hit, the issue is already likely
> fatal and so the fact the operation may be partially complete is
> acceptable.
>
Previously, any attempt to solely move a VMA would require that the span
specified reside within the span of that single VMA, with no gaps before or
afterwards.
After commit d23cb648e365 ("mm/mremap: permit mremap() move of multiple
VMAs"), the multi VMA move permitted a gap to exist only after VMAs. This
was done to provide maximum flexibility.
However, We have consequently permitted this behaviour for the move of a
single VMA including those not eligible for multi VMA move.
The change introduced here means that we no longer permit non-eligible VMAs
from being moved in this way.
This is consistent, as it means all eligible VMA moves are treated the
same, and all non-eligible moves are treated as they were before.
This change does not break previous behaviour, which equally would have
disallowed such a move (only in all cases).
> Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
On 8/3/25 13:11, Lorenzo Stoakes wrote:
> In remap_move() we must account for both a single VMA case, where we are
> permitted to move a single VMA regardless of multi-VMA move eligiblity, and
> multiple VMAs which, of course, must be eligible for such an operation.
>
> We determine this via vma_multi_allowed().
>
> Currently, if the first VMA is not eligible, but others are, we will move
> the first then return an error. This is not ideal, as we are performing an
> operation which we don't need to do which has an impact on the memory
> mapping.
>
> We can very easily determine if this is a multi VMA move prior to the move
> of the first VMA, by checking vma->vm_end vs. the specified end address.
>
> Therefore this patch does so, and as a result eliminates unnecessary logic
> around tracking whether the first VMA was permitted or not.
>
> This is most useful for cases where a user attempts to erroneously move
> mutliple VMAs which are not eligible for non-transient reasons - for
> instance, UFFD-armed VMAs, or file-backed VMAs backed by a file system or
> driver which specifies a custom f_op->get_unmapped_area.
>
> In the less likely instance of a failure due to transient issues such as
> out of memory or mapping limits being hit, the issue is already likely
> fatal and so the fact the operation may be partially complete is
> acceptable.
>
> Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
> ---
> mm/mremap.c | 20 ++++++++++++--------
> 1 file changed, 12 insertions(+), 8 deletions(-)
>
> diff --git a/mm/mremap.c b/mm/mremap.c
> index 46f9f3160dff..f61a9ea0b244 100644
> --- a/mm/mremap.c
> +++ b/mm/mremap.c
> @@ -1816,10 +1816,11 @@ static unsigned long remap_move(struct vma_remap_struct *vrm)
> unsigned long start = vrm->addr;
> unsigned long end = vrm->addr + vrm->old_len;
> unsigned long new_addr = vrm->new_addr;
> - bool allowed = true, seen_vma = false;
> unsigned long target_addr = new_addr;
> unsigned long res = -EFAULT;
> unsigned long last_end;
> + bool seen_vma = false;
> +
> VMA_ITERATOR(vmi, current->mm, start);
>
> /*
> @@ -1833,9 +1834,6 @@ static unsigned long remap_move(struct vma_remap_struct *vrm)
> unsigned long len = min(end, vma->vm_end) - addr;
> unsigned long offset, res_vma;
>
> - if (!allowed)
> - return -EFAULT;
> -
> /* No gap permitted at the start of the range. */
> if (!seen_vma && start < vma->vm_start)
> return -EFAULT;
> @@ -1863,9 +1861,14 @@ static unsigned long remap_move(struct vma_remap_struct *vrm)
> vrm->new_addr = target_addr + offset;
> vrm->old_len = vrm->new_len = len;
>
> - allowed = vma_multi_allowed(vma);
> - if (seen_vma && !allowed)
> - return -EFAULT;
> + if (!vma_multi_allowed(vma)) {
> + /* This is not the first VMA, abort immediately. */
> + if (seen_vma)
> + return -EFAULT;
> + /* This is the first, but there are more, abort. */
> + if (vma->vm_end < end)
> + return -EFAULT;
Hm there can just also be a gap, and we permit gaps at the end (unlike at
the start), right?
So we might be denying a multi vma mremap for !vma_multi_allowed() reasons
even if it's a single vma and a gap.
AFAICS this is not regressing the behavior prior to d23cb648e365
("mm/mremap: permit mremap() move of multiple VMAs") as such mremap() would
be denied anyway by the "/* We can't remap across vm area boundaries */"
check in check_prep_vma().
So the question is just if we want this odd corner case to behave like this,
and if yes then be more explicit about it perhaps.
> + }
>
> res_vma = check_prep_vma(vrm);
> if (!res_vma)
> @@ -1874,7 +1877,8 @@ static unsigned long remap_move(struct vma_remap_struct *vrm)
> return res_vma;
>
> if (!seen_vma) {
> - VM_WARN_ON_ONCE(allowed && res_vma != new_addr);
> + VM_WARN_ON_ONCE(vma_multi_allowed(vma) &&
> + res_vma != new_addr);
> res = res_vma;
> }
>
On Fri, Aug 08, 2025 at 04:19:09PM +0200, Vlastimil Babka wrote:
> On 8/3/25 13:11, Lorenzo Stoakes wrote:
[snip]
> > Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
> > ---
> > mm/mremap.c | 20 ++++++++++++--------
> > 1 file changed, 12 insertions(+), 8 deletions(-)
[snip]
> > @@ -1863,9 +1861,14 @@ static unsigned long remap_move(struct vma_remap_struct *vrm)
> > vrm->new_addr = target_addr + offset;
> > vrm->old_len = vrm->new_len = len;
> >
> > - allowed = vma_multi_allowed(vma);
> > - if (seen_vma && !allowed)
> > - return -EFAULT;
> > + if (!vma_multi_allowed(vma)) {
> > + /* This is not the first VMA, abort immediately. */
> > + if (seen_vma)
> > + return -EFAULT;
> > + /* This is the first, but there are more, abort. */
> > + if (vma->vm_end < end)
> > + return -EFAULT;
>
> Hm there can just also be a gap, and we permit gaps at the end (unlike at
> the start), right?
I don't think we should allow a single VMA with gap, it's actually more
correct to maintain existing behavour in this case.
> So we might be denying a multi vma mremap for !vma_multi_allowed()
> reasons even if it's a single vma and a gap.
This is therfore a useful exercise in preventing us from permitting this
case I think.
>
> AFAICS this is not regressing the behavior prior to d23cb648e365
> ("mm/mremap: permit mremap() move of multiple VMAs") as such mremap() would
> be denied anyway by the "/* We can't remap across vm area boundaries */"
> check in check_prep_vma().
Yup.
And this code is _only_ called for MREMAP_FIXED. So nothing else is impacted.
>
> So the question is just if we want this odd corner case to behave like this,
> and if yes then be more explicit about it perhaps.
We definitely do IMO. There's no reason to change this behaviour.
The end gap thing in multi was more a product of 'why not permit it' but
now is more a case of 'it means we don't have to go check or fail
partially'.
So I think this is fine.
>
> > + }
> >
> > res_vma = check_prep_vma(vrm);
> > if (!res_vma)
> > @@ -1874,7 +1877,8 @@ static unsigned long remap_move(struct vma_remap_struct *vrm)
> > return res_vma;
> >
> > if (!seen_vma) {
> > - VM_WARN_ON_ONCE(allowed && res_vma != new_addr);
> > + VM_WARN_ON_ONCE(vma_multi_allowed(vma) &&
> > + res_vma != new_addr);
> > res = res_vma;
> > }
> >
>
I can update the commit msg accordingly...
On Fri, Aug 08, 2025 at 03:34:13PM +0100, Lorenzo Stoakes wrote:
> On Fri, Aug 08, 2025 at 04:19:09PM +0200, Vlastimil Babka wrote:
> > On 8/3/25 13:11, Lorenzo Stoakes wrote:
> [snip]
> > > Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
> > > ---
> > > mm/mremap.c | 20 ++++++++++++--------
> > > 1 file changed, 12 insertions(+), 8 deletions(-)
> [snip]
> > > @@ -1863,9 +1861,14 @@ static unsigned long remap_move(struct vma_remap_struct *vrm)
> > > vrm->new_addr = target_addr + offset;
> > > vrm->old_len = vrm->new_len = len;
> > >
> > > - allowed = vma_multi_allowed(vma);
> > > - if (seen_vma && !allowed)
> > > - return -EFAULT;
> > > + if (!vma_multi_allowed(vma)) {
> > > + /* This is not the first VMA, abort immediately. */
> > > + if (seen_vma)
> > > + return -EFAULT;
> > > + /* This is the first, but there are more, abort. */
> > > + if (vma->vm_end < end)
> > > + return -EFAULT;
> >
> > Hm there can just also be a gap, and we permit gaps at the end (unlike at
> > the start), right?
>
> I don't think we should allow a single VMA with gap, it's actually more
> correct to maintain existing behavour in this case.
>
> > So we might be denying a multi vma mremap for !vma_multi_allowed()
> > reasons even if it's a single vma and a gap.
>
> This is therfore a useful exercise in preventing us from permitting this
> case I think.
>
> >
> > AFAICS this is not regressing the behavior prior to d23cb648e365
> > ("mm/mremap: permit mremap() move of multiple VMAs") as such mremap() would
> > be denied anyway by the "/* We can't remap across vm area boundaries */"
> > check in check_prep_vma().
>
> Yup.
>
> And this code is _only_ called for MREMAP_FIXED. So nothing else is impacted.
>
> >
> > So the question is just if we want this odd corner case to behave like this,
> > and if yes then be more explicit about it perhaps.
>
> We definitely do IMO. There's no reason to change this behaviour.
>
> The end gap thing in multi was more a product of 'why not permit it' but
> now is more a case of 'it means we don't have to go check or fail
> partially'.
>
> So I think this is fine.
>
> >
> > > + }
> > >
> > > res_vma = check_prep_vma(vrm);
> > > if (!res_vma)
> > > @@ -1874,7 +1877,8 @@ static unsigned long remap_move(struct vma_remap_struct *vrm)
> > > return res_vma;
> > >
> > > if (!seen_vma) {
> > > - VM_WARN_ON_ONCE(allowed && res_vma != new_addr);
> > > + VM_WARN_ON_ONCE(vma_multi_allowed(vma) &&
> > > + res_vma != new_addr);
> > > res = res_vma;
> > > }
> > >
> >
>
> I can update the commit msg accordingly...
I have asked Andrew to update with a clear explanation of this (see [0]), and
made clear why I feel it's consistent for us to disallow this behaviour for
non-eligible VMAs while permitting it for eligible ones.
It means we can simply say 'for eligible pure moves, you may specify gaps
between or after VMAs spanning 1 or more VMAs'.
Cheers, Lorenzo
[0]: https://lore.kernel.org/linux-mm/df80b788-0546-4b78-a2fa-64d26e5a35b8@lucifer.local/
© 2016 - 2025 Red Hat, Inc.