linux-kernel - Re: Fwd: Memory corruption in multithreaded user space program while calling fork

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: <CAHk-=wh498i3s+BgOF=pUOF=Qe_A0A16-mFcH2YGy+iZXvNChQ@mail.gmail.com>
Date:   Sat, 8 Jul 2023 11:04:49 -0700
From:   Linus Torvalds <torvalds@...ux-foundation.org>
To:     Andrew Morton <akpm@...ux-foundation.org>
Cc:     Thorsten Leemhuis <regressions@...mhuis.info>,
        Suren Baghdasaryan <surenb@...gle.com>,
        Bagas Sanjaya <bagasdotme@...il.com>,
        Jacob Young <jacobly.alt@...il.com>,
        Laurent Dufour <ldufour@...ux.ibm.com>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Linux Memory Management <linux-mm@...ck.org>,
        Linux PowerPC <linuxppc-dev@...ts.ozlabs.org>,
        Linux ARM <linux-arm-kernel@...ts.infradead.org>,
        Greg KH <gregkh@...uxfoundation.org>,
        Linux regressions mailing list <regressions@...ts.linux.dev>
Subject: Re: Fwd: Memory corruption in multithreaded user space program while
 calling fork

On Sat, 8 Jul 2023 at 10:39, Andrew Morton <akpm@...ux-foundation.org> wrote:
>
> That was the v1 fix, but after some discussion
> (https://lkml.kernel.org/r/20230705063711.2670599-1-surenb@google.com)
> it was decided to take the "excessive" approach.

That makes absolutely _zero_ sense.

It seems to be complete voodoo programming.

To some degree I don't care what happens in stable kernels, but
there's no way we'll do that kind of thing in mainline without some
logic or reason, when it makes no sense.

flush_cache_dup_mm() is entirely irrelevant to the whole issue, for
several reason, but the core one being that it only matters on broken
virtually indexed caches, so none of the architectures that do per-vma
locking.

And the argument that "After the mmap_write_lock_killable(), there
will still be a period where page faults can happen" may be true
(that's kind of the *point* of per-vma locking), but it's irrelevant.

It's true for *all* users of mmap_write_lock_killable, whether in fork
or anywhere else. What makes fork() so magically special?

It's why we have that vma_start_write(), to say "I'm now modifying
*this* vma, so stop accessing it in parallel".

Because no, flush_cache_dup_mm() is not the magical reason to do that thing.

Maybe there is something else going on, but no, we don't write crazy
code without a reason for it. That's completely unmaintainable,
because people will look at that code, not understand it (because
there is nothing to understand) and be afraid to touch it. For no
actual reason.

The obvious place to say "I'm now starting to modify the vma" is when
you actually start to modify the vma.

> Also, this change needs a couple more updates:

Those updates seem sane, and come with explanations of why they exist.
Looks fine to me.

Suren, please send me the proper fixes. Not the voodoo one. The ones
you can explain.

And if stable wants to do something else, then that's fine. But for
the development kernel,. we have two options:

 - fix the PER_VMA_LOCK code

 - decide that it's not worth it, and just revert it all

and honestly, I'm ok with that second option, simply because this has
all been way too much pain.

But no, we don't mark it broken thinking we can't deal with it, or do
random non-sensible code code we can't explain.

            Linus