* Re: [PATCH] mm: Fix mmap_assert_locked() in follow_pte()
@ 2024-07-12 12:17 Bert Karwatzki
2024-07-12 12:17 ` commit 1c29a32ce65f4cd0f1c causes Bad rss-counter state and firefox-esr crash in linux-next-20240613 Bert Karwatzki
0 siblings, 1 reply; 5+ messages in thread
From: Bert Karwatzki @ 2024-07-12 12:17 UTC (permalink / raw)
To: Pei Li
Cc: Bert Karwatzki, Andrew Morton, Shuah Khan, David Hildenbrand,
linux-mm, linux-kernel, linux-next, syzbot+35a4414f6e247f515443,
syzkaller-bugs, linux-kernel-mentees, senozhatsky
This is causing a deadlock for me, too. Since linux-next-20240712 I observe
a hang when starting the gui. I bisected this to commit a13252049629 and
reverting this commit in linux-next-20240712 fixes the issue for me.
I do not have any log messages to prove the dead lock, though, as I compiled
everything without CONFIG_LOCKDEP.
Bert Karwatzki
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: commit 1c29a32ce65f4cd0f1c causes Bad rss-counter state and firefox-esr crash in linux-next-20240613
2024-07-12 12:17 [PATCH] mm: Fix mmap_assert_locked() in follow_pte() Bert Karwatzki
@ 2024-07-12 12:17 ` Bert Karwatzki
2024-07-12 15:38 ` Liam R. Howlett
0 siblings, 1 reply; 5+ messages in thread
From: Bert Karwatzki @ 2024-07-12 12:17 UTC (permalink / raw)
To: Liam R . Howlett
Cc: Bert Karwatzki, Andrew Morton, Jiri Olsa, linux-mm, linux-kernel,
linux-next
I did some experiments on the rss counter bug. The next patch is made for linux-next-20240613
with commit 1c29a32ce65f4cd0f1c0f9 reverted. Then I simply inlined the code of do_vmi_unmap()
and do_vmi_align_munmap() into mmap_region(). This version of the code works fine and does not
show the rss counter bug.
diff --git a/mm/mmap.c b/mm/mmap.c
index f95af72ddc9f..0f020c535c83 100644
--- a/mm/mmap.c
+++ b/mm/mmap.c
@@ -733,7 +733,6 @@ int vma_expand(struct vma_iterator *vmi, struct vm_area_struct *vma,
vma_iter_store(vmi, vma);
vma_complete(&vp, vmi, vma->vm_mm);
- validate_mm(vma->vm_mm);
return 0;
nomem:
@@ -2911,6 +2910,8 @@ unsigned long mmap_region(struct file *file, unsigned long addr,
struct vm_area_struct *next, *prev, *merge;
pgoff_t pglen = len >> PAGE_SHIFT;
unsigned long charged = 0;
+ struct vma_munmap_struct vms;
+ struct ma_state mas_detach;
unsigned long end = addr + len;
unsigned long merge_start = addr, merge_end = end;
bool writable_file_mapping = false;
@@ -2933,12 +2934,46 @@ unsigned long mmap_region(struct file *file, unsigned long addr,
return -ENOMEM;
}
- /* Unmap any existing mapping in the area */
- error = do_vmi_munmap(&vmi, mm, addr, len, uf, false);
- if (error == -EPERM)
- return error;
- else if (error)
- return -ENOMEM;
+ /* Find the first overlapping VMA */
+ vma = vma_find(&vmi, end);
+ if (vma) {
+ struct maple_tree mt_detach;
+
+ /*
+ * Check if memory is sealed before arch_unmap.
+ * Prevent unmapping a sealed VMA.
+ * can_modify_mm assumes we have acquired the lock on MM.
+ */
+ if (unlikely(!can_modify_mm(mm, addr, end))) {
+ return -EPERM;
+ }
+
+ /* arch_unmap() might do unmaps itself. */
+ arch_unmap(mm, addr, end);
+
+ mt_init_flags(&mt_detach, vmi.mas.tree->ma_flags & MT_FLAGS_LOCK_MASK);
+ mt_on_stack(mt_detach);
+ mas_init(&mas_detach, &mt_detach, 0);
+
+ init_vma_munmap(&vms, &vmi, vma, addr, end, uf, false);
+ error = vms_gather_munmap_vmas(&vms, &mas_detach);
+ if (error) {
+ validate_mm(mm);
+ return -ENOMEM;
+ }
+
+ vma = NULL;
+ error = vma_iter_clear_gfp(&vmi, addr, end, GFP_KERNEL);
+ if (error) {
+ abort_munmap_vmas(&mas_detach);
+ return -ENOMEM;
+ }
+
+ /* Point of no return */
+ vms_complete_munmap_vmas(&vms, &mas_detach);
+ } else {
+ // TODO
+ }
/*
* Private writable mapping: check memory availability
The next patch now moves the call to vms_complete_munmap_vmas() towards the end of
mmap_region(). This code is also free of the rss counter bug.
commit a4b24bb18dde627792297455befcc465e45be66d
Author: Bert Karwatzki <spasswolf@web•de>
Date: Thu Jun 20 17:02:08 2024 +0200
mm: mmap: push back vms_complete_munmap_vmas()
In order to to debug the rss counter bug we're going to push back
vms_complete_munmap_vmas() in mmap_region.
Signed-off-by: Bert Karwatzki <spasswolf@web•de>
diff --git a/mm/mmap.c b/mm/mmap.c
index 0f020c535c83..4fb9dd2e6d6e 100644
--- a/mm/mmap.c
+++ b/mm/mmap.c
@@ -2970,9 +2970,9 @@ unsigned long mmap_region(struct file *file, unsigned long addr,
}
/* Point of no return */
- vms_complete_munmap_vmas(&vms, &mas_detach);
} else {
- // TODO
+ vms.end = 0;
+ vms.nr_pages = 0;
}
/*
@@ -3016,6 +3016,11 @@ unsigned long mmap_region(struct file *file, unsigned long addr,
vma_iter_next_range(&vmi);
}
+ if (vms.end) {
+ vms_complete_munmap_vmas(&vms, &mas_detach);
+ vms.end = 0; // avoid double unmap below
+ }
+
/* Actually expand, if possible */
if (vma &&
!vma_expand(&vmi, vma, merge_start, merge_end, vm_pgoff, next)) {
@@ -3026,7 +3031,8 @@ unsigned long mmap_region(struct file *file, unsigned long addr,
if (vma == prev)
vma_iter_set(&vmi, addr);
cannot_expand:
-
+ if (vms.end)
+ vms_complete_munmap_vmas(&vms, &mas_detach);
/*
* Determine the object being mapped and call the appropriate
* specific mapper. the address has already been validated, but
The next patch move vms_complete_munmap_vmas() a little further beyond the
call to vma_expand(). This code contain the rss counter bug.
commit 02d6be2410fa503d008f4cc8dcd1518ca56f8793
Author: Bert Karwatzki <spasswolf@web•de>
Date: Thu Jun 20 20:07:13 2024 +0200
mm: mmap: push back vms_complete_munmap_vmas()
This commit actually show the rss counter bug, while the previus does
not!
Signed-off-by: Bert Karwatzki <spasswolf@web•de>
diff --git a/mm/mmap.c b/mm/mmap.c
index 4fb9dd2e6d6e..c5f4b4b6fb84 100644
--- a/mm/mmap.c
+++ b/mm/mmap.c
@@ -3016,14 +3016,12 @@ unsigned long mmap_region(struct file *file, unsigned long addr,
vma_iter_next_range(&vmi);
}
- if (vms.end) {
- vms_complete_munmap_vmas(&vms, &mas_detach);
- vms.end = 0; // avoid double unmap below
- }
-
/* Actually expand, if possible */
if (vma &&
!vma_expand(&vmi, vma, merge_start, merge_end, vm_pgoff, next)) {
+ if (vms.end) {
+ vms_complete_munmap_vmas(&vms, &mas_detach);
+ }
khugepaged_enter_vma(vma, vm_flags);
goto expanded;
}
So there might be some unwanted interaction between vms_complete_munmap_vmas though
I've no yet figured out what exactly is happening. Hope this will be helpful in
solving the problem.
Bert Karwatzki
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] mm: Fix mmap_assert_locked() in follow_pte()
@ 2024-07-12 12:43 Bert Karwatzki
0 siblings, 0 replies; 5+ messages in thread
From: Bert Karwatzki @ 2024-07-12 12:43 UTC (permalink / raw)
To: Pei Li
Cc: Bert Karwatzki, Andrew Morton, Shuah Khan, David Hildenbrand,
linux-mm, linux-kernel, linux-next, syzbot+35a4414f6e247f515443,
syzkaller-bugs, linux-kernel-mentees, senozhatsky
diff --git a/mm/memory.c b/mm/memory.c
index 282203363177..2f4b4322ec0e 100644
--- a/mm/memory.c
+++ b/mm/memory.c
@@ -1817,6 +1817,7 @@ static void unmap_single_vma(struct mmu_gather *tlb,
{
unsigned long start = max(vma->vm_start, start_addr);
unsigned long end;
+ bool mm_read_locked;
if (start >= vma->vm_end)
return;
@@ -1829,11 +1830,11 @@ static void unmap_single_vma(struct mmu_gather *tlb,
if (unlikely(vma->vm_flags & VM_PFNMAP)) {
if (!mm_wr_locked)
- mmap_read_lock(vma->vm_mm);
+ mm_read_locked = !mmap_read_trylock(vma->vm_mm);
untrack_pfn(vma, 0, 0, mm_wr_locked);
- if (!mm_wr_locked)
+ if (!mm_wr_locked && !mm_read_locked)
mmap_read_unlock(vma->vm_mm);
}
This seems to fix the issue without completely removing the locking.
Bert Karwatzki
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: commit 1c29a32ce65f4cd0f1c causes Bad rss-counter state and firefox-esr crash in linux-next-20240613
2024-07-12 12:17 ` commit 1c29a32ce65f4cd0f1c causes Bad rss-counter state and firefox-esr crash in linux-next-20240613 Bert Karwatzki
@ 2024-07-12 15:38 ` Liam R. Howlett
2024-07-12 16:26 ` Bert Karwatzki
0 siblings, 1 reply; 5+ messages in thread
From: Liam R. Howlett @ 2024-07-12 15:38 UTC (permalink / raw)
To: Bert Karwatzki
Cc: Andrew Morton, Jiri Olsa, linux-mm, linux-kernel, linux-next
* Bert Karwatzki <spasswolf@web•de> [240712 08:18]:
> I did some experiments on the rss counter bug. The next patch is made for linux-next-20240613
> with commit 1c29a32ce65f4cd0f1c0f9 reverted. Then I simply inlined the code of do_vmi_unmap()
> and do_vmi_align_munmap() into mmap_region(). This version of the code works fine and does not
> show the rss counter bug.
Are you still working with v1 of this patch set?
I root-caused the rss counter issue and seg fault to the fact that next
or prev may be expanded and I was using the new start/end on munmap() in
the completion. This was fixed in subsequent patches.
I've sent v4 recently, but will have to a v5 to back off the removal of
arch_unmap() for PPC.
...
Thanks,
Liam
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: commit 1c29a32ce65f4cd0f1c causes Bad rss-counter state and firefox-esr crash in linux-next-20240613
2024-07-12 15:38 ` Liam R. Howlett
@ 2024-07-12 16:26 ` Bert Karwatzki
0 siblings, 0 replies; 5+ messages in thread
From: Bert Karwatzki @ 2024-07-12 16:26 UTC (permalink / raw)
To: Liam R. Howlett, Andrew Morton, Jiri Olsa, linux-mm, linux-kernel,
linux-next
Am Freitag, dem 12.07.2024 um 11:38 -0400 schrieb Liam R. Howlett:
> * Bert Karwatzki <spasswolf@web•de> [240712 08:18]:
> > I did some experiments on the rss counter bug. The next patch is made for linux-next-20240613
> > with commit 1c29a32ce65f4cd0f1c0f9 reverted. Then I simply inlined the code of do_vmi_unmap()
> > and do_vmi_align_munmap() into mmap_region(). This version of the code works fine and does not
> > show the rss counter bug.
>
> Are you still working with v1 of this patch set?
>
> I root-caused the rss counter issue and seg fault to the fact that next
> or prev may be expanded and I was using the new start/end on munmap() in
> the completion. This was fixed in subsequent patches.
>
> I've sent v4 recently, but will have to a v5 to back off the removal of
> arch_unmap() for PPC.
>
> ...
>
> Thanks,
> Liam
Sorry, that was a mistake when using git send-email while working on another bug
I accidently sent this old mail. I actually have tested the v3 and v4 patchsets
on linux-next up to 2024711 with no error. I havent't tested v4 on linux-
20240712, yet, because next-20240712 has a deadlock issue:
https://lore.kernel.org/all/20240712080414.GA47643@google.com/
(while sending a mail to this issue, I accidently send the old mail, too)
Bert Karwatzki
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2024-07-12 16:26 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-07-12 12:17 [PATCH] mm: Fix mmap_assert_locked() in follow_pte() Bert Karwatzki
2024-07-12 12:17 ` commit 1c29a32ce65f4cd0f1c causes Bad rss-counter state and firefox-esr crash in linux-next-20240613 Bert Karwatzki
2024-07-12 15:38 ` Liam R. Howlett
2024-07-12 16:26 ` Bert Karwatzki
-- strict thread matches above, loose matches on Subject: below --
2024-07-12 12:43 [PATCH] mm: Fix mmap_assert_locked() in follow_pte() Bert Karwatzki
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox