linux-kernel - Re: [PATCH 1/7] kernel: don't call do_exit() for PF_IO

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: <80915fd5-7cb1-167a-2c8a-01a0f8f21bd2@kernel.dk>
Date:   Fri, 26 Mar 2021 16:11:18 -0600
From:   Jens Axboe <axboe@...nel.dk>
To:     "Eric W. Biederman" <ebiederm@...ssion.com>
Cc:     io-uring@...r.kernel.org, torvalds@...ux-foundation.org,
        metze@...ba.org, oleg@...hat.com, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/7] kernel: don't call do_exit() for PF_IO_WORKER threads

On 3/26/21 2:43 PM, Eric W. Biederman wrote:
> Jens Axboe <axboe@...nel.dk> writes:
> 
>> Right now we're never calling get_signal() from PF_IO_WORKER threads, but
>> in preparation for doing so, don't handle a fatal signal for them. The
>> workers have state they need to cleanup when exiting, and they don't do
>> coredumps, so just return instead of performing either a dump or calling
>> do_exit() on their behalf. The threads themselves will detect a fatal
>> signal and do proper shutdown.
>>
>> Signed-off-by: Jens Axboe <axboe@...nel.dk>
>> ---
>>  kernel/signal.c | 9 +++++++++
>>  1 file changed, 9 insertions(+)
>>
>> diff --git a/kernel/signal.c b/kernel/signal.c
>> index f2a1b898da29..e3e1b8fbfe8a 100644
>> --- a/kernel/signal.c
>> +++ b/kernel/signal.c
>> @@ -2756,6 +2756,15 @@ bool get_signal(struct ksignal *ksig)
>>  		 */
>>  		current->flags |= PF_SIGNALED;
>>  
>> +		/*
>> +		 * PF_IO_WORKER threads will catch and exit on fatal signals
>> +		 * themselves. They have cleanup that must be performed, so
>> +		 * we cannot call do_exit() on their behalf. coredumps also
>> +		 * do not apply to them.
>> +		 */
>> +		if (current->flags & PF_IO_WORKER)
>> +			return false;
>> +
> 
> Returning false when get_signal needs the caller to handle a signal
> adds a very weird and awkward special case to how get_signal returns
> arguments.
> 
> Instead you should simply break and let get_signal return SIGKILL like
> any other signal that has a handler that the caller of get_signal needs
> to handle.
> 
> Something like:
>> +		/*
>> +		 * PF_IO_WORKER have cleanup that must be performed,
>> +		 * before calling do_exit().
>> +		 */
>> +		if (current->flags & PF_IO_WORKER)
>> +			break;
> 
> 
> As do_coredump does not call do_exit there is no reason to skip calling into
> the coredump handling either.   And allowing it will remove yet another
> special case from the io worker code.

Thanks, I'll turn it into a break, that does seem like a better idea in
general. Actually it wants to be a goto or similar, as a break will
assume that we have the sighand lock held. With the coredump being
irrelevant, I'll just it before the do_exit() call.

-- 
Jens Axboe