linux-kernel - Re: [RFC][PATCH] signal: Store pending signal exit in tsk.jobctl not in tsk.pending

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: <87imxwv9jp.fsf@xmission.com>
Date:   Wed, 06 Feb 2019 16:25:46 -0600
From:   ebiederm@...ssion.com (Eric W. Biederman)
To:     Oleg Nesterov <oleg@...hat.com>
Cc:     Thomas Gleixner <tglx@...utronix.de>,
        Dmitry Vyukov <dvyukov@...gle.com>,
        Ingo Molnar <mingo@...hat.com>,
        Peter Zijlstra <peterz@...radead.org>,
        LKML <linux-kernel@...r.kernel.org>,
        Arnaldo Carvalho de Melo <acme@...nel.org>,
        Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
        jolsa@...hat.com, Namhyung Kim <namhyung@...nel.org>,
        luca abeni <luca.abeni@...tannapisa.it>,
        syzkaller <syzkaller@...glegroups.com>
Subject: Re: [RFC][PATCH] signal: Store pending signal exit in tsk.jobctl not in tsk.pending

Oleg Nesterov <oleg@...hat.com> writes:

> Eric, at al,
>
> Sorry, I am on on vacation, can't even read this thread right now,
> so I am not sure I understand the problem correctly...

That is fair.  Please don't let me mess up your vacation.

The problem is an infinite stream of SIGHUP from a timer, making
processes unkillable.

Looking into that I see two bugs.
a) We handle fatal signals (esp SIGKILL) early in complete_signal and
   this SIGHUP stream is messing up our handling.
b) We don't properly distinguish between synchronous and asynchrous
   signals.

I am only aiming to fix the fatal signal issue with this change.

> On 02/05, Eric W. Biederman wrote:
>>
>> @@ -2393,6 +2393,11 @@ bool get_signal(struct ksignal *ksig)
>>  		goto relock;
>>  	}
>>
>> +	/* Has this task already been flagged for death? */
>> +	ksig->info.si_signo = signr = SIGKILL;
>> +	if (current->jobctl & JOBCTL_TASK_EXIT)
>> +		goto fatal;
>> +
>
> Can't we simply change, say, next_signal() to return SIGKILL if it is
> pending?

We could.  But since this isn't necessarily SIGKILL we are dealing with,
it could just as easily be an unhandled SIGINT, so having SIGKILL in the
per task signal queue could still lead to other problems.  I am afraid
that if continue abusing the per task signal queue other bugs of
confusion will occur.

> In any case, I am not sure we need JOBCTL_TASK_EXIT. Can't we rely on
> signal_group_exit() ?

Looking more closely yes I believe we can.

I thought de_thread would be a problem but signal_group_exit handles
that case and the core dump cases just fine.

It is a bit of a surprise but that would make:

> static inline int __fatal_signal_pending(struct task_struct *p)
> {
> 	return signal_group_exit(p->signal);
> }

I will respin and see if we can the change just depend on
signal_group_exit.  One less abstraction to have to keep
in sync sounds more robust in the long run.

Eric