Date: Tue, 29 Nov 2016 15:06:00 +0100
From: Oleg Nesterov <oleg@redhat.com>
To: Jamie Iles <jamie.iles@oracle.com>
Cc: linux-kernel@vger.kernel.org, Alexander Viro <viro@zeniv.linux.org.uk>,
        Ingo Molnar <mingo@redhat.com>, Peter Zijlstra <peterz@infradead.org>,
        Andrew Morton <akpm@linux-foundation.org>
Subject: Re: [PATCH] signal: protect SIGNAL_UNKILLABLE from unintentional
        clearing.
Message-ID: <20161129140600.GB9654@redhat.com>
References: <20161116125759.4727-1-jamie.iles@oracle.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20161116125759.4727-1-jamie.iles@oracle.com>
User-Agent: Mutt/1.5.18 (2008-05-17)
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 4399
Lines: 120

Jamie,

I am really sorry for the huge delay.

On 11/16, Jamie Iles wrote:
>
> Since 00cd5c37af (ptrace: permit ptracing of /sbin/init) we can now
> trace init processes.  init is initially protected with
> SIGNAL_UNKILLABLE which will prevent fatal signals such as SIGSTOP, but
> there are a number of paths during tracing where SIGNAL_UNKILLABLE can
> be implicitly cleared.

Yes, old problem which nobody bothered to fix ;) Thanks for doing this.
To remind, there are more problems with SIGNAL_UNKILLABLE, but this
patch looks like a good start to me.

However, I would like to ask you to make V2, see below.

> +static inline void signal_set_flags(struct signal_struct *sig,
> +				    unsigned int flags)
> +{
> +	sig->flags = (sig->flags & SIGNAL_PROTECTED_MASK) | flags;
> +}

OK, agreed, probably the helper makes sense, but I think it is overused
in this patch, please see below. In short, I'd suggest to use it only in
the jctl code, at least for now.

> +static inline void signal_clear_flags(struct signal_struct *sig,
> +				      unsigned int flags)
> +{
> +	sig->flags &= (~flags | SIGNAL_PROTECTED_MASK);
> +}

But this one looks pointless.

> --- a/kernel/exit.c
> +++ b/kernel/exit.c
> @@ -922,7 +922,7 @@ do_group_exit(int exit_code)
>  			exit_code = sig->group_exit_code;
>  		else {
>  			sig->group_exit_code = exit_code;
> -			sig->flags = SIGNAL_GROUP_EXIT;
> +			signal_set_flags(sig, SIGNAL_GROUP_EXIT);

Well. I am not sure about this change. SIGNAL_GROUP_EXIT is the terminal
state, it is fine to clear UNKILLABLE.

Yes, perhaps this actually makes sense, and we want to make UNKILLABLE
immutable, but then we need to change force_sig_info() too, at least.
And btw it should be changed anyway, in particular UNKILLABLE can be
wrongly cleared if the task is traced. But I'd prefer to do this later.

The same for the similar changes in zap_process(), coredump_finish(),
and complete_signal().

> @@ -1315,7 +1315,7 @@ static int wait_task_continued(struct wait_opts *wo, struct task_struct *p)
>  		return 0;
>  	}
>  	if (!unlikely(wo->wo_flags & WNOWAIT))
> -		p->signal->flags &= ~SIGNAL_STOP_CONTINUED;
> +		signal_clear_flags(p->signal, SIGNAL_STOP_CONTINUED);

Why? "flags &= ~SIGNAL_STOP_CONTINUED" can't clear other bits, so this
change and the helper itself looks pointless.

If you add this helper for readability I won't argue. But then it should
not deny SIGNAL_PROTECTED_MASK, and force_sig_info() should use it too.

> @@ -346,7 +346,7 @@ static bool task_participate_group_stop(struct task_struct *task)
>  	 * fresh group stop.  Read comment in do_signal_stop() for details.
>  	 */
>  	if (!sig->group_stop_count && !(sig->flags & SIGNAL_STOP_STOPPED)) {
> -		sig->flags = SIGNAL_STOP_STOPPED;
> +		signal_set_flags(sig, SIGNAL_STOP_STOPPED);
>  		return true;

OK,

> @@ -837,7 +837,7 @@ static bool prepare_signal(int sig, struct task_struct *p, bool force)
>  			 * will take ->siglock, notice SIGNAL_CLD_MASK, and
>  			 * notify its parent. See get_signal_to_deliver().
>  			 */
> -			signal->flags = why | SIGNAL_STOP_CONTINUED;
> +			signal_set_flags(signal, why | SIGNAL_STOP_CONTINUED);

OK,

> @@ -922,7 +922,7 @@ static void complete_signal(int sig, struct task_struct *p, int group)
>  			 * running and doing things after a slower
>  			 * thread has the fatal signal pending.
>  			 */
> -			signal->flags = SIGNAL_GROUP_EXIT;
> +			signal_set_flags(signal, SIGNAL_GROUP_EXIT);

Again, I'd prefer to just set SIGNAL_GROUP_EXIT, like in do_group_exit().
Note also that SIGNAL_UNKILLABLE can't be set in this case, see the check
above, so this change has no effect. And at the same time this code needs
the changes too, this SIGNAL_UNKILLABLE check is not 100% correct, but this
is off-topic.

So I think it would be better to start with the minimal change which fixes
task_participate_group_stop() and prepare_signal() only. And while I won't
insist, perhaps we should add

	#define SIGNAL_STOP_MASK (SIGNAL_CLD_MASK | SIGNAL_STOP_STOPPED | SIGNAL_STOP_CONTINUED)

	signal_set_stop_flags(signal, flags)
	{
		signal->flags = (signal->flags & ~SIGNAL_STOP_MASK) | flags;
	}

instead of signal_set_flags(). This way we can, say, add
WARN_ON(signal->flags & (SIGNAL_GROUP_EXIT|SIGNAL_GROUP_COREDUMP)) into this helper.
Then later we can add signal_set_exit_flags() which manipulates
GROUP_EXIT/COREDUMP/UNKILLABLE.

Not sure, up to you.

Oleg.