Re: [RFC PATCH 8/X] ptrace: introduce ptrace_tracer() helper

[Roland McGrath <roland@redhat.com>]

> Introduce ptrace_tracer() (or suggest a better name) to simplify/cleanup
> the code which needs the tracer and checks task_ptrace(). From now nobody
> else uses ->pt_tracer except ptrace_link/ptrace_unlink.

There is nothing really wrong with this.  But I think that this stuff will
get sufficiently reworked again differently later on if it's converted to
use utrace that this incremental cleanup may not really help any.

> Question. Note that ptrace_tracer() is equal to tracehook_tracer_task().
> But I do not understand the future plans for tracehook_tracer_task().
> Should we just use tracehook_tracer_task() ? If yes, how
> ptrace_reparented() can use this helper?

It seems likely that we will rework tracehook_tracer_task() later.
It has three kinds of callers:

1. task_state() for "TracerPid:" line.
   It remains to be seen if we want to make some hookified way that might
   ever have a non-ptrace tracer supply the value here.  This was the main
   original expectation of what tracehook_tracer_task() would do.
2. check_mem_permission()
   I've already suggested to you that I think we want to swallow this
   use as part of the clean-up/replacement of ptrace_may_access().
3. SELinux: selinux_bprm_set_creds(), selinux_setprocattr()
   It makes sure that "PROCESS PTRACE" tracer->tracee avc checks can
   inhibit the transition (exec/setprocattr call).

For each of these, we have yet to hash out whether we will only ever want a
cleaned-up ptrace support here, or if in a future generalized tracing setup
like utrace these should be hooks that some non-ptrace kind of tracer
facility could also supply.  Figuring any piece of all that out is way
beyond the simple data structure cleanup phase.  I don't think we want to
get into any of that quite yet.

> +	parent = ptrace_tracer(tsk);
> +	if (likely(!parent))
>  		parent = tsk->real_parent;

This likely() doesn't buy much anyway, I'd just write the shorter:

	parent = ptrace_tracer(task) ?: tsk->real_parent;

>  static inline int may_ptrace_stop(void)
>  {
> -	if (!likely(task_ptrace(current)))
> +	struct task_struct *tracer = ptrace_tracer(current);
> +
> +	if (!likely(tracer))
>  		return 0;

Is there a particular rationale to checking ptrace_tracer() != NULL vs
task_ptrace() != 0?  Or is it just that they should already be guaranteed
synonymous, and here you have use for the tracer pointer a few lines later?

> +	pid = task_pid_vnr(tracer);
> +	uid = task_uid(tracer);
>  
>  	ptrace_signal_deliver(regs, cookie);
>  
> @@ -1779,8 +1785,8 @@ static int ptrace_signal(int signr, sigi
>  		info->si_signo = signr;
>  		info->si_errno = 0;
>  		info->si_code = SI_USER;
> -		info->si_pid = task_pid_vnr(current->ptrace_task->pt_tracer);
> -		info->si_uid = task_uid(current->ptrace_task->pt_tracer);
> +		info->si_pid = pid;
> +		info->si_uid = uid;

I think the different clean-up I suggested is better for this.  (Move that
logic to resume-time in the tracer context.)  It's an inconsequential nit,
but it feels a little wrong e.g. that you take task_uid(tracer) before the
stop, but the tracer could call setuid() before it does PTRACE_CONT.  The
PTRACE_CONT (or whatever) is the "signal generation event", so that's the
point at which the si_uid value being determined makes most sense to me
because it parallels what a normal signal generation does.


Thanks,
Roland