ptracee data structures cleanup

Roland McGrath roland at redhat.com
Wed Apr 22 03:22:05 UTC 2009


[We have been on fine details here that are quite purely ptrace innards for
a while now.  I think discussion at this level of detail about this stuff
quite far from utrace per se belongs on LKML.]

> But __ptrace_may_access()->get_dumpable(task->mm) is not safe without
> task_lock(). This is easy.

Yes, probably just use ptrace_may_access() and fold them back together.

> > The clean-up should get rid of PT_DTRACE entirely.
> 
> Agreed. But this needs another patch...

Yes, or several.  It always gets fiddly when to get lots of little arch
changes merged.  The 90% that are just one-liner removal of wholly unused
PT_DTRACE can probably go in as a single patch to Linus instead of tiny
ones through each arch tree.

> Roland, I have to apologize for delay again. The first patch (move ->ptrace
> in struct ->ptrace_task) is not ready. Will do my best to send tomorrow.

No worries.

> I was distracted by other problems, and then I found that this (trivial) patch
> becomes really huge. There are 116 places which use task->ptrace directly,
> most in arch/.

Two things about this.  First, I expected ->ptrace would be the most
annoying to touch, though maybe I failed to point this out.  I had
suspected it might be easiest to move it last among the cleanups, do the
rest of the data structure fields incrementally first (or at least submit
it that way).

Second, this indicates need for more cleanup rather than just mechanically
converting the crufty code cosmetically.  Basically, any direct use of
->ptrace in arch code is suspect.  This is what tracehook et al are for.

We need to look at each of these arch-specific uses and see what they
really need.  Many are pure cruft like PT_DTRACE that can just be removed
(the arch maintainers will say they don't even know why it's there).
Others are crufty old boilerplate like:

	if (!test_thread_flag(TIF_SYSCALL_TRACE))
		return;

	if (!(current->ptrace & PT_PTRACED))
		return;

Those ->ptrace checks should just be removed.  The generic code handles
that case (and this cruft doesn't handle the races in it, anyway).

In fact, those are examples where the arch just needs to get with it and
convert to tracehook_report_syscall_* instead of the old code altogether.
But you can leave figuring that out to the arch people, and just remind
them of it.  Meanwhile, they (or Linus/akpm) can merge the removal of the
->ptrace checks where the ptrace maintainers are saying it's useless.

> Do you agree with these helpers? Perhaps even ptrace_test_flag() makes sense...

Nope.  When we're done with all the clean-up, there should be no place
outside of ptrace innards (i.e. ptrace.[ch], exit.c wait code, tracehook.h)
that even knows that ->ptrace exists.  

All the arch cruft makes that a bit of a tall order, and is why I expect
the actual move of the ->ptrace field will be one of the very last pieces
to get merged in.  (Another reason to do all the other clean-up in tiny
incremental pieces that we can parallelize and reorder for submission.)


Thanks,
Roland




More information about the utrace-devel mailing list