* Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx> [230916 15:32]: > On Sat, 16 Sept 2023 at 04:43, Bagas Sanjaya <bagasdotme@xxxxxxxxx> wrote: > > > > Thanks for the regression report. Michael had already bisected it [1], so > > telling regzbot: > > > > #regzbot ^introduced: 408579cd627a15 > > #regzbot title: huge committed memory due to returning 0 on do_vmi_align_mmunmap() success > > > > [1]: https://lore.kernel.org/linux-parisc/30f16b4f-a2fa-fc42-fe6e-abad01c3f794@xxxxxxxxxxxxx/ > > Funky. That commit isn't actually supposed to change anything, and the > only locking change was because it incorrectly ended up doing the > unlock a bit too early (before it did a validate_mm() - fixed in > commit b5641a5d8b8b ("mm: don't do validate_mm() unnecessarily and > without mmap locking"). > > HOWEVER. > > Now that I look at it again, I note this change in move_vma(). > > - if (do_vmi_munmap(&vmi, mm, old_addr, old_len, uf_unmap, false) < 0) { > + if (!do_vmi_munmap(&vmi, mm, old_addr, old_len, uf_unmap, false)) { > > and I think that is wrong. > > The return value that changed was the old "return 1 if successful > _and_ lock downgraded". > > Now it does "lock is always released on success if requested". So the > special "1" return went away, but the failure case didn't change. > > So that change to "move_vma()" seems to be bogus. It used to do "if > failed". Now it does "if success". > > Does the attached patch fix the problem? > > Liam - or am I just crazy? That return value check change really looks > bogus to me, but it looks *so* bogus that it makes me think I'm > missing something. You are correct. I should not have changed that check. I haven't devised a way to check this on parisc (I finally have a qemu parisc vm booting), but it should happen on any call to move_vma(). It looks like you have a tester though, so I'll return to dinner. Thanks, Liam
