v2:
- Added r-bs for Lorenzo and Liam
- Reworded patch 1's commit message [Lorenzo]
This series contains two patches that fix vma merge/split for userfaultfd
on two separate issues. The patchset is based on akpm/mm-hotfixes-unstable
with 2f628010799e reverted (where patch 1 should be used to replace it
which seems to be the plan we reached).
Patch 1 fixes a regression since 6.1+ due to something we overlooked when
converting to maple tree apis. The plan is we use patch 1 to replace the
commit "2f628010799e (mm: userfaultfd: avoid passing an invalid range to
vma_merge())" in mm-hostfixes-unstable tree if possible, so as to bring
uffd vma operations back aligned with the rest code again.
Patch 2 fixes a long standing issue that vma can be left unmerged even if
we can for either uffd register or unregister.
Many thanks to Lorenzo on either noticing this issue from the assert
movement patch, looking at this problem, and also provided a reproducer on
the unmerged vma issue [1].
Please have a look, thanks.
[1] https://gist.github.com/lorenzo-stoakes/a11a10f5f479e7a977fc456331266e0e
Peter Xu (2):
mm/uffd: Fix vma operation where start addr cuts part of vma
mm/uffd: Allow vma to merge as much as possible
fs/userfaultfd.c | 13 +++++++++++--
1 file changed, 11 insertions(+), 2 deletions(-)
--
2.39.1
It seems vma merging with uffd paths is broken with either
register/unregister, where right now we can feed wrong parameters to
vma_merge() and it's found by recent patch which moved asserts upwards in
vma_merge() by Lorenzo Stoakes:
https://lore.kernel.org/all/[email protected]/
It's possible that "start" is contained within vma but not clamped to its
start. We need to convert this into either "cannot merge" case or "can
merge" case 4 which permits subdivision of prev by assigning vma to
prev. As we loop, each subsequent VMA will be clamped to the start.
This patch will eliminate the report and make sure vma_merge() calls will
become legal again.
One thing to mention is that the "Fixes: 29417d292bd0" below is there only
to help explain where the warning can start to trigger, the real commit to
fix should be 69dbe6daf104. Commit 29417d292bd0 helps us to identify the
issue, but unfortunately we may want to keep it in Fixes too just to ease
kernel backporters for easier tracking.
Cc: Lorenzo Stoakes <[email protected]>
Cc: Mike Rapoport (IBM) <[email protected]>
Cc: Liam R. Howlett <[email protected]>
Reported-by: Mark Rutland <[email protected]>
Reviewed-by: Lorenzo Stoakes <[email protected]>
Reviewed-by: Liam R. Howlett <[email protected]>
Fixes: 29417d292bd0 ("mm/mmap/vma_merge: always check invariants")
Fixes: 69dbe6daf104 ("userfaultfd: use maple tree iterator to iterate VMAs")
Closes: https://lore.kernel.org/all/[email protected]/
Cc: linux-stable <[email protected]>
Signed-off-by: Peter Xu <[email protected]>
---
fs/userfaultfd.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/fs/userfaultfd.c b/fs/userfaultfd.c
index 0fd96d6e39ce..17c8c345dac4 100644
--- a/fs/userfaultfd.c
+++ b/fs/userfaultfd.c
@@ -1459,6 +1459,8 @@ static int userfaultfd_register(struct userfaultfd_ctx *ctx,
vma_iter_set(&vmi, start);
prev = vma_prev(&vmi);
+ if (vma->vm_start < start)
+ prev = vma;
ret = 0;
for_each_vma_range(vmi, vma, end) {
@@ -1625,6 +1627,9 @@ static int userfaultfd_unregister(struct userfaultfd_ctx *ctx,
vma_iter_set(&vmi, start);
prev = vma_prev(&vmi);
+ if (vma->vm_start < start)
+ prev = vma;
+
ret = 0;
for_each_vma_range(vmi, vma, end) {
cond_resched();
--
2.39.1
On Wed, 17 May 2023 15:09:15 -0400 Peter Xu <[email protected]> wrote:
> It seems vma merging with uffd paths is broken with either
> register/unregister, where right now we can feed wrong parameters to
> vma_merge() and it's found by recent patch which moved asserts upwards in
> vma_merge() by Lorenzo Stoakes:
>
> https://lore.kernel.org/all/[email protected]/
>
> It's possible that "start" is contained within vma but not clamped to its
> start. We need to convert this into either "cannot merge" case or "can
> merge" case 4 which permits subdivision of prev by assigning vma to
> prev. As we loop, each subsequent VMA will be clamped to the start.
>
> This patch will eliminate the report and make sure vma_merge() calls will
> become legal again.
>
> One thing to mention is that the "Fixes: 29417d292bd0" below is there only
> to help explain where the warning can start to trigger, the real commit to
> fix should be 69dbe6daf104. Commit 29417d292bd0 helps us to identify the
> issue, but unfortunately we may want to keep it in Fixes too just to ease
> kernel backporters for easier tracking.
>
> Cc: Lorenzo Stoakes <[email protected]>
> Cc: Mike Rapoport (IBM) <[email protected]>
> Cc: Liam R. Howlett <[email protected]>
> Reported-by: Mark Rutland <[email protected]>
> Reviewed-by: Lorenzo Stoakes <[email protected]>
> Reviewed-by: Liam R. Howlett <[email protected]>
> Fixes: 29417d292bd0 ("mm/mmap/vma_merge: always check invariants")
> Fixes: 69dbe6daf104 ("userfaultfd: use maple tree iterator to iterate VMAs")
I don't know how -stable maintainers are to handle more than a single
Fixes: target, given that Fixes: means "kernels which have that patch
need this one". Can we narrow this down to a single commit for this
purpose?
On Wed, May 17, 2023 at 01:23:21PM -0700, Andrew Morton wrote:
> On Wed, 17 May 2023 15:09:15 -0400 Peter Xu <[email protected]> wrote:
>
> > It seems vma merging with uffd paths is broken with either
> > register/unregister, where right now we can feed wrong parameters to
> > vma_merge() and it's found by recent patch which moved asserts upwards in
> > vma_merge() by Lorenzo Stoakes:
> >
> > https://lore.kernel.org/all/[email protected]/
> >
> > It's possible that "start" is contained within vma but not clamped to its
> > start. We need to convert this into either "cannot merge" case or "can
> > merge" case 4 which permits subdivision of prev by assigning vma to
> > prev. As we loop, each subsequent VMA will be clamped to the start.
> >
> > This patch will eliminate the report and make sure vma_merge() calls will
> > become legal again.
> >
> > One thing to mention is that the "Fixes: 29417d292bd0" below is there only
> > to help explain where the warning can start to trigger, the real commit to
> > fix should be 69dbe6daf104. Commit 29417d292bd0 helps us to identify the
> > issue, but unfortunately we may want to keep it in Fixes too just to ease
> > kernel backporters for easier tracking.
> >
> > Cc: Lorenzo Stoakes <[email protected]>
> > Cc: Mike Rapoport (IBM) <[email protected]>
> > Cc: Liam R. Howlett <[email protected]>
> > Reported-by: Mark Rutland <[email protected]>
> > Reviewed-by: Lorenzo Stoakes <[email protected]>
> > Reviewed-by: Liam R. Howlett <[email protected]>
> > Fixes: 29417d292bd0 ("mm/mmap/vma_merge: always check invariants")
> > Fixes: 69dbe6daf104 ("userfaultfd: use maple tree iterator to iterate VMAs")
>
> I don't know how -stable maintainers are to handle more than a single
> Fixes: target, given that Fixes: means "kernels which have that patch
> need this one". Can we narrow this down to a single commit for this
> purpose?
Please just keep:
Fixes: 69dbe6daf104 ("userfaultfd: use maple tree iterator to iterate VMAs")
I just noticed 29417d292bd0 is only in rc1 so no backport needed anyway.
We definitely need 69dbe6daf104 marked Fixes for backport till 6.1+.
Thanks,
--
Peter Xu