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>
In-Reply-To: <8d063a26-43f5-0bb7-3203-c6a04dc159f8@proxmox.com>
From:   Linus Torvalds <torvalds@linux-foundation.org>
Date:   Tue, 25 Jul 2023 09:38:32 -0700
Message-ID: <CAHk-=whKBx_UUKagfyF72EJrpqNCupF4yeoPgapjEBe1bynGcw@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>,
        "Eric W. Biederman" <ebiederm@xmission.com>,
        Oleg Nesterov <oleg@redhat.com>
Cc:     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: multipart/mixed; boundary="0000000000009e069c0601525dd0"
Precedence: bulk

--0000000000009e069c0601525dd0
Content-Type: text/plain; charset="UTF-8"

On Tue, 25 Jul 2023 at 04:16, Fiona Ebner <f.ebner@proxmox.com> wrote:
>
> will end up without a vma and cause/log the segfault. Of course the
> process is already being killed, but I'd argue it is very confusing to
> users when apparent segfaults from such processes are being logged by
> the kernel.

Ahh. Yes, that wasn't the intent. A process that is being killed
should exit with the lethal signal, not SIGSEGV.

This is not new, btw - this situation is exactly the same as if you
use something like NFS where the process is killed and the IO is
interrupted and the page fault faults for that reason.

But I suspect *very* few people actually encounter that NFS situation
(you can get it on local filesystems too, but the IO race window is
then so small as to probably not be triggerable at all).

So the new killable() check is probably much easier to actually
trigger in practice, even though it's not a new situation per se.

What exactly made you notice? Is it just the logging from
'show_unhandled_signals' being set?

Because the actual signal itself, from the

        force_sig_fault(SIGSEGV, si_code, (void __user *)address);

in __bad_area_nosemaphore() should be overridden by the fact that a
lethal signal was already pending.

But let's add a couple of signal people rather than the mm people to
the participants. Eric, Oleg - would not an existing fatal signal take
precedence over a new SIGSEGV? I obviously thought it did, but looking
at 'get_signal()' and the signal delivery, I don't actually see any
code to that effect.

Fiona - that patch is easily reverted, and it was done as a separate
patch exactly because I was wondering if there was some subtle reason
we didn't already do that.

But before we revert it, would you mind trying out the attached
trivial patch instead?

I'd also still be interested if the symptoms were anything else than
'show_unhandled_signals' causing the show_signal_msg() dance, and
resulting in a message something like

    a.out[1567]: segfault at xyz ip [..] likely on CPU X

in dmesg...

                 Linus

--0000000000009e069c0601525dd0
Content-Type: text/x-patch; charset="US-ASCII"; name="patch.diff"
Content-Disposition: attachment; filename="patch.diff"
Content-Transfer-Encoding: base64
Content-ID: <f_lkiims3u0>
X-Attachment-Id: f_lkiims3u0

IGFyY2gveDg2L21tL2ZhdWx0LmMgfCA0ICsrKysKIDEgZmlsZSBjaGFuZ2VkLCA0IGluc2VydGlv
bnMoKykKCmRpZmYgLS1naXQgYS9hcmNoL3g4Ni9tbS9mYXVsdC5jIGIvYXJjaC94ODYvbW0vZmF1
bHQuYwppbmRleCBlODcxMWIyY2FmYWYuLmI0YTAyOTBlOTYzYyAxMDA2NDQKLS0tIGEvYXJjaC94
ODYvbW0vZmF1bHQuYworKysgYi9hcmNoL3g4Ni9tbS9mYXVsdC5jCkBAIC04MzEsNiArODMxLDEw
IEBAIF9fYmFkX2FyZWFfbm9zZW1hcGhvcmUoc3RydWN0IHB0X3JlZ3MgKnJlZ3MsIHVuc2lnbmVk
IGxvbmcgZXJyb3JfY29kZSwKIAkgKi8KIAlsb2NhbF9pcnFfZW5hYmxlKCk7CiAKKwkvKiBJZiBh
IGZhdGFsIHNpZ25hbCBpcyBwZW5kaW5nLCBkb24ndCBib3RoZXIgd2l0aCBhbnl0aGluZyBlbHNl
ICovCisJaWYgKGZhdGFsX3NpZ25hbF9wZW5kaW5nKCkpCisJCXJldHVybjsKKwogCS8qCiAJICog
VmFsaWQgdG8gZG8gYW5vdGhlciBwYWdlIGZhdWx0IGhlcmUgYmVjYXVzZSB0aGlzIG9uZSBjYW1l
CiAJICogZnJvbSB1c2VyIHNwYWNlOgo=
--0000000000009e069c0601525dd0--