Re: s390 user_enable_single_step() (Was: odd utrace testing results on s390x)

2010-01-05 Thread Martin Schwidefsky 
On Mon, 4 Jan 2010 19:14:12 +0100
Oleg Nesterov o...@redhat.com wrote:

 On 01/04, Martin Schwidefsky wrote:
  Subject: [PATCH] fix loading of PER control registers for utrace.
 
  From: Martin Schwidefsky schwidef...@de.ibm.com
 
  If the current task enables / disables PER tracing for itself the
  PER control registers need to be loaded in FixPerRegisters.
 
  Signed-off-by: Martin Schwidefsky schwidef...@de.ibm.com
  ---
   arch/s390/kernel/ptrace.c |3 +++
   1 file changed, 3 insertions(+)
 
  --- a/arch/s390/kernel/ptrace.c
  +++ b/arch/s390/kernel/ptrace.c
  @@ -98,6 +98,9 @@ FixPerRegisters(struct task_struct *task
  per_info-control_regs.bits.storage_alt_space_ctl = 1;
  else
  per_info-control_regs.bits.storage_alt_space_ctl = 0;
  +
  +   if (task == current)
  +   __ctl_load(per_info-control_regs.words, 9, 11);
   }
 
 Yes it does fix the problem! Thanks a lot Martin.

Ok, I will add that patch to the git390 queue.

 However. Could you please look at 6580807da14c423f0d0a708108e6df6ebc8bc83d ?
 I am worried, perhaps this commit is not enough for s390. OK, do_single_step()
 tracehook_consider_fatal_signal(), this means the forked thread will not
 be killed by SIGTRAP if it is not auto-attached, but still this may be
 wrong.
 
 IOW. I think this problem is minor and probably can be ignored, but if
 I remove tracehook_consider_fatal_signal() check from do_single_step(),
 
 --- a/arch/s390/kernel/traps.c
 +++ b/arch/s390/kernel/traps.c
 @@ -382,8 +382,7 @@ void __kprobes do_single_step(struct pt_
   SIGTRAP) == NOTIFY_STOP){
   return;
   }
 - if (tracehook_consider_fatal_signal(current, SIGTRAP))
 - force_sig(SIGTRAP, current);
 + force_sig(SIGTRAP, current);
  }
 
  static void default_trap_handler(struct pt_regs * regs, long 
 interruption_code)
 ---
 
 then the test-case from 6580807da14c423f0d0a708108e6df6ebc8bc83d
 fails. This probably means that copy_process()-user_disable_single_step()
 is not enough to clear the this task wants single-stepping copied
 from parent.

user_disable_single_step() does not remove the TIF_SINGLE_STEP bit from the
forked task. Perhaps we should just clear the bit in the function.

-- 
blue skies,
   Martin.

Reality continues to ruin my life. - Calvin.

Re: s390 user_enable_single_step() (Was: odd utrace testing results on s390x)

2010-01-05 Thread Oleg Nesterov 
On 01/05, Martin Schwidefsky wrote:

 On Mon,  4 Jan 2010 13:11:47 -0800 (PST)
 Roland McGrath rol...@redhat.com wrote:

   This probably means that copy_process()-user_disable_single_step()
   is not enough to clear the this task wants single-stepping copied
   from parent.
 
  I would suspect s390's TIF_SINGLE_STEP flag here.  That flag means a
  single-step trap occurred.  This is what causes do_single_step to be
  called before returning to user mode, rather than the machine trap doing it
  directly as is done in the other arch implementations.

 Just my thinking as well.

Oh, I am not sure. But I don't understand TIF_SINGLE_STEP on s390,
absolutely.

For example, why do_signal() sets TIF_SINGLE_STEP? Why can't we do

--- a/arch/s390/kernel/signal.c
+++ b/arch/s390/kernel/signal.c
@@ -500,18 +500,10 @@ void do_signal(struct pt_regs *regs)
clear_thread_flag(TIF_RESTORE_SIGMASK);
 
/*
-* If we would have taken a single-step trap
-* for a normal instruction, act like we took
-* one for the handler setup.
-*/
-   if (current-thread.per_info.single_step)
-   set_thread_flag(TIF_SINGLE_STEP);
-
-   /*
 * Let tracing know that we've done the handler 
setup.
 */
tracehook_signal_handler(signr, info, ka, 
regs,
-
test_thread_flag(TIF_SINGLE_STEP));
+   
current-thread.per_info.single_step);
}
return;
}

