linux-kernel - Re: [PATCH] mm: Always downgrade mmap

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] [day] [month] [year] [list]

Message-ID: <CAHk-=wiuXi4KmjgTocOZMpHTZuZP+y8r5wYynvvT_ZPXyG+TYA@mail.gmail.com>
Date:   Thu, 29 Jun 2023 13:00:35 -0700
From:   Linus Torvalds <torvalds@...ux-foundation.org>
To:     "Matthew Wilcox (Oracle)" <willy@...radead.org>
Cc:     Andrew Morton <akpm@...ux-foundation.org>, linux-mm@...ck.org,
        linux-kernel@...r.kernel.org,
        "Liam R . Howlett" <Liam.Howlett@...cle.com>
Subject: Re: [PATCH] mm: Always downgrade mmap_lock if requested

On Thu, 29 Jun 2023 at 12:14, Matthew Wilcox (Oracle)
<willy@...radead.org> wrote:
>
> Now that stack growth must always hold the mmap_lock for write, we can
> always downgrade the mmap_lock to read and safely unmap pages from the
> page table, even if we're next to a stack.

Can we please also fix the really odd return value semantics?

Right now that function returns either an error - meaning that it
didn't downgrade, or it returns 0/1 as a success to show "did I
downgrade as you asked me to"?

That is *really* confusing, but it was needed in that bad old world order.

But now that the downgrade is not a "try to downgrade if you can", but
something reliable, can we please just make the success case be 0, and
make the callers all know that on success, it was downgraded?

And yes, I realize that that means do_vmi_munmap() also has to be
changed. The documentation for that function is horrid, btw, in that
it says

 * Returns: -EINVAL on failure, 1 on success and unlock, 0 otherwise.

which is just not true. It can return other errors than -EINVAL
(through exactly that do_vmi_align_munmap() function), and the "1 on
success and unlock" is not true, it's a "success and downgrade if you
asked me to".

So I think all of those callers should also be changed to "if you
asked for a downgrade, and do_vmi_munmap() returned success, then you
got a downgrade".

Then some of the callers of *that* can be simplified too.

Please?

                 Linus