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