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.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.

With or without my bug fix ?

-- 
blue skies,
   Martin.

"Reality continues to ruin my life." - Calvin.

Reply via email to