Messages in this thread | | | Date | Wed, 27 May 2009 23:45:57 +0200 | From | Oleg Nesterov <> | Subject | Re: [RFC PATCH 8/X] ptrace: introduce ptrace_tracer() helper |
| |
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.
| |