linux-kernel - Re: [CFT}[PATCH] coredump: Limit what can interrupt coredumps

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: <87y2bh4jg5.fsf@disp2133>
Date:   Thu, 10 Jun 2021 14:18:34 -0500
From:   ebiederm@...ssion.com (Eric W. Biederman)
To:     Linus Torvalds <torvalds@...ux-foundation.org>
Cc:     Olivier Langlois <olivier@...llion01.com>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        linux-fsdevel <linux-fsdevel@...r.kernel.org>,
        io-uring <io-uring@...r.kernel.org>,
        Alexander Viro <viro@...iv.linux.org.uk>,
        Jens Axboe <axboe@...nel.dk>,
        "Pavel Begunkov\>" <asml.silence@...il.com>,
        Oleg Nesterov <oleg@...hat.com>
Subject: Re: [CFT}[PATCH] coredump: Limit what can interrupt coredumps

Linus Torvalds <torvalds@...ux-foundation.org> writes:

> On Thu, Jun 10, 2021 at 12:01 PM Eric W. Biederman
> <ebiederm@...ssion.com> wrote:
>>
>> diff --git a/kernel/signal.c b/kernel/signal.c
>> index f7c6ffcbd044..83d534deeb76 100644
>> --- a/kernel/signal.c
>> +++ b/kernel/signal.c
>> @@ -943,8 +943,6 @@ static bool prepare_signal(int sig, struct task_struct *p, bool force)
>>         sigset_t flush;
>>
>>         if (signal->flags & (SIGNAL_GROUP_EXIT | SIGNAL_GROUP_COREDUMP)) {
>> -               if (!(signal->flags & SIGNAL_GROUP_EXIT))
>> -                       return sig == SIGKILL;
>>                 /*
>>                  * The process is in the middle of dying, nothing to do.
>>                  */
>
> I do think this part of the patch is correct, but I'd like to know
> what triggered this change?
>
> It seems fairly harmless - SIGKILL used to be the only signal that was
> passed through in the coredump case, now you pass through all
> non-ignored signals.
>
> But since SIGKILL is the only signal that is relevant for the
> fatal_signal_pending() case, this change seems irrelevant for the
> coredump issue. Any other signals passed through won't matter.
>
> End result: I think removing those two lines is likely a good idea,
> but I also suspect it could/should just be a separate patch with a
> separate explanation for it.
>
> Hmm?

I just didn't want those two lines hiding any other issues we might
have in the coredumps.

That is probably better development thinking than minimal fix thinking.

I am annoyed at the moment that those two lines even exist and figure
they are the confusing root cause of the problem.  That if we had
realized all it would take was to call fatal_signal_pending || freezing
than we could have avoided a problem entirely.

Eric