Re: [PATCH 133] stepping, accommodate to utrace-cleanup changes

2009-11-20 Thread Roland McGrath 
  The former is e.g. PTRACE_SINGLESTEP while an unrelated engine uses
  UTRACE_EVENT(SYSCALL_ENTRY).  The ptrace engine's report_quiesce returns
  UTRACE_SINGLESTEP.  finish_resume_report() calls user_enable_single_step().
  That seems fine.  Right?
 
 In this case ptrace_report_quiesce(event) returns UTRACE_RESUME, note
 that it does
 
   return event ? UTRACE_RESUME : ctx-resume;
 
 and event == SYSCALL_ENTRY. This looks correct.

With the utrace layer changes we're discussing, we need it to return
UTRACE_SINGLESTEP (i.e. ctx-resume) here.  AFAICT that never hurts
even with today's utrace layer.

  I see.  finish_resume_report() will only do utrace_stop() and then we'll
  go directly into the syscall unless someone used UTRACE_REPORT.
 
 Yes,
 
  utrace_stop() will only set TIF_NOTIFY_RESUME and utrace-resume.  In
  the real resume-to-user cases, that's fine because we'll handle that in
  utrace_resume() or utrace_get_signal() before doing anything else.
 
 Not sure I understand. utrace_stop() will not set TIF_NOTIFY_RESUME?
 
 We are talking about the case when the tracee stops in SYSCALL_ENTER,
 and then the tracer does utrace_control(UTRACE_SINGLESTEP) to resume.
 In this case utrace_control() sets -resume/TIF_NOTIFY_RESUME, yes.

I think you do understand fine.  Yes, that's what it will do.  I was just
recognizing that this isn't enough in the syscall-entry case.

  this
  seems like the right idea for how to get tracehook_report_syscall_exit
  called in the cases where it is in old ptrace.
 
 Afaics yes.
 
 But, what if another engine does utrace_control(UTRACE_INTERRUPT) ?

Hmm.  Yes, I think this is the one case that is unlike all the others.
That is, UTRACE_INTERRUPT at syscall-entry.  In all other cases,
nothing would care about utrace-resume until we get to
get_signal_to_deliver anyway, where the UTRACE_SIGNAL_REPORT pass will
trump any pending resume action anyway so you don't care that you lost
track of it when UTRACE_INTERRUPT came in.

Hmm.  So what does UTRACE_INTERRUPT mean here anyway?  It means that
we should report soon and that signal_pending() should be true until
that report.  So today that means you can get into the syscall with
signal_pending() and depending on the particular call that might cause
a normal blocking path not to block and/or a -EINTR/-ERESTART* return,
but some syscalls will just complete normally.

How about if we say that UTRACE_INTERRUPT at syscall-entry means that
the syscall really doesn't happen at all?  That is, instead, you force
an -ERESTARTNOHAND return and the normal roll-back such that you get
your report_signal callback before the syscall is entered.  That even
seems like a useful utrace API feature, as well as conveniently
smoothing over this quirk in the resume action handling.

My idea here is that this makes it OK to lose UTRACE_SINGLESTEP here
because from the user-mode-centric perspective no instruction has
happened yet.  The original guy who did UTRACE_SINGLESTEP (and perhaps
never cared about syscall tracing) will get a generic report_quiesce
or report_signal at which it needs to reassert UTRACE_SINGLESTEP.

This merits more thought.  For now, let's just leave an XXX comment
about a UTRACE_INTERRUPT effectively swallowing the UTRACE_SINGLESTEP
that should cause utrace_report_syscall_exit to be called.  I think we
can revisit this corner after we have merged up all the rest of it.

 Argh. I meant, from the caller pov, utrace_control(UTRACE_SINGLESTEP)
 does enable_step() immediately, like it did before utrace-cleanup
 changes.

I see.  From the utrace API perspective, I don't think anything changes
here.  In today's code, the resume action can take effect without a
subsequent utrace callback.  So that's the same as it ever was, and where
this actually happens is not something that a utrace caller should know or
can tell.

 IOW, suppose the tracee is stopped, and ptrace does UTRACE_SINGLESTEP.
 The tracee resumes, processes -resume, and does enable_step().
 From the ptrace pov, it looks as if utrace_control() does enable_step()
 before utrace_wakeup().

