Re: s390 user_enable_single_step() (Was: odd utrace testing results on s390x)
On Thu, 21 Jan 2010 21:51:13 +0100 Oleg Nesterov o...@redhat.com wrote: On 01/07, Roland McGrath wrote: I am confused as well. Yes, I thought about regs-psw.mask change too, but I don't understand why it helps.. [...] But. Acoording to the testing I did (unless I did something wrong again) this patch doesn't make any difference in this particular case. 6580807da14c423f0d0a708108e6df6ebc8bc83d does. Those results are quite mysterious to me. I think we'll have to get Martin to sort it out definitively. Finally nailed that one. Grrmpf.. the special case in the program check handler for single stepped svcs clobbers the argument registers. With our test case this affects the clone() system call. Funny things happen when the clone_flags argument is more or less random .. The following patch fixes the problem for me. -- Subject: [PATCH] fix single stepped svcs with TRACE_IRQFLAGS=y From: Martin Schwidefsky schwidef...@de.ibm.com If irq flags tracing is enabled the TRACE_IRQS_ON macros expands to a function call which clobbers registers %r0-%r5. The macro is used in the code path for single stepped system calls. The argument registers %r2-%r6 need to be restored from the stack before the system call function is called. Cc: sta...@kernel.org Signed-off-by: Martin Schwidefsky schwidef...@de.ibm.com --- arch/s390/kernel/entry.S |1 + arch/s390/kernel/entry64.S |1 + 2 files changed, 2 insertions(+) diff -urpN linux-2.6/arch/s390/kernel/entry64.S linux-2.6-patched/arch/s390/kernel/entry64.S --- linux-2.6/arch/s390/kernel/entry64.S2009-12-03 04:51:21.0 +0100 +++ linux-2.6-patched/arch/s390/kernel/entry64.S2010-01-26 14:04:58.0 +0100 @@ -549,6 +549,7 @@ pgm_svcper: mvc __THREAD_per+__PER_access_id(1,%r8),__LC_PER_ACCESS_ID oi __TI_flags+7(%r9),_TIF_SINGLE_STEP # set TIF_SINGLE_STEP TRACE_IRQS_ON + lmg %r2,%r6,SP_R2(%r15) # load svc arguments stosm __SF_EMPTY(%r15),0x03 # reenable interrupts j sysc_do_svc diff -urpN linux-2.6/arch/s390/kernel/entry.S linux-2.6-patched/arch/s390/kernel/entry.S --- linux-2.6/arch/s390/kernel/entry.S 2009-12-03 04:51:21.0 +0100 +++ linux-2.6-patched/arch/s390/kernel/entry.S 2010-01-26 14:04:58.0 +0100 @@ -571,6 +571,7 @@ pgm_svcper: mvc __THREAD_per+__PER_access_id(1,%r8),__LC_PER_ACCESS_ID oi __TI_flags+3(%r9),_TIF_SINGLE_STEP # set TIF_SINGLE_STEP TRACE_IRQS_ON + lm %r2,%r6,SP_R2(%r15) # load svc arguments stosm __SF_EMPTY(%r15),0x03 # reenable interrupts b BASED(sysc_do_svc) -- blue skies, Martin. Reality continues to ruin my life. - Calvin.
Re: s390 user_enable_single_step() (Was: odd utrace testing results on s390x)
On Thu, 7 Jan 2010 13:46:42 -0800 (PST) Roland McGrath rol...@redhat.com wrote: Clear the TIF_SINGLE_STEP bit in copy_thread. If the new process is not auto-attached by the tracer it is wrong to delivere SIGTRAP to the new process. The change is right, but this log entry is confusing. auto-attached has nothing to do with it, nor does anything about tracing the new process or not. The new process has not experienced a PER trap of its own, so it is wrong to deliver a SIGTRAP that is meant for its creator. Ok, I changed the wording slightly: Clear the TIF_SINGLE_STEP bit in copy_thread. The new process did not get a PER event of its own. It is wrong deliver a SIGTRAP that was meant for the parent process. -- blue skies, Martin. Reality continues to ruin my life. - Calvin.
Re: s390 user_enable_single_step() (Was: odd utrace testing results on s390x)
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)
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)
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)
On Mon, 4 Jan 2010 16:52:25 +0100 Oleg Nesterov o...@redhat.com wrote: Hi! We have some strange problems with utrace on s390, and so far this _looks_ like a s390 problem. Looks like, on any CPU user_enable_single_step() does not work until at least one thread with per_info.single_step = 1 does the context switch. This doesn't matter with the old ptrace implementation, but with utrace the tracee itself does user_enable_single_step(current) and returns to user-mode. Until it does at least one context switch the single-stepping doesn't work, after that everything works fine till the next reboot. The PER control registers only get reloaded on task switch. Can you test if this patch fixes your problem? -- 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); } void user_enable_single_step(struct task_struct *task) -- blue skies, Martin. Reality continues to ruin my life. - Calvin.
