web-site presence

2009-11-17 Thread Deville, Melanie 
Dear Utrace-devel

With your permission, we would like to show you how to get  
better positioning  and more traffic on the web.  If you are 
interested, reply us and we'll do a complementary no charge 
site assessment.


Sincerely,

Melanie Deville
Spark Media























utrace-devel@redhat.com
17/11/2009

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

kernel crash: solved (Was: copy_process utrace_init_task)

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

 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.

 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.

Damn!!!

It works, the kernel does NOT crash.

(by mistake, I copied bzImage to the wrong location, iow I tested
 the kernel without the patch above).

Oleg.

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: [PATCH] utrace: utrace_attach_task() forgets to return when -utrace == NULL

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

  But this smp_rmb() in task_utrace_struct() is only needed when the
  caller does something like
 
  if (task_utrace_flags(tsk))
  do_something(task_utrace_struct());

 If you look at where task_utrace_struct() is used, it's basically always
 like this.  All the tracehook.h callers into utrace.c do that.

 ...

 But that's not the only place, is it?  Every utrace_report_* and most every
 other utrace.c entry point is called from tracehook.h code using this pattern.
 Those can have the same issue.  So you'd have to make all of those do an:
   if (unlikely(!utrace))
   return;
 sort of check.  Is that what you mean?

Yes, agreed, this doesn't look good.

So, I think it is better to add the barriers into task_utrace_struct()
like you already did.

(a very minor nit: s/read_barrier_depends/smp_read_barrier_depends/)


Just to complete the discussion, we could do

static inline unsigned long task_utrace_flags(struct task_struct *task)
{
if (!task-utrace)
return 0;
return task-utrace_flags;
}

to void rmb() without complicationg the code, but this still adds
some overhead to the hot path.

And a bit offtopic question. Apart from task_utrace_flags() should
be as fast as possible, any other reason we can't move -utrace_flags
into struct utrace ?

In the likely case we should worry about (the task was never
traced), -utrace == NULL and

task_utrace_flags(struct task_struct *task)
{
if (likley(!task-utrace))
return 0;
return task-utrace-flags;
}

shouldn't be slower. If the task was ever traced and then utraced
this adds the extra dereference, yes.

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