Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <8d063a26-43f5-0bb7-3203-c6a04dc159f8@proxmox.com>
 <CAHk-=whKBx_UUKagfyF72EJrpqNCupF4yeoPgapjEBe1bynGcw@mail.gmail.com> <cc502fe7-716b-8114-c9e6-439e3b9cf0f6@proxmox.com>
In-Reply-To: <cc502fe7-716b-8114-c9e6-439e3b9cf0f6@proxmox.com>
From:   Linus Torvalds <torvalds@linux-foundation.org>
Date:   Wed, 26 Jul 2023 10:59:41 -0700
Message-ID: <CAHk-=wivat-bcWsGnQOd3=ODx0zFnc7R82tiee=fSU+DF4tD5g@mail.gmail.com>
Subject: Re: segfaults of processes while being killed after commit "mm: make
 the page fault mmap locking killable"
To:     Fiona Ebner <f.ebner@proxmox.com>
Cc:     "Eric W. Biederman" <ebiederm@xmission.com>,
        Oleg Nesterov <oleg@redhat.com>, akpm@linux-foundation.org,
        Thomas Lamprecht <t.lamprecht@proxmox.com>,
        Wolfgang Bumiller <w.bumiller@proxmox.com>, linux-mm@kvack.org,
        linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Wed, 26 Jul 2023 at 01:19, Fiona Ebner <f.ebner@proxmox.com> wrote:
>
> Checking the status from waitpid, it does show that the process was
> terminated by signal 9, even if the segfault was logged.

Thanks for verifying. That's what I thought, and I had just entirely
forgotten about the logging of failed page faults.

This whole "fatal signals during IO can also cause a failed page
fault" has been true for a long long time, but because it's done later
by the actual VM code, there we actually end up going through
"fault_signal_pending()" and suppressing the logging of the page fault
failure that way.

> > But before we revert it, would you mind trying out the attached
> > trivial patch instead?
>
> The patch works for me too :) (after adding the missing tsk argument
> like Thomas pointed out)

So it turns out that not only did I forget the argument, I decided
that I put that test for fatal signals in the wrong place.

The patch obviously does fix the problem on x86, and we could do the
same thing for all the other architectures that do this signal
logging.

But there's actually a much better place to put the fatal signal
check, which will take care of all architectures: just do it in the
'unhandled_signal()' function.

So I fixed the missing argument, and moved the test to a different
place, but I still added your (and Thomas') "Tested-by:" even if you
ended up testing something that was a bit different.

Oleg, I took your Acked-by too. Despite the final patch being somewhat
different. Holler if you see something objectionable.

It's commit 5f0bc0b042fc ("mm: suppress mm fault logging if fatal
signal already pending") in my tree now.

And because it's a bit different from what you already tested, it
would be lovely to just get a confirmation that I didn't screw
anything up when I decided I needed to make a fix that covers more
than just x86.

Thanks,
                     Linus