linux-kernel - Re: Possible 6.5 regression: Huge values for "commited memory"

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20230916223518.h7zv2ysybitemoab@revolver>
Date:   Sat, 16 Sep 2023 18:35:18 -0400
From:   "Liam R. Howlett" <Liam.Howlett@...cle.com>
To:     Linus Torvalds <torvalds@...ux-foundation.org>
Cc:     Bagas Sanjaya <bagasdotme@...il.com>,
        Michael Labiuk <michael.labiuk@...tuozzo.com>,
        Christoph Biedl <linux-kernel.bfrz@...chmal.in-ulm.de>,
        Linux PARISC <linux-parisc@...r.kernel.org>,
        Linux Memory Management List <linux-mm@...ck.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Linux Regressions <regressions@...ts.linux.dev>,
        Andrew Morton <akpm@...ux-foundation.org>,
        "Matthew Wilcox (Oracle)" <willy@...radead.org>
Subject: Re: Possible 6.5 regression: Huge values for "commited memory"

* Linus Torvalds <torvalds@...ux-foundation.org> [230916 15:32]:
> On Sat, 16 Sept 2023 at 04:43, Bagas Sanjaya <bagasdotme@...il.com> 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@virtuozzo.com/
> 
> 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