syscall_regfunc() && TIF_SYSCALL_TRACEPOINT

PATCH Superseded Oleg Nesterov 2 participants 10 messages Mar 30, 2012 – Apr 20, 2012

Subsystems (4)

MM core Tracing Scheduler exec and binfmt

lore

Share & tools

lore

Oleg Nesterov

author

2012-03-30 18:38 UTC #0 lore raw

Hello.

I've looked at syscall_regfunc/unregfunc by accident, and I am a bit confused...

	void syscall_regfunc(void)
	{
		unsigned long flags;
		struct task_struct *g, *t;

		if (!sys_tracepoint_refcount) {
			read_lock_irqsave(&tasklist_lock, flags);

Why _irqsave? write_lock(tasklist) needs to disable irqs, but read_ doesn't. Any subtle reason I missed?

			do_each_thread(g, t) {
				/* Skip kernel threads. */
				if (t->mm)

We should check PF_KTHREAD, not ->mm.

					set_tsk_thread_flag(t, TIF_SYSCALL_TRACEPOINT);

But the main question is, can't we race with clone() and miss the new child? The new task is not "visible" to do_each_thread() until copy_process()->list_add_tail_rcu(thread_group/init_task.tasks).

Don't we need something like the patch below?

Oleg.

kernel/fork.c +5 -0

@@ -1446,7 +1446,12 @@ static struct task_struct *copy_process(
 
 	total_forks++;
 	spin_unlock(&current->sighand->siglock);
+#ifdef CONFIG_TRACEPOINTS
+	if (unlikely(test_thread_flag(TIF_SYSCALL_TRACEPOINT)))
+		set_tsk_thread_flag(p, TIF_SYSCALL_TRACEPOINT);
+#endif
 	write_unlock_irq(&tasklist_lock);
+
 	proc_fork_connector(p);
 	cgroup_post_fork(p);
 	if (clone_flags & CLONE_THREAD)

Steven Rostedt

maintainer

2012-03-30 19:02 UTC #1 lore raw

Quoted 13 lines · Oleg Nesterov

Hello.
I've looked at syscall_regfunc/unregfunc by accident, and I am a bit confused...
	void syscall_regfunc(void)
	{
		unsigned long flags;
		struct task_struct *g, *t;

		if (!sys_tracepoint_refcount) {
			read_lock_irqsave(&tasklist_lock, flags);

Quoted 2 lines

Why _irqsave? write_lock(tasklist) needs to disable irqs, but read_ doesn't. Any subtle reason I missed?

As long as an interrupt doesn't take the tasklist lock as write, we don't. If it doesn't then we should be safe not to disable interrupts here.

Quoted 6 lines

			do_each_thread(g, t) {
				/* Skip kernel threads. */
				if (t->mm)
We should check PF_KTHREAD, not ->mm.

A lot of places test ->mm for kernel threads. Just search for ->mm in kernel/sched/core.c

Quoted 15 lines

					set_tsk_thread_flag(t, TIF_SYSCALL_TRACEPOINT);
But the main question is, can't we race with clone() and miss the new child? The new task is not "visible" to do_each_thread() until copy_process()->list_add_tail_rcu(thread_group/init_task.tasks).
Don't we need something like the patch below?
Oleg.

kernel/fork.c static struct task_struct *copy_process( · line 1451

Quoted 6 lines

kernel/fork.c +3 -0

@@ -1446,7 +1446,12 @@ static struct task_struct *copy_process(
 
 	total_forks++;
 	spin_unlock(&current->sighand->siglock);
+#ifdef CONFIG_TRACEPOINTS
+	if (unlikely(test_thread_flag(TIF_SYSCALL_TRACEPOINT)))
+		set_tsk_thread_flag(p, TIF_SYSCALL_TRACEPOINT);

I'm not so worried about the set (although that should be done) but it is entirely possible that we need a clear too. Leaving this flag set would cause a task to take the overhead of tracing syscalls without ever tracing them.

-- Steve

Quoted 6 lines

kernel/fork.c +2 -0

+#endif
 	write_unlock_irq(&tasklist_lock);
+
 	proc_fork_connector(p);
 	cgroup_post_fork(p);
 	if (clone_flags & CLONE_THREAD)

Oleg Nesterov

author

2012-03-30 20:23 UTC #2 lore raw

Quoted 12 lines · Steven Rostedt

Quoted 10 lines · Oleg Nesterov
Hello.
I've looked at syscall_regfunc/unregfunc by accident, and I am a bit confused...
	void syscall_regfunc(void)
	{
		unsigned long flags;
		struct task_struct *g, *t;

Quoted 7 lines

Quoted 5 lines
		if (!sys_tracepoint_refcount) {
			read_lock_irqsave(&tasklist_lock, flags);
Why _irqsave? write_lock(tasklist) needs to disable irqs, but read_ doesn't. Any subtle reason I missed?
As long as an interrupt doesn't take the tasklist lock as write,

No, this is forbidden.

Quoted 7 lines

Quoted 5 lines
			do_each_thread(g, t) {
				/* Skip kernel threads. */
				if (t->mm)
We should check PF_KTHREAD, not ->mm.
A lot of places test ->mm for kernel threads.

And this is wrong, use_mm() can set ->mm != NULL. This is the common mistake.

Quoted 2 lines

Just search for ->mm in
kernel/sched/core.c

Probably normalize_rt_tasks() and __sched_setscheduler() should be fixed.

Quoted 9 lines

Quoted 9 lines
Don't we need something like the patch below?
Oleg.
kernel/fork.c +0 -0
@@ -1446,7 +1446,12 @@ static struct task_struct *copy_process(
 

Quoted 10 lines

kernel/fork.c static struct task_struct *copy_process( · line 1451
Quoted 5 lines
kernel/fork.c +3 -0
 	total_forks++;
 	spin_unlock(&current->sighand->siglock);
+#ifdef CONFIG_TRACEPOINTS
+	if (unlikely(test_thread_flag(TIF_SYSCALL_TRACEPOINT)))
+		set_tsk_thread_flag(p, TIF_SYSCALL_TRACEPOINT);
I'm not so worried about the set (although that should be done) but it is entirely possible that we need a clear too. Leaving this flag set would cause a task to take the overhead of tracing syscalls without ever tracing them.

Agreed. OK, I'll send the patch with "else clear".

But I don't really understand why do you think that "clear" is more important. Sure, the wrong TIF_SYSCALL_TRACEPOINT triggers the slow path unnecessary, but this is more or less harmless. But if we do not set the task obviously won't report trace_sys*, this looks like a bug even if nothing bad can happen.

Oleg.

Steven Rostedt

maintainer

2012-03-31 00:13 UTC #3 lore raw

Quoted 9 lines · Oleg Nesterov

Quoted 1 line
A lot of places test ->mm for kernel threads.
And this is wrong, use_mm() can set ->mm != NULL. This is the common mistake.
Quoted 2 lines
Just search for ->mm in
kernel/sched/core.c
Probably normalize_rt_tasks() and __sched_setscheduler() should be fixed.

Heh, the __sched_setscheduler() usage is from me. But I'm not sure we had an alternative back in 2005.

Quoted 2 lines

But I don't really understand why do you think that "clear" is more important.

They are both important. But as I tend to consider performance when tracing is off as critical, I'm more concerned about that. But both must be fixed, because not reporting traces can confuse a developer.

Quoted 4 lines

Sure, the wrong TIF_SYSCALL_TRACEPOINT triggers the slow
path unnecessary, but this is more or less harmless. But if we do not set the task obviously won't report trace_sys*, this looks like a bug even if nothing bad can happen.

Agreed, both are a bug and both should be fixed.

Thanks,

-- Steve

Oleg Nesterov

author

2012-03-31 20:45 UTC #4 lore raw

Quoted 9 lines · Steven Rostedt

Quoted 2 lines · Oleg Nesterov
But I don't really understand why do you think that "clear" is more important.
They are both important. But as I tend to consider performance when tracing is off as critical, I'm more concerned about that. But both must be fixed, because not reporting traces can confuse a developer.

Ah, got it, thanks.

I was going to send the simple patch we discussed, but suddenly I realized that I have another question.

Why do we want to filter out the kernel threads in syscall_regfunc?

Quoted 1 line

From cc3b13c1 "tracing: Don't trace kernel thread syscalls"

then it has no effect to trace the kernel thread calls to syscalls in that path.
Setting the TIF_SYSCALL_TRACEPOINT flag is then useless for these.

OK, but then it doesn't hurt? Or is there another reason why TIF_SYSCALL_TRACEPOINT is not desirable on kthread?

The problem is ____call_usermodehelper() which execs the user-space task. This clears PF_KTHREAD (sets ->mm), but obviously if sys_tracepoint_refcount != 0 this is too late.

So what do you think we should do,

- keep this check

- remove it

- remove it in a separate patch

- add the "sync with sys_tracepoint_refcount" hook before kernel_execve()

Oleg.

Steven Rostedt

maintainer

2012-03-31 21:37 UTC #5 lore raw

Quoted 18 lines · Oleg Nesterov

Quoted 9 lines · Steven Rostedt
Quoted 2 lines · Oleg Nesterov
But I don't really understand why do you think that "clear" is more important.
They are both important. But as I tend to consider performance when tracing is off as critical, I'm more concerned about that. But both must be fixed, because not reporting traces can confuse a developer.
Ah, got it, thanks.
I was going to send the simple patch we discussed, but suddenly I realized that I have another question.
Why do we want to filter out the kernel threads in syscall_regfunc?

Quoted 8 lines

Quoted 1 line
From cc3b13c1 "tracing: Don't trace kernel thread syscalls"
then it has no effect to trace the kernel thread calls to syscalls in that path.
Setting the TIF_SYSCALL_TRACEPOINT flag is then useless for these.
OK, but then it doesn't hurt? Or is there another reason why TIF_SYSCALL_TRACEPOINT is not desirable on kthread?

Right, it doesn't hurt. I was about to say that in a previous email.

Quoted 5 lines

The problem is ____call_usermodehelper() which execs the user-space task. This clears PF_KTHREAD (sets ->mm), but obviously if sys_tracepoint_refcount != 0 this is too late.

Quoted 7 lines

So what do you think we should do,
- keep this check
- remove it
- remove it in a separate patch

I say this one (remove it in a separate patch). That way if something breaks we know exactly what did it ;-)

Quoted 5 lines

- add the "sync with sys_tracepoint_refcount" hook before kernel_execve()
?

-- Steve

Oleg Nesterov

author

2012-04-01 21:38 UTC #6 lore raw

cover (Was: syscall_regfunc() && TIF_SYSCALL_TRACEPOINT)

Quoted 5 lines · Steven Rostedt

Quoted 3 lines · Oleg Nesterov
So what do you think we should do,

Quoted 8 lines

Quoted 5 lines
- keep this check
- remove it
- remove it in a separate patch
I say this one (remove it in a separate patch). That way if something breaks we know exactly what did it ;-)

OK, agreed.

Don't really know how can I test this... but the kernel didn't crash after I enabled the syscall tracer ;)

Oleg.

Oleg Nesterov

author

2012-04-01 21:38 UTC #7 lore raw

PATCH 1/2 tracing: syscall_*regfunc() can race with copy_process()

syscall_regfunc() and syscall_unregfunc() should set/clear TIF_SYSCALL_TRACEPOINT system-wide, but do_each_thread() can race with copy_process() and miss the new child which was not added to init_task.tasks list yet.

Change copy_process() to update the child's TIF_SYSCALL_TRACEPOINT under tasklist.

While at it,

- remove _irqsafe from syscall_regfunc/syscall_unregfunc, read_lock(tasklist) doesn't need to disable irqs.

- change syscall_unregfunc() to check PF_KTHREAD to skip the kernel threads, ->mm != NULL is the common mistake.

Note: probably this check should be simply removed, needs another patch.

Signed-off-by Oleg Nesterov <[email protected]>

---
 kernel/fork.c       |    7 +++++++
 kernel/tracepoint.c |   12 +++++-------
 2 files changed, 12 insertions(+), 7 deletions(-)

kernel/fork.c +7 -0

index 34d7ed1..772d4a5 100644
@@ -1446,7 +1446,14 @@ static struct task_struct *copy_process(unsigned long clone_flags,
 
 	total_forks++;
 	spin_unlock(&current->sighand->siglock);
+#ifdef CONFIG_TRACEPOINTS
+	if (test_thread_flag(TIF_SYSCALL_TRACEPOINT))
+		set_tsk_thread_flag(p, TIF_SYSCALL_TRACEPOINT);
+	else
+		clear_tsk_thread_flag(p, TIF_SYSCALL_TRACEPOINT);
+#endif
 	write_unlock_irq(&tasklist_lock);
+
 	proc_fork_connector(p);
 	cgroup_post_fork(p);
 	if (clone_flags & CLONE_THREAD)

kernel/tracepoint.c +5 -7

index f1539de..e2a4523 100644
@@ -734,33 +734,31 @@ static int sys_tracepoint_refcount;
 
 void syscall_regfunc(void)
 {
-	unsigned long flags;
 	struct task_struct *g, *t;
 
 	if (!sys_tracepoint_refcount) {
-		read_lock_irqsave(&tasklist_lock, flags);
+		read_lock(&tasklist_lock);
 		do_each_thread(g, t) {
 			/* Skip kernel threads. */
-			if (t->mm)
+			if (!(t->flags & PF_KTHREAD))
 				set_tsk_thread_flag(t, TIF_SYSCALL_TRACEPOINT);
 		} while_each_thread(g, t);
-		read_unlock_irqrestore(&tasklist_lock, flags);
+		read_unlock(&tasklist_lock);
 	}
 	sys_tracepoint_refcount++;
 }
 
 void syscall_unregfunc(void)
 {
-	unsigned long flags;
 	struct task_struct *g, *t;
 
 	sys_tracepoint_refcount--;
 	if (!sys_tracepoint_refcount) {
-		read_lock_irqsave(&tasklist_lock, flags);
+		read_lock(&tasklist_lock);
 		do_each_thread(g, t) {
 			clear_tsk_thread_flag(t, TIF_SYSCALL_TRACEPOINT);
 		} while_each_thread(g, t);
-		read_unlock_irqrestore(&tasklist_lock, flags);
+		read_unlock(&tasklist_lock);
 	}
 }
 #endif

Signature · 1 line

1.5.5.1

Oleg Nesterov

author

2012-04-01 21:38 UTC #8 lore raw

PATCH 2/2 tracing: syscall_regfunc() should not skip kernel threads

In reply to Oleg Nesterov

syscall_regfunc() ignores the kernel thread because "it has no effect", see cc3b13c1 "Don't trace kernel thread syscalls".

However, this means that a user-space task spawned by call_usermodehelper() won't report the system calls if kernel_execve() is called when sys_tracepoint_refcount != 0.

Remove this check. Hopefully the unnecessary report from ret_from_fork path mentioned by cc3b13c1 is fine. In fact "this is the only case" is not true. Say, kernel_execve() itself does "int 80" on X86_32. Hopefully fine too.

Signed-off-by Oleg Nesterov <[email protected]>

---
 kernel/tracepoint.c |    4 +---
 1 files changed, 1 insertions(+), 3 deletions(-)

kernel/tracepoint.c +1 -3

index e2a4523..2403e60 100644
@@ -739,9 +739,7 @@ void syscall_regfunc(void)
 	if (!sys_tracepoint_refcount) {
 		read_lock(&tasklist_lock);
 		do_each_thread(g, t) {
-			/* Skip kernel threads. */
-			if (!(t->flags & PF_KTHREAD))
-				set_tsk_thread_flag(t, TIF_SYSCALL_TRACEPOINT);
+			set_tsk_thread_flag(t, TIF_SYSCALL_TRACEPOINT);
 		} while_each_thread(g, t);
 		read_unlock(&tasklist_lock);
 	}

Signature · 1 line

1.5.5.1

Oleg Nesterov

author

2012-04-20 21:27 UTC #9 lore raw

Re: [PATCH 0/2] (Was: syscall_regfunc() && TIF_SYSCALL_TRACEPOINT)

In reply to Oleg Nesterov

Should I resend this or we do not really care?

The problem is minor, but both patches look like the simple and obvious bugfix to me.

Quoted 22 lines · Oleg Nesterov

Quoted 5 lines · Steven Rostedt
Quoted 3 lines · Oleg Nesterov
So what do you think we should do,
Quoted 8 lines
Quoted 5 lines
- keep this check
- remove it
- remove it in a separate patch
I say this one (remove it in a separate patch). That way if something breaks we know exactly what did it ;-)
OK, agreed.
Don't really know how can I test this... but the kernel didn't crash after I enabled the syscall tracer ;)
Oleg.