Re: [PATCH v2] ptrace: disable single step in __ptrace_unlink for protecting init task

[Oleg Nesterov <oleg@redhat.com>]

On 10/25, chen zhang wrote:
>
> Thanks for your reply. I think kernel should not panic when the
> application has a bug, or a fault operation such as ctrl+c,

	a) init is special. If it exits, the kernel panics. This is
	   by design.

	b) debugger can always crash the tracee. In particular if it
	   exits without ptrace(PTRACE_DETACH) which implies
	   user_disable_single_step().

> This patch can really prevent kernel panic on
> my x86 machine.

Really? You ignored this part of my previous email,

	Not to mention I don't understand how your patch can actually help. If nothing
	else,

		- debugger does ptrace(PTRACE_SINGLESTEP), this wakes the tracee up

		- the tracee enters force_sig_info_to_task(SIGTRAP) after single step

		- debugger exits, __ptrace_unlink() clears ptrace/TIF_SINGLESTEP

		- force_sig_info_to_task() clears SIGNAL_UNKILLABLE, the traced init
		  will be killed.

Am I wrong?

Finally,

> --- a/kernel/ptrace.c
> +++ b/kernel/ptrace.c
> @@ -130,6 +130,8 @@ void __ptrace_unlink(struct task_struct *child)
>  	put_cred(old_cred);
>
>  	spin_lock(&child->sighand->siglock);
> +	if (unlikely(child->signal->flags & SIGNAL_UNKILLABLE))
> +		user_disable_single_step(child);
>  	child->ptrace = 0;
>  	/*

I don't understnd why do you call user_disable_single_step() with ->siglock
held, but this is minor.

user_disable_single_step(child) assumes that child is stopped and frozen,
see ptrace_freeze_traced(). This is not necessarily true if __ptrace_unlink()
is called by the exiting tracer, so the patch is wrong in any case.

Nack, sorry.

Oleg.