?

Apart from arch/s390/signal.c, TIF_SINGLE_STEP is used by entry.S
but I don't understand this asm at all.

Anyway. I modified the debugging patch a bit:

--- K/arch/s390/kernel/traps.c~ 2009-12-22 10:41:52.909174198 -0500
+++ K/arch/s390/kernel/traps.c  2010-01-05 09:49:19.541792379 -0500
@@ -384,6 +384,8 @@ void __kprobes do_single_step(struct pt_
}
if (tracehook_consider_fatal_signal(current, SIGTRAP))
force_sig(SIGTRAP, current);
+   else
+   printk(XXX: %d %d\n, current-pid, 
test_thread_flag(TIF_SINGLE_STEP));
 }
 
 static void default_trap_handler(struct pt_regs * regs, long interruption_code)
---

Now, when I run this 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())
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;
}

dmesg shows 799 lines of

XXX: 2389 0


The kernel is 2.6.32.2 + utrace, but CONFIG_UTRACE is not set.

Oleg.

Re: s390 user_enable_single_step() (Was: odd utrace testing results on s390x)

2010-01-05 Thread Martin Schwidefsky 
On Tue, 5 Jan 2010 16:36:33 +0100
Oleg Nesterov o...@redhat.com wrote:

 On 01/05, Martin Schwidefsky wrote:
 
  On Mon,  4 Jan 2010 13:11:47 -0800 (PST)
  Roland McGrath rol...@redhat.com wrote:
 
This probably means that copy_process()-user_disable_single_step()
is not enough to clear the this task wants single-stepping copied
from parent.
  
   I would suspect s390's TIF_SINGLE_STEP flag here.  That flag means a
   single-step trap occurred.  This is what causes do_single_step to be
   called before returning to user mode, rather than the machine trap doing 
   it
   directly as is done in the other arch implementations.
 
  Just my thinking as well.
 
 Oh, I am not sure. But I don't understand TIF_SINGLE_STEP on s390,
 absolutely.
 
 For example, why do_signal() sets TIF_SINGLE_STEP? Why can't we do
 
   --- a/arch/s390/kernel/signal.c
   +++ b/arch/s390/kernel/signal.c
   @@ -500,18 +500,10 @@ void do_signal(struct pt_regs *regs)
   clear_thread_flag(TIF_RESTORE_SIGMASK);

   /*
   -* If we would have taken a single-step trap
   -* for a normal instruction, act like we took
   -* one for the handler setup.
   -*/
   -   if (current-thread.per_info.single_step)
   -   set_thread_flag(TIF_SINGLE_STEP);
   -
   -   /*
* Let tracing know that we've done the handler 
 setup.
*/
   tracehook_signal_handler(signr, info, ka, 
 regs,
   -
 test_thread_flag(TIF_SINGLE_STEP));
   +   
 current-thread.per_info.single_step);
   }
   return;
   }
 
 ?

The reason why we set the TIF_SINGLE_STEP bit in do_signal is that we
want to be able to stop the debugged program before the first
instruction of the signal handler has been executed. The PER single
step causes a trap after an instruction has been executed. That first
instruction can do bad things to the arguments of the signal handler..

 Apart from arch/s390/signal.c, TIF_SINGLE_STEP is used by entry.S
 but I don't understand this asm at all.
 
 Anyway. I modified the debugging patch a bit:
 
 --- K/arch/s390/kernel/traps.c~   2009-12-22 10:41:52.909174198 -0500
 +++ K/arch/s390/kernel/traps.c2010-01-05 09:49:19.541792379 -0500
 @@ -384,6 +384,8 @@ void __kprobes do_single_step(struct pt_
   }
   if (tracehook_consider_fatal_signal(current, SIGTRAP))
   force_sig(SIGTRAP, current);
 + else
 + printk(XXX: %d %d\n, current-pid, 
 test_thread_flag(TIF_SINGLE_STEP));
  }
 
  static void default_trap_handler(struct pt_regs * regs, long 
 interruption_code)
 ---
 
 Now, when I run this 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())
   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;
   }
 
 dmesg shows 799 lines of
 
   XXX: 2389 0
 
 
 The kernel is 2.6.32.2 + utrace, but CONFIG_UTRACE is not set.

With or without my bug fix ?

-- 
blue skies,
   Martin.

Reality continues to ruin my life. - Calvin.