Sure.

 I meant, every time the tracee stops, it should process -resume
 after wakeup. Looks like, the patch you sent in another thread
 (which adds apply_resume_action()) does something like this, right?

Right.


Thanks,
Roland

Re: [PATCH 133] stepping, accommodate to utrace-cleanup changes

2009-11-19 Thread Oleg Nesterov 
On 11/18, Roland McGrath wrote:

  Hmm. not sure I understand how this can work. I mean, this won't
  enable stepping (in this particular case we are discussing).

 I might be confused.  I'm thinking of two cases: the report_syscall_entry
 pass yields UTRACE_SINGLESTEP, or that it yields UTRACE_STOP and then
 utrace_control() sets utrace-resume to UTRACE_SINGLESTEP.

 The former is e.g. PTRACE_SINGLESTEP while an unrelated engine uses
 UTRACE_EVENT(SYSCALL_ENTRY).  The ptrace engine's report_quiesce returns
 UTRACE_SINGLESTEP.  finish_resume_report() calls user_enable_single_step().
 That seems fine.  Right?

In this case ptrace_report_quiesce(event) returns UTRACE_RESUME, note
that it does

return event ? UTRACE_RESUME : ctx-resume;

and event == SYSCALL_ENTRY. This looks correct.

 The latter is e.g. PTRACE_SINGLESTEP after ptrace stop at syscall entry.

Yes, this is the case I was talking about.

 I see.  finish_resume_report() will only do utrace_stop() and then we'll
 go directly into the syscall unless someone used UTRACE_REPORT.

Yes,

 utrace_stop() will only set TIF_NOTIFY_RESUME and utrace-resume.  In
 the real resume-to-user cases, that's fine because we'll handle that in
 utrace_resume() or utrace_get_signal() before doing anything else.

Not sure I understand. utrace_stop() will not set TIF_NOTIFY_RESUME?

