linux-kernel - Re: [PATCH 20/20] signal: Don't restart fork when signals come in.

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]

Date:   Tue, 24 Jul 2018 15:05:28 -0500
From:   ebiederm@...ssion.com (Eric W. Biederman)
To:     Linus Torvalds <torvalds@...ux-foundation.org>
Cc:     Oleg Nesterov <oleg@...hat.com>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Wen Yang <wen.yang99@....com.cn>, majiang <ma.jiang@....com.cn>
Subject: Re: [PATCH 20/20] signal: Don't restart fork when signals come in.

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

> On Tue, Jul 24, 2018 at 10:58 AM Eric W. Biederman
> <ebiederm@...ssion.com> wrote:
>>
>> Yes you are quite right.   Easy enough to fix, but it definitely needs
>> to be fixed.
>>
>> I will respin.
>
> Would you mind trying a slightly different approach for this?
>
> How about moving the "copy_signal()" and "copy_sighandler()" cases up
> to fairly early in the "copy_process()" function (and clean up late,
> obviously).
>
> Then, instead of that "struct multiprocess_signals" thing, just add a
> "struct hlist_node node" to "struct signal" itself, and add it to the
> multiprocess hlist there.
>
> And then you can just remove it in bad_fork_cleanup_signal.
>
> Does this make "struct signal" a bit larger? Yes, but it's not a huge
> deal. We *could* make is some union with existing fields if we cared.
>
> But I think it would make the code *much* more understandable, and it
> would allow us to not have that "sigpending" copy, because you can
> just populate the final "signal->shared_pending" directly.
>
> Hmm?

I don't like it.

What I hear you asking is  moving up copy_signal copy_sighand copy_creds
and alloc_pid, and anything else that signal delivery might depend on.

Then in send_signal running __send_signal in a loop first for the
forking process and then for every process that is currently in the
middle of fork.

It feels like this gets us much later in fork, and there is a lot more
code to move and review.   Which to some extent makes us more
susceptible to periodic signals, as more work will be thrown away and
have to be redone.  Plus it makes the whole thing susceptible to signal
delivery growing some additional dependency (perhaps cgroups?) and that
getting missed and never noticed until someone manages to time a sending
a signal just right.

I really want something very simple and straight forward because I don't
see us testing or hitting this code path much in practice.  Moving this
into the middle of fork and adding more depedencies does not seem like
it will be that kind of straight forward.

Eric