Re: s390 user_enable_single_step() (Was: odd utrace testing results on s390x)

2010-01-05 Thread Oleg Nesterov 
On 01/05, Oleg Nesterov wrote:

 Anyway. I modified the debugging patch a bit:

 --- K/arch/s390/kernel/traps.c~   2009-12-22 10:41:52.909174198 -0500
 +++ K/arch/s390/kernel/traps.c2010-01-05 09:49:19.541792379 -0500
 @@ -384,6 +384,8 @@ void __kprobes do_single_step(struct pt_
   }
   if (tracehook_consider_fatal_signal(current, SIGTRAP))
   force_sig(SIGTRAP, current);
 + else
 + printk(XXX: %d %d\n, current-pid, 
 test_thread_flag(TIF_SINGLE_STEP));
  }

  static void default_trap_handler(struct pt_regs * regs, long 
 interruption_code)
 ---

Ah, please ignore. I guess TIF_SINGLE_STEP was already cleared by the caller
in entry.S

Oleg.

Re: s390 user_enable_single_step() (Was: odd utrace testing results on s390x)

2010-01-05 Thread Martin Schwidefsky 
On Tue, 5 Jan 2010 16:47:25 +0100
Oleg Nesterov o...@redhat.com wrote:

 On 01/05, Oleg Nesterov wrote:
 
  Anyway. I modified the debugging patch a bit:
 
  --- K/arch/s390/kernel/traps.c~ 2009-12-22 10:41:52.909174198 -0500
  +++ K/arch/s390/kernel/traps.c  2010-01-05 09:49:19.541792379 -0500
  @@ -384,6 +384,8 @@ void __kprobes do_single_step(struct pt_
  }
  if (tracehook_consider_fatal_signal(current, SIGTRAP))
  force_sig(SIGTRAP, current);
  +   else
  +   printk(XXX: %d %d\n, current-pid, 
  test_thread_flag(TIF_SINGLE_STEP));
   }
 
   static void default_trap_handler(struct pt_regs * regs, long 
  interruption_code)
  ---
 
 Ah, please ignore. I guess TIF_SINGLE_STEP was already cleared by the caller
 in entry.S

Yes, TIF_SINGLE_STEP is checked in entry.S and cleared before do_signal
is called. That is the ni instruction at sysc_singlestep and
sysc_sigpending.

-- 
blue skies,
   Martin.

Reality continues to ruin my life. - Calvin.

Re: s390 user_enable_single_step() (Was: odd utrace testing results on s390x)

2010-01-05 Thread Oleg Nesterov 
On 01/05, Martin Schwidefsky wrote:

 On Tue, 5 Jan 2010 16:36:33 +0100
 Oleg Nesterov o...@redhat.com wrote:

  For example, why do_signal() sets TIF_SINGLE_STEP? Why can't we do
  
  --- a/arch/s390/kernel/signal.c
  +++ b/arch/s390/kernel/signal.c
  @@ -500,18 +500,10 @@ void do_signal(struct pt_regs *regs)
  clear_thread_flag(TIF_RESTORE_SIGMASK);
   
  /*
  -* If we would have taken a single-step trap
  -* for a normal instruction, act like we took
  -* one for the handler setup.
  -*/
  -   if (current-thread.per_info.single_step)
  -   set_thread_flag(TIF_SINGLE_STEP);
  -
  -   /*
   * Let tracing know that we've done the handler 
  setup.
   */
  tracehook_signal_handler(signr, info, ka, 
  regs,
  -
  test_thread_flag(TIF_SINGLE_STEP));
  +   
  current-thread.per_info.single_step);
  }
  return;
  }
  
  ?

 The reason why we set the TIF_SINGLE_STEP bit in do_signal is that we
 want to be able to stop the debugged program before the first
 instruction of the signal handler has been executed. The PER single
 step causes a trap after an instruction has been executed. That first
 instruction can do bad things to the arguments of the signal handler..

Yes, but afaics all we need is to pass the correct int stepping arg
to tracehook_signal_handler(). If it is true, the tracee does
ptrace_notify() before it returns to user-mode.

  dmesg shows 799 lines of
 
  XXX: 2389 0
 
 
  The kernel is 2.6.32.2 + utrace, but CONFIG_UTRACE is not set.

 With or without my bug fix ?

With, but please see another email.


I'll add clear_bit(TIF_SINGLE_STEP) into do_fork() path and re-test.

Oleg.