From: Linus Torvalds <torvalds@linux-foundation.org>
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
Subject: Re: segfaults of processes while being killed after commit "mm: make the page fault mmap locking killable"
Date: Wed, 26 Jul 2023 10:59:41 -0700 [thread overview]
Message-ID: <CAHk-=wivat-bcWsGnQOd3=ODx0zFnc7R82tiee=fSU+DF4tD5g@mail.gmail.com> (raw)
In-Reply-To: <cc502fe7-716b-8114-c9e6-439e3b9cf0f6@proxmox.com>
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
next prev parent reply other threads:[~2023-07-26 18:00 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-07-25 11:16 Fiona Ebner
2023-07-25 16:38 ` Linus Torvalds
2023-07-26 6:51 ` Thomas Lamprecht
2023-07-26 8:19 ` Fiona Ebner
2023-07-26 17:59 ` Linus Torvalds [this message]
2023-07-27 7:57 ` Fiona Ebner
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CAHk-=wivat-bcWsGnQOd3=ODx0zFnc7R82tiee=fSU+DF4tD5g@mail.gmail.com' \
--to=torvalds@linux-foundation.org \
--cc=akpm@linux-foundation.org \
--cc=ebiederm@xmission.com \
--cc=f.ebner@proxmox.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mm@kvack.org \
--cc=oleg@redhat.com \
--cc=t.lamprecht@proxmox.com \
--cc=w.bumiller@proxmox.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox