Re: [RFC,PATCH 0/14] utrace/ptrace
Note to all: I'm on the road this week (having had a holiday last week) and will be somewhat slow in replying on these threads, but I will be sure to get to them all. Yes, nobody likes 2 implementations. I guess Roland and me hate CONFIG_UTRACE much more than anybody else. :-) We both hate maintaining the old ptrace implementation, that is true! The other thing we hate is having our work delayed arbitrarily again and again to wait for the arch maintainers of arch's we don't use ourselves. Oleg and I have been trying to follow the advice we get on how to get this work merged in. When multiple gate-keepers give conflicting advice, we really don't know what to do next. Our interest is in whatever path smooths the merging of our work. We'd greatly prefer to spend our time hacking on our new code to make it better or different in cool and useful ways than to spend more time guessing what order of patches and rejuggling of the work will fit the changing whims of the next round of review. We don't want to rush anyone, like uninterested arch maintainers. We don't want to break anyone who doesn't care about our work (we do test for ptrace regressions but of course new code will always have new bugs so some instances of instability in the transition to a new ptrace implementation have to be expected no matter how hard we try). We just don't want to keep working out of tree. The advantage of making the new code optional is, obviously, that you can turn it off. That is, lagging arch's won't be broken, just unable to turn it on. And, anyone who doesn't want to try anything new yet can just turn it off and not be exposed to new code. The advantage of making the new code nonoptional is, obviously, that you can't turn it off. The old implementation will be gone and we won't have to maintain it any more (outside some -stable streams for a while). The kernel source will be cleaner with no optional old cruft code duplicating functionality. All ptrace users will be testing the new code, and so we'll find its bugs and gain confidence that it works solidly. Like I've said, we want to do whatever the people want. My concern about what Christoph has suggested is that it really seems like an open-ended delay depending on some arch people who are not even in the conversation. For anyone either who likes utrace or who is concerned about its details, the sooner we are working in-tree the sooner we can really hash it out thoroughly and get to having merged code that works well. As long as there is anything unfinished like arch work that we've decided is a gating event, then the review and hashing out of the new code itself does not seem to get very serious. (To wit, this thread is still talking about merge order and such, another long thread was about incidental trivia, and we've only just started to see a tiny bit of actual code review today.) Thanks, Roland
Re: [RFC,PATCH 0/14] utrace/ptrace
On Wed 2009-11-25 16:48:18, Christoph Hellwig wrote: On Tue, Nov 24, 2009 at 09:01:27PM +0100, Oleg Nesterov wrote: Hello. This is the new iteration of Roland's utrace patch, this time with rewrite-ptrace-via-utrace + cleanups in utrace core. 1-7 are already in -mm tree, I am sending them to simplify the review. 8-12 don not change the behaviour, simple preparations. 13-14 add utrace-ptrace and utrace Skipped over it very, very briefly. One thing I really hate about this is that it introduces two ptrace implementation by adding the new one without removing the old one. Given that's it's pretty much too later for the 2.6.33 cycle anyway I'd suggest you make sure the remaining two major architectures (arm and mips) get converted, and if the remaining minor architectures don't manage to get their homework done they're left without ptrace. I don't think introducing regressions to force people to rewrite code is a good way to go... Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
Re: [RFC,PATCH 0/14] utrace/ptrace
On Thu, Nov 26, 2009 at 01:24:41PM +0100, Ingo Molnar wrote: FYI, the merge window has not opened yet, so it cannot close in a few days. subsystems merged window, not Linus'. [...] and thus not getting any of the broad -next test coverage is a pretty bad idea. In the end it will be the maintainers ruling but that doesn't make it a good idea from the engineering point of view. FYI, it's been in -mm, that's where it's maintained. None of the recent mm snapshots has anything utrace related in there, just a few ptrace patches from Oleg (which are in this series but a very small part of it) and certainly not all this new code that is pretty recent (take a look at the utrace list for the development). Yes. Which is a further argument to not do it like that but to do one arch at a time. Trying to do too much at once is bad engineering. I'm not sure why you're trying to pick fights here, but no one has said about doing it all in once. The point I'm trying to make is that it's pretty bad to keep parallel ptrace implementations, and we should settle on one. A pre-requisite of using the new once genericly is to have the architecture ptrace code updated. I think arm and mips are the two only relevant ones still missing, so updating them and killing the other ones is easy. If you think keeping the two ptrace implementations is fine argue for that directly, but please stick to the technical points instead of just fighting for fightings sake.
Re: [RFC,PATCH 0/14] utrace/ptrace
On 11/27, Christoph Hellwig wrote: On Thu, Nov 26, 2009 at 01:24:41PM +0100, Ingo Molnar wrote: FYI, it's been in -mm, that's where it's maintained. None of the recent mm snapshots has anything utrace related in there, Well, not that I think this is important, but... Two weeks ago we asked Andrew do drop utrace-core.patch from -mm, it should be replaced by this updated version. Oleg.
Re: [RFC,PATCH 0/14] utrace/ptrace
* Christoph Hellwig h...@infradead.org wrote: [...] Given that's it's pretty much too later for the 2.6.33 cycle anyway I'd suggest you make sure the remaining two major architectures (arm and mips) get converted, and if the remaining minor architectures don't manage to get their homework done they're left without ptrace. I suspect the opinion of the ptrace maintainers matters heavily whether it's appropriate for v2.6.33. You are not going to maintain this, they are. Regarding porting it to even more architectures - that's pretty much the worst idea possible. It increases maintenance and testing overhead by exploding the test matrix, while giving little to end result. Plus the worst effect of it is that it becomes even more intrusive and even harder (and riskier) to merge. So dont do that. The best strategy is to concentrate on just one or two well-tested architectures, and then grow to other architectures gradually. Like we've done it for basically all big kernel features in the past 10 years (that had non-trivial arch dependencies), with no exception that i can remember. Thanks, Ingo
Re: [RFC,PATCH 0/14] utrace/ptrace
* Christoph Hellwig h...@infradead.org wrote: On Thu, Nov 26, 2009 at 10:10:52AM +0100, Ingo Molnar wrote: [...] Given that's it's pretty much too later for the 2.6.33 cycle anyway I'd suggest you make sure the remaining two major architectures (arm and mips) get converted, and if the remaining minor architectures don't manage to get their homework done they're left without ptrace. I suspect the opinion of the ptrace maintainers matters heavily whether it's appropriate for v2.6.33. You are not going to maintain this, they are. I am whoever like many others going to use it. And throwing in new code a few days before the merge window closes [...] FYI, the merge window has not opened yet, so it cannot close in a few days. [...] and thus not getting any of the broad -next test coverage is a pretty bad idea. In the end it will be the maintainers ruling but that doesn't make it a good idea from the engineering point of view. FYI, it's been in -mm, that's where it's maintained. Regarding porting it to even more architectures - that's pretty much the worst idea possible. It increases maintenance and testing overhead by exploding the test matrix, while giving little to end result. Plus the worst effect of it is that it becomes even more intrusive and even harder (and riskier) to merge. But it doesn't. Take a look at what these patches actually do, they basically introduce a new utrace layer, and (conditionally) rewrite ptrace to use it. The arch support isn't actually part of these patches directly but rather the cleanup of the underlying arch ptrace code to use regsets, tracehooks and co so that the new ptrace code can use. ( I am aware of its design, i merged the original tracehook patches for x86. ) What the patches in the current form do is to introduce two different ptrace implementations, with one used on the architectures getting most testing and another secondary one for left over embedded or dead architectures with horrible results. So removing the old one is much better. The arm ptrace rewrite has already been posted by Roland, btw including some feedback from Russell, but nothing really happened to it. Yes. Which is a further argument to not do it like that but to do one arch at a time. Trying to do too much at once is bad engineering. Ingo
Re: [RFC,PATCH 0/14] utrace/ptrace
On Thu, 2009-11-26 at 12:37 +0530, Srikar Dronamraju wrote: Hi Christoph, The other thing is that this patchset really doesn't quite justify utrace. It's growing a lot more code without actually growing any useful functionality. What about all those other utrace killer features that have been promised for a long time? We are working on in-kernel gdbstub which was one of the features that you had asked for. gdbstub does pass unit tests; but we are looking at some way to hack the GDB testsuite to run its regression tests. Once we are able to run the GDB testsuite and utrace is part of some upstream tree, we plan to post these patches to LKML for comments. gdbstub uses utrace and uprobes underneath. Uprobes was rewritten to remove issues that LKML developers had opposed. Uprobes also has its own ftrace plugin to use uprobes. Currently in-kernel gdbstub is hosted by Frank Ch. Eigler over here: git://web.elastic.org/~fche/utrace-ext.git branch name utrace-gdbstub-uprobes If its anywhere near functioning it would have made sense to send it out as an RFC patch-set right along with the utrace one.
Re: [RFC,PATCH 0/14] utrace/ptrace
On 11/26, Christoph Hellwig wrote: What the patches in the current form do is to introduce two different ptrace implementations, with one used on the architectures getting most testing and another secondary one for left over embedded or dead architectures with horrible results. Yes, nobody likes 2 implementations. I guess Roland and me hate CONFIG_UTRACE much more than anybody else. So removing the old one is much better. I am in no position to discuss this option. It is very easy to remove the old code and break !HAVE_ARCH_TRACEHOOK architectures. Although personally I am not sure this is practical. If we merge utrace, perhaps we will get more attention from maintainers, the old code will be officially deprecated/obsolete. I sent some trivial initial changes in arch/um/ a long ago, the patch was silently ignored. Even if I was able to fix arch/xxx myself, I don't understand how can I send the patches to maintainers until utrace is already merged in -mm at least. Oleg.
Re: [RFC,PATCH 0/14] utrace/ptrace
On Tue, Nov 24, 2009 at 09:01:27PM +0100, Oleg Nesterov wrote:
Hello.
This is the new iteration of Roland's utrace patch, this time
with rewrite-ptrace-via-utrace + cleanups in utrace core.
1-7 are already in -mm tree, I am sending them to simplify the
review.
8-12 don not change the behaviour, simple preparations.
13-14 add utrace-ptrace and utrace
Oleg,
I ran the ptrace-tests testsuite [1] on powerpc on the vanilla ptrace
and then with ptrace/utrace. The results for ptrace/utrace look better
:-)
All tests are 'make check xcheck'.
Ananth
[1]
cvs -d :pserver:anoncvs:anon...@sources.redhat.com:/cvs/systemtap co ptrace-tests
-
Vanilla ptrace:
PASS: ptrace-on-job-control-stopped
PASS: attach-wait-on-stopped
PASS: detach-can-signal
PASS: attach-into-signal
PASS: attach-sigcont-wait
PASS: sa-resethand-on-cont-signal
PASS: ptrace_cont-defeats-sigblock
PASS: ptrace-cont-sigstop-detach
PASS: ptrace_event_clone
PASS: tif-syscall-trace-after-detach
PASS: event-exit-proc-maps
PASS: event-exit-proc-environ
SKIP: x86_64-ia32-gs
SKIP: x86_64-gsbase
PASS: powerpc-altivec
PASS: peekpokeusr
PASS: watchpoint
PASS: block-step
PASS: step-jump-cont
SKIP: step-jump-cont-strict
PASS: ppc-dabr-race
PASS: signal-loss
PASS: step-into-handler
SKIP: user-area-access
PASS: user-regs-peekpoke
PASS: erestartsys
SKIP: erestart-debugger
SKIP: step-to-breakpoint
errno 14 (Bad address)
syscall-reset: syscall-reset.c:95: main: Assertion `(*__errno_location ()) ==
38' failed.
unexpected child status 67f
FAIL: syscall-reset
PASS: reparent-zombie
PASS: step-simple
SKIP: step-through-sigret
PASS: stop-attach-then-wait
FAIL: detach-stopped
PASS: detach-stopped-rhel5
PASS: clone-multi-ptrace
PASS: clone-ptrace
PASS: o_tracevfork
PASS: o_tracevforkdone
PASS: detach-parting-signal
PASS: detach-sigkill-race
PASS: waitpid-double-report
PASS: o_tracevfork-parent
PASS: stopped-detach-sleeping
FAIL: stopped-attach-transparency
SKIP: erestartsys-trap
SKIP: highmem-debugger
PASS: sigint-before-syscall-exit
SKIP: syscall-from-clone
step-from-clone: step-from-clone.c:195: main: Assertion `(status 8) == 5'
failed.
step-from-clone: step-from-clone.c:119: handler_fail: Assertion `0' failed.
/bin/sh: line 5: 19825 Aborted ${dir}$tst
FAIL: step-from-clone
step-fork: step-fork.c:56: handler_fail: Assertion `0' failed.
/bin/sh: line 5: 19832 Aborted ${dir}$tst
FAIL: step-fork
5 of 41 tests failed
(10 tests were not run)
Please report to utrace-devel@redhat.com
make[3]: *** [check-TESTS] Error 1
make[3]: Leaving directory `/home/ananth/ptrace-tests/tests'
make[2]: *** [check-am] Error 2
make[2]: Leaving directory `/home/ananth/ptrace-tests/tests'
make[1]: *** [check] Error 2
make[1]: Leaving directory `/home/ananth/ptrace-tests/tests'
make: *** [check-recursive] Error 1
-
ptrace over utrace:
PASS: ptrace-on-job-control-stopped
PASS: attach-wait-on-stopped
PASS: detach-can-signal
PASS: attach-into-signal
PASS: attach-sigcont-wait
PASS: sa-resethand-on-cont-signal
PASS: ptrace_cont-defeats-sigblock
PASS: ptrace-cont-sigstop-detach
PASS: ptrace_event_clone
PASS: tif-syscall-trace-after-detach
PASS: event-exit-proc-maps
PASS: event-exit-proc-environ
SKIP: x86_64-ia32-gs
SKIP: x86_64-gsbase
PASS: powerpc-altivec
PASS: peekpokeusr
PASS: watchpoint
PASS: block-step
PASS: step-jump-cont
SKIP: step-jump-cont-strict
PASS: ppc-dabr-race
PASS: signal-loss
PASS: step-into-handler
SKIP: user-area-access
PASS: user-regs-peekpoke
PASS: erestartsys
SKIP: erestart-debugger
SKIP: step-to-breakpoint
errno 14 (Bad address)
syscall-reset: syscall-reset.c:95: main: Assertion `(*__errno_location ()) ==
38' failed.
unexpected child status 67f
FAIL: syscall-reset
PASS: reparent-zombie
PASS: step-simple
SKIP: step-through-sigret
PASS: stop-attach-then-wait
PASS: detach-stopped
PASS: detach-stopped-rhel5
PASS: clone-multi-ptrace
PASS: clone-ptrace
PASS: o_tracevfork
PASS: o_tracevforkdone
PASS: detach-parting-signal
PASS: detach-sigkill-race
PASS: waitpid-double-report
PASS: o_tracevfork-parent
PASS: stopped-detach-sleeping
PASS: stopped-attach-transparency
SKIP: erestartsys-trap
SKIP: highmem-debugger
PASS: sigint-before-syscall-exit
SKIP: syscall-from-clone
SKIP: step-from-clone
step-fork: step-fork.c:56: handler_fail: Assertion `0' failed.
/bin/sh: line 5: 24803 Aborted ${dir}$tst
FAIL: step-fork
2 of 40 tests failed
(11 tests were not run)
Please report to utrace-devel@redhat.com
make[3]: *** [check-TESTS] Error 1
make[3]: Leaving directory `/home/ananth/ptrace-tests/tests'
make[2]: *** [check-am] Error 2
make[2]: Leaving directory `/home/ananth/ptrace-tests/tests'
make[1]: *** [check] Error 2
make[1]: Leaving directory `/home/ananth/ptrace-tests/tests'
make: *** [check-recursive] Error 1
Re: [RFC,PATCH 0/14] utrace/ptrace
On 11/25, Ananth N Mavinakayanahalli wrote:
I ran the ptrace-tests testsuite [1] on powerpc on the vanilla ptrace
and then with ptrace/utrace. The results for ptrace/utrace look better
:-)
Great! thanks a lot Ananth for doing this.
ptrace-utrace still fails 2 tests,
FAIL: syscall-reset
I'll take a look later. Since unpatched kernel fails this test too
I am not going to worry right now. I think this is ppc specific, x86
passes this test.
step-fork: step-fork.c:56: handler_fail: Assertion `0' failed.
/bin/sh: line 5: 24803 Aborted ${dir}$tst
FAIL: step-fork
This is expected. Should be fixed by
ptrace-copy_process-should-disable-stepping.patch
in -mm tree. (I am attaching this patch below just in case)
I din't mention this patch in this series because this bug
is ortogonal to utrace/ptrace.
Oleg.
--
If the tracee calls fork() after PTRACE_SINGLESTEP, the forked child
starts with TIF_SINGLESTEP/X86_EFLAGS_TF bits copied from ptraced parent.
This is not right, especially when the new child is not auto-attaced: in
this case it is killed by SIGTRAP.
Change copy_process() to call user_disable_single_step(). Tested on x86.
Test-case:
#include stdio.h
#include unistd.h
#include signal.h
#include sys/ptrace.h
#include sys/wait.h
#include assert.h
int main(void)
{
int pid, status;
if (!(pid = fork())) {
assert(ptrace(PTRACE_TRACEME) == 0);
kill(getpid(), SIGSTOP);
if (!fork()) {
/* kernel bug: this child will be killed by
SIGTRAP */
printf(Hello world\n);
return 43;
}
wait(status);
return WEXITSTATUS(status);
}
for (;;) {
assert(pid == wait(status));
if (WIFEXITED(status))
break;
assert(ptrace(PTRACE_SINGLESTEP, pid, 0,0) == 0);
}
assert(WEXITSTATUS(status) == 43);
return 0;
}
Signed-off-by: Oleg Nesterov o...@redhat.com
Acked-by: Roland McGrath rol...@redhat.com
Signed-off-by: Andrew Morton a...@linux-foundation.org
---
diff -puN kernel/fork.c~ptrace-copy_process-should-disable-stepping
kernel/fork.c
--- a/kernel/fork.c~ptrace-copy_process-should-disable-stepping
+++ a/kernel/fork.c
@@ -1203,9 +1203,10 @@ static struct task_struct *copy_process(
p-sas_ss_sp = p-sas_ss_size = 0;
/*
-* Syscall tracing should be turned off in the child regardless
-* of CLONE_PTRACE.
+* Syscall tracing and stepping should be turned off in the
+* child regardless of CLONE_PTRACE.
*/
+ user_disable_single_step(p);
clear_tsk_thread_flag(p, TIF_SYSCALL_TRACE);
#ifdef TIF_SYSCALL_EMU
clear_tsk_thread_flag(p, TIF_SYSCALL_EMU);
Re: [RFC,PATCH 0/14] utrace/ptrace
On Tue, Nov 24, 2009 at 09:01:27PM +0100, Oleg Nesterov wrote: Hello. This is the new iteration of Roland's utrace patch, this time with rewrite-ptrace-via-utrace + cleanups in utrace core. 1-7 are already in -mm tree, I am sending them to simplify the review. 8-12 don not change the behaviour, simple preparations. 13-14 add utrace-ptrace and utrace Skipped over it very, very briefly. One thing I really hate about this is that it introduces two ptrace implementation by adding the new one without removing the old one. Given that's it's pretty much too later for the 2.6.33 cycle anyway I'd suggest you make sure the remaining two major architectures (arm and mips) get converted, and if the remaining minor architectures don't manage to get their homework done they're left without ptrace. The other thing is that this patchset really doesn't quite justify utrace. It's growing a lot more code without actually growing any useful functionality. What about all those other utrace killer features that have been promised for a long time?
Re: [RFC,PATCH 0/14] utrace/ptrace
On 11/25, Christoph Hellwig wrote: On Tue, Nov 24, 2009 at 09:01:27PM +0100, Oleg Nesterov wrote: Hello. This is the new iteration of Roland's utrace patch, this time with rewrite-ptrace-via-utrace + cleanups in utrace core. 1-7 are already in -mm tree, I am sending them to simplify the review. 8-12 don not change the behaviour, simple preparations. 13-14 add utrace-ptrace and utrace Skipped over it very, very briefly. One thing I really hate about this is that it introduces two ptrace implementation by adding the new one without removing the old one. Yes, we obviously need the old one when CONFIG_UTRACE is not enabled. So, I'd like to try to restate: one thing we all really hate is that CONFIG_UTRACE exists. Given that's it's pretty much too later for the 2.6.33 cycle anyway I'd suggest you make sure the remaining two major architectures (arm and mips) get converted, and if the remaining minor architectures don't manage to get their homework done they're left without ptrace. Well, I can't comment this. I mean, I can't judge. The other thing is that this patchset really doesn't quite justify utrace. It's growing a lot more code without actually growing any useful functionality. This should be clarified. I don't think ptrace-utrace adds a lot more code compared to the old ptrace. Note that we can kill a lot of old code once CONFIG_UTRACE goes away. ptrace_signal(), ptrace_notify(), even task_struct-almost_all_ptrace_related can go away. kernel/utrace.c does add 12280 bytes (on my machine), yes. What about all those other utrace killer features that have been promised for a long time? It is not clear how we can expect the new killer modules/applications which use utrace before we merge it. We already have some users, say, systemtap. But I don not know what can be counted as a really killer application of utrace. Oleg.
Re: [RFC,PATCH 0/14] utrace/ptrace
Hi Christoph, The other thing is that this patchset really doesn't quite justify utrace. It's growing a lot more code without actually growing any useful functionality. What about all those other utrace killer features that have been promised for a long time? We are working on in-kernel gdbstub which was one of the features that you had asked for. gdbstub does pass unit tests; but we are looking at some way to hack the GDB testsuite to run its regression tests. Once we are able to run the GDB testsuite and utrace is part of some upstream tree, we plan to post these patches to LKML for comments. gdbstub uses utrace and uprobes underneath. Uprobes was rewritten to remove issues that LKML developers had opposed. Uprobes also has its own ftrace plugin to use uprobes. Currently in-kernel gdbstub is hosted by Frank Ch. Eigler over here: git://web.elastic.org/~fche/utrace-ext.git branch name utrace-gdbstub-uprobes -- Regards Srikar
Re: [RFC,PATCH 0/14] utrace/ptrace
On Wed, Nov 25, 2009 at 04:40:52PM +0100, Oleg Nesterov wrote:
On 11/25, Ananth N Mavinakayanahalli wrote:
I ran the ptrace-tests testsuite [1] on powerpc on the vanilla ptrace
and then with ptrace/utrace. The results for ptrace/utrace look better
:-)
Great! thanks a lot Ananth for doing this.
ptrace-utrace still fails 2 tests,
FAIL: syscall-reset
I'll take a look later. Since unpatched kernel fails this test too
I am not going to worry right now. I think this is ppc specific, x86
passes this test.
step-fork: step-fork.c:56: handler_fail: Assertion `0' failed.
/bin/sh: line 5: 24803 Aborted ${dir}$tst
FAIL: step-fork
This is expected. Should be fixed by
ptrace-copy_process-should-disable-stepping.patch
in -mm tree. (I am attaching this patch below just in case)
I din't mention this patch in this series because this bug
is ortogonal to utrace/ptrace.
Oleg,
The patch doesn't seem to fix the issue on powerpc:
step-fork: step-fork.c:56: handler_fail: Assertion `0' failed.
/bin/sh: line 5: 17325 Aborted ${dir}$tst
FAIL: step-fork
Ananth
