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

2010-01-04 Thread Martin Schwidefsky
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.



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

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

 IOW. I think this problem is minor and probably can be ignored,

Or may be not...

Even if the child is not killed by SIGTRAP, it can get a lot of
unnecessary traps.

To verify, I did another trivial patch (below), and the test
case from 6580807da14c423f0d0a708108e6df6ebc8bc83d does trigger
a lot of false step printks.

Hmm. And sometimes there is nothing in dmesg, but the test-case
needs a lot of time to complete. taskset -c seems to always
trigger printk's. Magic.

Oleg.

--- arch/s390/kernel/traps.c~   2009-12-22 10:41:52.909174198 -0500
+++ arch/s390/kernel/traps.c2010-01-04 13:19:51.038187586 -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(false step\n);
 }
 
 static void default_trap_handler(struct pt_regs * regs, long interruption_code)



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

2010-01-04 Thread Roland McGrath
 The PER control registers only get reloaded on task switch. Can you test
 if this patch fixes your problem?

Long ago when I first worked with David Wilder on s390 arch code,
I remember we made this change.  It seems to have been forgotten
in the later rounds of reworking and merging.


Thanks,
Roland



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

2010-01-04 Thread Roland McGrath
 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.

If I'm right, then this task wants single-stepping is not the problem,
and that really is fully cleared.  In fact, looking at s390's copy_thread
(arch/s390/kernel/process.c) it clears out all the state that is actually
touched by user_enable_single_step and user_disable_single_step.  So for
s390 the new fork.c call is actually superfluous AFAICT.

The problem is that the copied parent state includes the this task has a
pending single-step to report flag.  IMHO it clearly makes sense for
s390's copy_thread to clear this flag in a new task, which it does not do now.

An alternative to that would be just to make its user_disable_single_step
clear the flag.  That could in theory also have an effect on e.g. the
(authentic) pending step report of a tracee that was stopped with
TIF_SINGLE_STEP set when its tracer detached.  This might be considered a
good thing, but since every other arch posts the SIGTRAP immediately they
all have the equivalent issue and s390 doesn't need to be any better than
they are before we have a generic resolution to the whole subject of
tracer-induced signals (which we won't get into now).  I'm not even sure
from my insufficient reading of the s390 assembly code whether this path is
even possible, i.e. do_signal called before do_single_step.

Martin, I suggest having copy_thread clear TIF_SINGLE_STEP.
That bit is always task-private state that should not be copied.

Btw, given the complexity of FixPerRegisters (and its new additional cost
on task==current), you might want to make user_*_single_step bail out if
per_info.single_step is already set/clear on entry.


Thanks,
Roland