We are talking about the case when the tracee stops in SYSCALL_ENTER,
and then the tracer does utrace_control(UTRACE_SINGLESTEP) to resume.
In this case utrace_control() sets -resume/TIF_NOTIFY_RESUME, yes.

 @@ -1794,11 +1794,13 @@ static void finish_resume_report(struct
  {
   finish_report_reset(task, utrace, report);

 - switch (report-action) {
 - case UTRACE_STOP:
 - utrace_stop(task, utrace, report-resume_action);
 - break;
 + if (report-action == UTRACE_STOP) {
 + utrace_stop(task, utrace, report-resume_action, true);
 + report-action = start_report(utrace);
 + WARN_ON(report-action == UTRACE_STOP);
 + }

 + switch (report-action) {
   case UTRACE_INTERRUPT:
   if (!signal_pending(task))
   set_tsk_thread_flag(task, TIF_SIGPENDING);

Yes, this means the tracee gets TIF_SINGLESTEP right after resume,
and

 this
 seems like the right idea for how to get tracehook_report_syscall_exit
 called in the cases where it is in old ptrace.

Afaics yes.

But, what if another engine does utrace_control(UTRACE_INTERRUPT) ?

  Instead of fixing utrace_report_syscall_entry(), perhaps we should
  change utrace_stop() path to enable the stepping if needed, but this
  means we return to synchronous utrace_control(UTRACE_XXXSTEP).

 I don't really follow what you mean by synchronous.

Argh. I meant, from the caller pov, utrace_control(UTRACE_SINGLESTEP)
does enable_step() immediately, like it did before utrace-cleanup
changes.

IOW, suppose the tracee is stopped, and ptrace does UTRACE_SINGLESTEP.
The tracee resumes, processes -resume, and does enable_step().
From the ptrace pov, it looks as if utrace_control() does enable_step()
before utrace_wakeup().

 Your suggestion is
 equivalent to making utrace_finish_vfork call finish_resume_report instead
 of utrace_stop, isn't it?

I meant, every time the tracee stops, it should process -resume
after wakeup. Looks like, the patch you sent in another thread
(which adds apply_resume_action()) does something like this, right?

I'll try to read it carefully now.

Oleg.

Re: copy_process utrace_init_task (Was: [PATCH 133] stepping, accommodate to utrace-cleanup changes)

2009-11-18 Thread Roland McGrath 
 Found the trivial but nasty problem.

Ah!  Good catch.

I added tracehook_init_task() in my tree.  I don't see much benefit in
sending any tracehook patch upstream for this.  tracehook_init_task()
corresponds to tracehook_free_task(), which is only added by utrace
(and both would just be empty in a separate preparatory patch).

I don't see any reason to fiddle the ptrace_init_task() call.  The
setup it does is all stuff that only matters for teardown done by
release_task() or even earlier in the exit sequence.  So it makes
enough sense that the setup side should happen later as it does now.
In the long run, the ptrace init stuff will all just go away.  Before
then I can't see any rationale for touching it.


Thanks,
Roland

Re: copy_process utrace_init_task (Was: [PATCH 133] stepping, accommodate to utrace-cleanup changes)

2009-11-18 Thread Oleg Nesterov 
On 11/18, Roland McGrath wrote:

 I added tracehook_init_task() in my tree.  I don't see much benefit in
 sending any tracehook patch upstream for this.  tracehook_init_task()
 corresponds to tracehook_free_task(), which is only added by utrace
 (and both would just be empty in a separate preparatory patch).

 I don't see any reason to fiddle the ptrace_init_task() call.
 ...
 In the long run, the ptrace init stuff will all just go away.

OK, agreed.

Oleg.

Re: [PATCH 133] stepping, accommodate to utrace-cleanup changes

2009-11-18 Thread Oleg Nesterov 
On 11/18, Roland McGrath wrote:

  Once again. The tracee sleeps in SYSCALL_ENTER. The tracer resumes
  the tracee via utrace_control(UTRACE_SINGLESTEP).

 Right.  This is another strange corner.  It doesn't necessarily make
 especially good sense intrinsically for single-step to have this meaning
 from this place.  But it's the status quo in the ptrace interface.

Yes.

  So, I think we should do something like
 
  tracehook_report_syscall_exit(step)
 
  // do not use step at all
  if (task_utrace_flags() != 0)
  utrace_report_syscall_exit();

 The trouble here is that the arch code looks like this:

   step = test_thread_flag(TIF_SINGLESTEP);
   if (step || test_thread_flag(TIF_SYSCALL_TRACE))
   tracehook_report_syscall_exit(regs, step);

 i.e. unless syscall tracing is enabled, we won't even get there at all.

You are right, I missed this.

  utrace_report_syscall_exit()
 
  if (UTRACE_EVENT(SYSCALL_EXIT))
  REPORT(report_syscall_exit);
 
  if (utrace-resume == UTRACE_SINGLESTEP ||
  utrace-resume == UTRACE_BLOCKSTEP)
  send_sigtrap();
 
  What do you think?

 This can't work in the REPORT case.  There start_report() will have reset
 utrace-resume,

Yes, yes, the pseudo-code above was just for illustration.

 (This would go with un-reverting f19442c and reverting 3bd4be9.)

 --- a/kernel/utrace.c
 +++ b/kernel/utrace.c
 @@ -1584,20 +1584,25 @@ static inline u32 do_report_syscall_entr
UTRACE_EVENT(SYSCALL_ENTRY), report_syscall_entry,
resume_report | report-result | report-action,
engine, current, regs);
 - finish_report(task, utrace, report, false);
 +
 + /*
 +  * Now we'll stop if asked.  If we're proceeding with the syscall,
 +  * now we'll first enable single-step if asked.  That leaves the
 +  * task in stepping state so tracehook_report_syscall_exit() and
 +  * the arch code that calls it will know.
 +  */
 + finish_resume_report(task, utrace, report);

Hmm. not sure I understand how this can work. I mean, this won't
enable stepping (in this particular case we are discussing).

 - } else if (utrace-resume == UTRACE_REPORT) {
 + } else if (utrace-resume = UTRACE_REPORT) {

perhaps, you meant = UTRACE_REPORT ? In this case yes, UTRACE_SINGLESTEP
will be noticed after resume, and ptrace or another engine can reassert
it during the next reporting loop, and then finish_resume_report() will
enable the stepping. But we can't trust =, another engine can
do utrace_control(INTERRUPT) and change -resume before the tracee
resumes.

OK, probably I missed the implemenation details, but I think I got
the idea, and this should fix the step-after-syscall_enter problem.

But. We have the same problem with utrace_finish_vfork(), no?


Instead of fixing utrace_report_syscall_entry(), perhaps we should
change utrace_stop() path to enable the stepping if needed, but this
means we return to synchronous utrace_control(UTRACE_XXXSTEP).

Oleg.

Re: [PATCH 133] stepping, accommodate to utrace-cleanup changes

2009-11-18 Thread Oleg Nesterov 
I'll reply tomorrow, it is to late for me.

But I noticed the funny detail in your email,

On 11/18, Roland McGrath wrote:

 Yes.  But, hmm.

   utrace_stop(task, utrace, UTRACE_RESUME); /* XXX */

 This XXX was there about passing UTRACE_RESUME, because that's not really
 right.

I didn't notice this XXX marker, but I guess I just sent the patch
which makes this case correct. (of course this patch doesn't fix
the problem we are dicussing in this thread).

Oleg.

copy_process utrace_init_task (Was: [PATCH 133] stepping, accommodate to utrace-cleanup changes)

2009-11-17 Thread Oleg Nesterov 
On 11/16, Oleg Nesterov wrote:

 On 11/16, Oleg Nesterov wrote:
 
  And I didn't check make xcheck, I guess it still crashes the kernel.

 Yes it does. I am almost sure the bug should be trivial, but
 somehow can't find find it.

Found the trivial but nasty problem.

tracehook_finish_clone()-utrace_init_task() sets -utrace = NULL, but
this is too late. If copy_process() fails before, tracehook_free_task()
will free parent's -utrace copied by dup_task_struct().

This is nasty because we need some changes outside of utrace/tracehook
files. Perhaps we should send something like the patch below to Andrew
right now?


And! While this bug could perfectly explain the crash, it doesn't.
I appiled this patch

--- UTRACE-PTRACE/kernel/fork.c~XXX_CRASH   2009-11-16 
20:26:23.0 +0100
+++ UTRACE-PTRACE/kernel/fork.c 2009-11-17 20:33:50.0 +0100
@@ -1019,6 +1019,7 @@ static struct task_struct *copy_process(
if (!p)
goto fork_out;
 
+p-utrace = NULL;
ftrace_graph_init_task(p);
 
rt_mutex_init_task(p);

but make xcheck still crashes. Still investigating.

Oleg.

--- TH/include/linux/ptrace.h~TH_INIT_TASK  2009-11-10 21:31:25.0 
+0100
+++ TH/include/linux/ptrace.h   2009-11-17 20:43:42.0 +0100
@@ -148,6 +148,14 @@ static inline int ptrace_event(int mask,
return 1;
 }
 
+static inline void ptrace_init_task(struct task_struct *child)
+{
+   INIT_LIST_HEAD(child-ptrace_entry);
+   INIT_LIST_HEAD(child-ptraced);
+   child-parent = child-real_parent;
+   child-ptrace = 0;
+}
+
 /**
  * ptrace_init_task - initialize ptrace state for a new child
  * @child: new child task
@@ -158,12 +166,8 @@ static inline int ptrace_event(int mask,
  *
  * Called with current's siglock and write_lock_irq(tasklist_lock) held.
  */
-static inline void ptrace_init_task(struct task_struct *child, bool ptrace)
+static inline void ptrace_finish_clone(struct task_struct *child, bool ptrace)
 {
-   INIT_LIST_HEAD(child-ptrace_entry);
-   INIT_LIST_HEAD(child-ptraced);
-   child-parent = child-real_parent;
-   child-ptrace = 0;
if (unlikely(ptrace)  (current-ptrace  PT_PTRACED)) {
child-ptrace = current-ptrace;
__ptrace_link(child, current-parent);
--- TH/include/linux/tracehook.h~TH_INIT_TASK   2009-11-11 21:49:42.0 
+0100
+++ TH/include/linux/tracehook.h2009-11-17 20:42:43.0 +0100
@@ -247,6 +247,11 @@ static inline int tracehook_prepare_clon
return 0;
 }
 
+static inline void tracehook_init_task(struct task_struct *child)
+{
+   ptrace_init_task(child);
+}
+
 /**
  * tracehook_finish_clone - new child created and being attached
  * @child: new child task
@@ -261,7 +266,7 @@ static inline int tracehook_prepare_clon
 static inline void tracehook_finish_clone(struct task_struct *child,
  unsigned long clone_flags, int trace)
 {
-   ptrace_init_task(child, (clone_flags  CLONE_PTRACE) || trace);
+   ptrace_finish_clone(child, (clone_flags  CLONE_PTRACE) || trace);
 }
 
 /**
--- TH/kernel/fork.c~TH_INIT_TASK   2009-11-07 22:15:15.0 +0100
+++ TH/kernel/fork.c2009-11-17 20:40:52.0 +0100
@@ -1018,8 +1018,8 @@ static struct task_struct *copy_process(
if (!p)
goto fork_out;
 
+   tracehook_init_task(p);
ftrace_graph_init_task(p);
-
rt_mutex_init_task(p);
 
 #ifdef CONFIG_PROVE_LOCKING

tracehook_report_syscall_exit() PT_PTRACED (Was: [PATCH 133] stepping, accommodate to utrace-cleanup changes)

2009-11-17 Thread Oleg Nesterov 
On 11/16, Roland McGrath wrote:

 The change we talked about before seems simple enough and should cover this
 without new kludges in the ptrace layer.  I did this (commit f19442c).

I will reply to this in the next email, I'd like to discuss
another minor related issue first.

I noticed this change in your tree

commit 2583b3267ed599cb25f6f893c24465204e06b3a6
utrace: nit for utrace-ptrace

--- a/include/linux/tracehook.h
+++ b/include/linux/tracehook.h
@@ -143,7 +143,7 @@ static inline void 
tracehook_report_syscall_exit(struct pt_regs *regs, int step)
if (task_utrace_flags(current)  UTRACE_EVENT(SYSCALL_EXIT))
utrace_report_syscall_exit(regs);
 
-   if (step  task_ptrace(current)) {
+   if (step  (task_ptrace(current)  PT_PTRACED)) {
siginfo_t info;
user_single_step_siginfo(current, regs, info);
force_sig_info(SIGTRAP, info, current);

Yes, and in fact I already had this change in my tree but didn't
send it to you yet.

But, I can't understand whats going on,

-   if (step  task_ptrace(current)) {
+   if (step  (task_ptrace(current)  PT_PTRACED)) {

The code after 
ptrace-change-tracehook_report_syscall_exit-to-handle-stepping.patch
is if (step), not if (step  task_ptrace(current)), can't understand
where did this  task_ptrace(current) come from. The previous commit in
your tree is

462a57bb7a6a5c67b70e4388f97239f1e4da98df
ptrace: change tracehook_report_syscall_exit() to handle stepping


Initially I was going to add the change below into
tracehooks: prepare for utrace-ptrace,

--- a/include/linux/tracehook.h
+++ b/include/linux/tracehook.h
@@ -134,6 +134,9 @@ static inline __must_check int tracehook
  */
 static inline void tracehook_report_syscall_exit(struct pt_regs *regs, 
int step)
 {
+   if (!(task_ptrace(current)  PT_PTRACED))
+   return;
+
if (step) {
siginfo_t info;
user_single_step_siginfo(current, regs, info);

but now I think perhaps it would be better to send
ptrace-change-tracehook_report_syscall_exit-to-handle-stepping_fix
to akpm right now:

--- a/include/linux/tracehook.h
+++ b/include/linux/tracehook.h
@@ -134,7 +134,7 @@ static inline __must_check int tracehook
  */
 static inline void tracehook_report_syscall_exit(struct pt_regs *regs, 
int step)
 {
-   if (step) {
+   if (step  (task_ptrace(current)  PT_PTRACED)) {
siginfo_t info;
user_single_step_siginfo(current, regs, info);
force_sig_info(SIGTRAP, info, current);

What do you think?

Now, let's return to the original topic. Please note that utrace
does not need int step at all, see the next email.

Oleg.

Re: [PATCH 133] stepping, accommodate to utrace-cleanup changes

2009-11-17 Thread Oleg Nesterov 
On 11/16, Roland McGrath wrote:

 Whatever temporary hacks are fine by me one way or the other.
 They will just be coming out later along with assorted other cleanup.
 We certainly do want to get this right in the utrace layer.

Yes. But imho it is always good to test/review the patches against
the working kernel. The patch I sent is very simple, and can be
easily reverted once we improve utrace.

IOW, I am asking you to apply my patch for now and revert your
change to have the working tree, then discuss the right fix.

 The change we talked about before seems simple enough and should cover this
 without new kludges in the ptrace layer.  I did this (commit f19442c).

I don't think this can work.

tracehook_report_syscall_exit(step)

if (step || UTRACE_EVENT(SYSCALL_EXIT))
utrace_report_syscall_exit(step);

and,
utrace_report_syscall_exit(step)

if (step)
send_sigtrap();

The problems is: we can trust bool step, and in fact we do
not need it at all.

Once again. The tracee sleeps in SYSCALL_ENTER. The tracer resumes
the tracee via utrace_control(UTRACE_SINGLESTEP).

By the time the resumed tracee passes tracehook_report_syscall_exit()
step == F, utrace_control() does not set TIF_SINGLESTEP.

So, I think we should do something like

tracehook_report_syscall_exit(step)

// do not use step at all
if (task_utrace_flags() != 0)
utrace_report_syscall_exit();

// this code below is only for old ptrace

if (step  (task_ptrace(current)  PT_PTRACED))
send_sigtrap();
ptrace_report_syscall();

and,
utrace_report_syscall_exit()

if (UTRACE_EVENT(SYSCALL_EXIT))
REPORT(report_syscall_exit);

if (utrace-resume == UTRACE_SINGLESTEP ||
utrace-resume == UTRACE_BLOCKSTEP)
send_sigtrap();

What do you think?

Oleg.

Re: tracehook_report_syscall_exit() PT_PTRACED (Was: [PATCH 133] stepping, accommodate to utrace-cleanup changes)

2009-11-17 Thread Roland McGrath 
 but now I think perhaps it would be better to send
 ptrace-change-tracehook_report_syscall_exit-to-handle-stepping_fix
 to akpm right now:
 
   --- a/include/linux/tracehook.h
   +++ b/include/linux/tracehook.h
   @@ -134,7 +134,7 @@ static inline __must_check int tracehook
 */
static inline void tracehook_report_syscall_exit(struct pt_regs *regs, 
 int step)
{
   -   if (step) {
   +   if (step  (task_ptrace(current)  PT_PTRACED)) {
   siginfo_t info;
   user_single_step_siginfo(current, regs, info);
   force_sig_info(SIGTRAP, info, current);
 
 What do you think?

Yes, this makes it consistent with the x86 behavior before the change,
which used tracehook_consider_fatal_signal(current, SIGTRAP) in its test.


Thanks,
Roland

[PATCH 133] stepping, accommodate to utrace-cleanup changes

2009-11-16 Thread Oleg Nesterov 
This needs more comment, I'll try to add them in a separate patch.

With the recent changes in utrace API utrace_control(UTRACE_SINGLESTEP)
postpones enable_step() to utrace_resume() stage. The tracee can pass
tracehook_report_syscall_exit() without TIF_SINGLESTEP, this breaks
the send_sigtrap() logic.

Change ptrace_resume_action() to set UTRACE_EVENT(SYSCALL_EXIT) in
PTRACE_SINGLESTEP/PTRACE_SINGLEBLOCK case, change report_syscall_exit()
to check ctx-resume to detect this case. To synthesize the trap we
set ctx-signr = SIGTRAP and provoke UTRACE_SIGNAL_REPORT, like
ptrace_resume() does.

Roland, we already discussed this a bit and I agree, this is not optimal.
We need some changes in utrace, but currently it has a lot of problems
and it is not clear yet what utrace should do. Let's fix the code now,
once we find a better solution we can revert this change.

With this patch make check passes all tests again (to clarify, except
some tests which upstream doesn't pass too), including this one:

#include stdio.h
#include unistd.h
#include stdlib.h
#include signal.h
#include errno.h
#include sys/wait.h
#include sys/ptrace.h
#include assert.h
#include sys/user.h
#include sys/debugreg.h
#include sys/syscall.h

#define WEVENT(s) ((s  0xFF)  16)

static int verbose;

#define d_printf(fmt, ...) do { if (verbose) printf(fmt, 
##__VA_ARGS__); } while (0)

static struct user_regs_struct regs;

static void resume(int pid, int req, int ck_stat)
{
int stat;

assert(0 == ptrace(req, pid, 0, 0));
assert(waitpid(pid, stat, __WALL) == pid);
//d_printf(=== %06X\n %06X\n, ck_stat, stat);
assert(stat == ck_stat);

assert(0 == ptrace(PTRACE_GETREGS, pid, NULL, regs));
}

int main(int argc, const char *argv[])
{
int pid, child, stat;
long rip, nxt_rip;

if (getpid() == __NR_getppid) {
printf(sorry, restart\n);
return 0;
}

verbose = argc  1;

pid = fork();
if (!pid) {
assert(0 == ptrace(PTRACE_TRACEME, 0,0,0));
kill(getpid(), SIGSTOP);

// 1: SYSCALL + SYSCALL + STEP
getppid();

// 2: SYSCALL + STEP
getppid();

// 3: STEP
getppid();

// 4: SYSCALL + STEP
if (!fork()) exit(73);

// STEPs only
if (!fork()) exit(73);

assert(0);
}

assert(wait(stat) == pid);
assert(WIFSTOPPED(stat)  WSTOPSIG(stat) == SIGSTOP);

assert(0 == ptrace(PTRACE_SETOPTIONS, pid, 0,
PTRACE_O_TRACESYSGOOD | 
PTRACE_O_TRACEFORK));


//-
d_printf(1: syscall enter\n);
resume(pid, PTRACE_SYSCALL, 0x857F);
assert(regs.orig_rax == __NR_getppid);
assert(regs.rax == -ENOSYS);
rip = regs.rip;

d_printf(1: syscall leave\n);
resume(pid, PTRACE_SYSCALL, 0x857F);
assert(regs.orig_rax == __NR_getppid);
assert(regs.rax == getpid());
assert(regs.rip == rip);

d_printf(1: singlestep\n);
resume(pid, PTRACE_SINGLESTEP, 0x57F);
assert((long)regs.orig_rax  0);
assert(regs.rax == getpid());
assert(regs.rip != rip);

d_printf(1: singlestep\n);
resume(pid, PTRACE_SINGLESTEP, 0x57F);
assert(regs.rip != rip);


//
d_printf(2: stop before syscall insn\n);
do {
resume(pid, PTRACE_SINGLESTEP, 0x57F);
} while (regs.rip != rip - 2);
assert(regs.rax == __NR_getppid);

d_printf(2: syscall enter\n);
resume(pid, PTRACE_SYSCALL, 0x857F);
assert(regs.orig_rax == __NR_getppid);
assert(regs.rax == -ENOSYS);
assert(regs.rip == rip);

d_printf(2: singlestep\n);
resume(pid, PTRACE_SINGLESTEP, 0x57F);
assert(regs.orig_rax == __NR_getppid);
assert(regs.rax == getpid());
assert(regs.rip == rip);

d_printf(2: singlestep\n);
resume(pid, PTRACE_SINGLESTEP,

Re: [PATCH 133] stepping, accommodate to utrace-cleanup changes

2009-11-16 Thread Oleg Nesterov 
Just in case, forgot to clarify...

On 11/16, Oleg Nesterov wrote:

 With this patch make check passes all tests again (to clarify, except
 some tests which upstream doesn't pass too), including this one:

with this patch +
[HACK] utrace: fix utrace_resume()-finish_resume_report() logic

And I didn't check make xcheck, I guess it still crashes the kernel.

Oleg.

Re: [PATCH 133] stepping, accommodate to utrace-cleanup changes

2009-11-16 Thread Oleg Nesterov 
On 11/16, Oleg Nesterov wrote:

 And I didn't check make xcheck, I guess it still crashes the kernel.

Yes it does. I am almost sure the bug should be trivial, but
somehow can't find find it. Just fyi, to ensure this is connected
to utrace-indirect I applied the hack below and the bug goes away.

OK. Tomorrow I will just read utrace.c trying to understand the
details of the new code, perhaps I will notice something.

Oleg.

--- UTRACE-PTRACE/kernel/fork.c~XXX_CRASH   2009-11-16 20:26:23.0 
+0100
+++ UTRACE-PTRACE/kernel/fork.c 2009-11-16 21:18:57.0 +0100
@@ -970,6 +970,8 @@ static void posix_cpu_timers_init(struct
  * parts of the process environment (as per the clone
  * flags). The actual kick-off is left to the caller.
  */
+bool utrace_task_alloc(struct task_struct *task);
+
 static struct task_struct *copy_process(unsigned long clone_flags,
unsigned long stack_start,
struct pt_regs *regs,
@@ -1227,6 +1229,10 @@ static struct task_struct *copy_process(
cgroup_fork_callbacks(p);
cgroup_callbacks_done = 1;
 
+   p-utrace = NULL;
+   if (!utrace_task_alloc(p))
+   WARN_ON(1);
+
/* Need tasklist lock for parent etc handling! */
write_lock_irq(tasklist_lock);
 
--- UTRACE-PTRACE/kernel/utrace.c~XXX_CRASH 2009-11-16 20:31:38.0 
+0100
+++ UTRACE-PTRACE/kernel/utrace.c   2009-11-16 21:19:26.0 +0100
@@ -87,9 +87,9 @@ module_init(utrace_init);
  *
  * This returns false only in case of a memory allocation failure.
  */
-static bool utrace_task_alloc(struct task_struct *task)
+bool utrace_task_alloc(struct task_struct *task)
 {
-   struct utrace *utrace = kmem_cache_zalloc(utrace_cachep, GFP_KERNEL);
+   struct utrace *utrace = kzalloc(sizeof(struct utrace), GFP_KERNEL);
if (unlikely(!utrace))
return false;
spin_lock_init(utrace-lock);
@@ -108,7 +108,7 @@ static bool utrace_task_alloc(struct tas
}
task_unlock(task);
if (unlikely(task-utrace != utrace))
-   kmem_cache_free(utrace_cachep, utrace);
+   kfree(utrace);
return true;
 }
 
@@ -118,7 +118,7 @@ static bool utrace_task_alloc(struct tas
  */
 void utrace_free_task(struct task_struct *task)
 {
-   kmem_cache_free(utrace_cachep, task-utrace);
+   kfree(task-utrace);
 }
 
 /*
@@ -286,6 +286,8 @@ struct utrace_engine *utrace_attach_task
struct utrace_engine *engine;
int ret;
 
+   WARN_ON(!utrace);
+
if (!(flags  UTRACE_ATTACH_CREATE)) {
if (unlikely(!utrace))
return ERR_PTR(-ENOENT);
--- UTRACE-PTRACE/include/linux/utrace.h~XXX_CRASH  2009-11-16 
01:06:34.0 +0100
+++ UTRACE-PTRACE/include/linux/utrace.h2009-11-16 20:50:38.0 
+0100
@@ -163,7 +163,7 @@ static inline struct utrace *task_utrace
 static inline void utrace_init_task(struct task_struct *task)
 {
task-utrace_flags = 0;
-   task-utrace = NULL;
+   WARN_ON(!task-utrace);
 }
 
 void utrace_release_task(struct task_struct *);

Re: [PATCH 133] stepping, accommodate to utrace-cleanup changes

2009-11-16 Thread Roland McGrath 
Whatever temporary hacks are fine by me one way or the other.
They will just be coming out later along with assorted other cleanup.
We certainly do want to get this right in the utrace layer.

The change we talked about before seems simple enough and should cover this
without new kludges in the ptrace layer.  I did this (commit f19442c).
I think that should cover what you need without this ptrace change.
(All untested, of course!)


Thanks,
Roland

Re: [PATCH 133] stepping, accommodate to utrace-cleanup changes

2009-11-16 Thread Roland McGrath 
 Yes it does. I am almost sure the bug should be trivial, but
 somehow can't find find it. Just fyi, to ensure this is connected
 to utrace-indirect I applied the hack below and the bug goes away.

Does s/kmem_cache_zalloc/kzalloc/ really have anything to do with it?
Isn't it just the allocation synchronization logic?

 OK. Tomorrow I will just read utrace.c trying to understand the
 details of the new code, perhaps I will notice something.

Ok.  I hope you'll find something not too strange.  But if it takes long,
then probably we should just change utrace_init_task() to eagerly call
utrace_task_alloc() and let you work through other issues first.


Thanks,
Roland