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

[Oleg Nesterov <oleg@redhat.com>]

On 05/26, Roland McGrath wrote:
>
> > 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.

Yes, but currently this change really makes the code look better. Just look
at this

-       if (task_ptrace(child) && child->ptrace_task->pt_tracer == current) {
+       if (ptrace_tracer(child) == current) {

change. But yes, these cosmetic changes will likely be reconsidered
later. The same for s/task->ptrace/task_ptrace(task)/ changes.

> > 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.

So, I assume it is better to not use tracehook_tracer_task() and add
another helper like this patch does.

> > +	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;

OK,

> >  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?

No, except the code looks better, imho.

> Or is it just that they should already be guaranteed
> synonymous, and here you have use for the tracer pointer a few lines later?

Yes.

Oleg.