system call notification with self_ptrace
Subsystems (6)
PATCH 1/1 system call notification with self_ptrace
Subject: [PATCH] system call notification with self_ptrace
From: Pierre Morel <[email protected]>
PTRACE SELF
This patch adds a new functionality to ptrace: system call notification to the current process.
When a process requests self ptrace, with the new request PTRACE_SELF_ON:
1. the next system call performed by the process will not be executed 2. self ptrace will be disabled for the process 3. a SIGSYS signal will be sent to the process.
With an appropriate SIGSYS signal handler, the process can access its own data structures to
1. get the system call number from the siginfo structure 2. get the system call arguments from the stack 3. instrument the system call with other system calls 4. emulate the system call with other system calls 5. change the arguments of the system call 6. perform the system call for good 7. change the return value of the system call 8. request self ptrace again before returning.
The new request PTRACE_SELF_OFF disables self ptrace.
---
arch/s390/kernel/ptrace.c | 16 ++++++++++++++++ arch/s390/kernel/signal.c | 5 +++++ arch/x86/kernel/ptrace.c | 29 +++++++++++++++++++++++++++++ arch/x86/kernel/signal_32.c | 5 +++++ arch/x86/kernel/signal_64.c | 5 +++++ include/asm-generic/siginfo.h | 6 ++++++ include/linux/ptrace.h | 18 ++++++++++++++++++ include/linux/sched.h | 1 + kernel/ptrace.c | 32 ++++++++++++++++++++++++++++++++ 9 files changed, 117 insertions(+)
arch/s390/kernel/ptrace.c +0 -0
@@ -583,6 +583,22 @@ syscall_trace(struct pt_regs *regs, int
if (!test_thread_flag(TIF_SYSCALL_TRACE)) goto out;
+
+ if (is_self_ptracing(regs->gprs[2])) {
+ if (!entryexit) {
+ struct siginfo info;
+
+ memset(&info, 0, sizeof(struct siginfo));
+ info.si_signo = SIGSYS;
+ info.si_code = SYS_SYSCALL;
+ info.si_errno = regs->gprs[2];
+ info.si_addr = (void *)regs->orig_gpr2;
+ send_sig_info(SIGSYS, &info, current);
+ regs->gprs[2] = -1;
+ }
+ return;
+ }
+
if (!(current->ptrace & PT_PTRACED)) goto out; ptrace_notify(SIGTRAP | ((current->ptrace & PT_TRACESYSGOOD)
arch/s390/kernel/signal.c +0 -0
@@ -409,6 +409,11 @@ handle_signal(unsigned long sig, struct
spin_unlock_irq(¤t->sighand->siglock); }
+ if (current->instrumentation) {
+ clear_thread_flag(TIF_SYSCALL_TRACE);
+ current->instrumentation &= ~PTS_SELF;
+ }
+
return ret; }
arch/x86/kernel/ptrace.c +0 -0
@@ -1394,6 +1394,19 @@ int do_syscall_trace(struct pt_regs *reg
if (!entryexit) secure_computing(regs->orig_ax);
+ if (is_self_ptracing(regs->orig_ax)) {
+ if (!entryexit) {
+ struct siginfo info;
+
+ memset(&info, 0, sizeof(struct siginfo));
+ info.si_signo = SIGSYS;
+ info.si_code = SYS_SYSCALL;
+ info.si_addr = (void *) regs->orig_ax;
+ send_sig_info(SIGSYS, &info, current);
+ }
+ return 1; /* Skip system call, deliver signal. */
+ }
+
if (unlikely(current->audit_context)) {
if (entryexit)
audit_syscall_exit(AUDITSC_RESULT(regs->ax),
@@ -1486,6 +1499,18 @@ asmlinkage void syscall_trace_enter(stru
/* do the secure computing check first */
secure_computing(regs->orig_ax);+ if (is_self_ptracing(regs->orig_ax)) {
+ struct siginfo info;
+
+ memset(&info, 0, sizeof(struct siginfo));
+ info.si_signo = SIGSYS;
+ info.si_code = SYS_SYSCALL;
+ info.si_addr = (void *) regs->orig_ax;
+ send_sig_info(SIGSYS, &info, current);
+ regs->ax = -1 ;
+ return; /* Skip system call, deliver signal. */
+ }
+
if (test_thread_flag(TIF_SYSCALL_TRACE) && (current->ptrace & PT_PTRACED)) syscall_trace(regs);
@@ -1507,6 +1532,10 @@ asmlinkage void syscall_trace_enter(stru
asmlinkage void syscall_trace_leave(struct pt_regs *regs) {
+ if (is_self_ptracing(regs->orig_ax)) {
+ regs->ax = -1 ;
+ return; /* Skip system call. */
+ }
if (unlikely(current->audit_context)) audit_syscall_exit(AUDITSC_RESULT(regs->ax), regs->ax);
arch/x86/kernel/signal_32.c +0 -0
@@ -568,6 +568,11 @@ handle_signal(unsigned long sig, siginfo
recalc_sigpending(); spin_unlock_irq(¤t->sighand->siglock);
+ if (current->instrumentation & PTS_SELF) {
+ clear_thread_flag(TIF_SYSCALL_TRACE);
+ current->instrumentation &= ~PTS_SELF;
+ }
+
return 0; }
arch/x86/kernel/signal_64.c +0 -0
@@ -402,6 +402,11 @@ handle_signal(unsigned long sig, siginfo
spin_unlock_irq(¤t->sighand->siglock); }
+ if (current->instrumentation & PTS_SELF) {
+ clear_thread_flag(TIF_SYSCALL_TRACE);
+ current->instrumentation &= ~PTS_SELF;
+ }
+
return ret; }
include/asm-generic/siginfo.h +0 -0
@@ -224,6 +224,12 @@ typedef struct siginfo {
#define NSIGPOLL 6
/*
+ * SIGSYS si_codes
+ */
+#define SYS_SYSCALL (__SI_FAULT|1) /* system call notification */
+#define NSIGSYS 1
+
+/*
* sigevent definitions
*
* It seems likely that SIGEV_THREAD will have to be handled from
include/linux/ptrace.h +0 -0
@@ -27,6 +27,10 @@
#define PTRACE_GETSIGINFO 0x4202
#define PTRACE_SETSIGINFO 0x4203
+/* PTRACE SELF requests */
+#define PTRACE_SELF_ON 0x4281
+#define PTRACE_SELF_OFF 0x4282
+
/* options set using PTRACE_SETOPTIONS */
#define PTRACE_O_TRACESYSGOOD 0x00000001
#define PTRACE_O_TRACEFORK 0x00000002
@@ -78,7 +82,21 @@
#include <linux/compiler.h> /* For unlikely. */
#include <linux/sched.h> /* For struct task_struct. */
+#include <linux/unistd.h> /* For syscall definitions */
+
+#define PTS_INSTRUMENTED 0x00000001
+#define PTS_SELF 0x00000002
+static inline int is_self_ptracing(unsigned long syscall) +{
+ if (!(current->instrumentation & PTS_SELF))
+ return 0;
+ if (syscall == __NR_rt_sigreturn)
+ return 0;
+ if (syscall == __NR_ptrace)
+ return 0;
+ return 1;
+}
extern long arch_ptrace(struct task_struct *child, long request, long addr, long data); extern struct task_struct *ptrace_get_task_struct(pid_t pid);
kernel/ptrace.c +0 -0
@@ -543,6 +543,38 @@ asmlinkage long sys_ptrace(long request,
* This lock_kernel fixes a subtle race with suid exec */ lock_kernel();
+ if (request == PTRACE_SELF_ON) {
+ task_lock(current);
+ if (current->ptrace) {
+ task_unlock(current);
+ ret = -EPERM;
+ goto out;
+ }
+ set_thread_flag(TIF_SYSCALL_TRACE);
+ current->instrumentation |= PTS_INSTRUMENTED|PTS_SELF;
+ task_unlock(current);
+ ret = 0;
+ goto out;
+ }
+ if (request == PTRACE_SELF_OFF) {
+ task_lock(current);
+ if (current->ptrace) {
+ task_unlock(current);
+ ret = -EPERM;
+ goto out;
+ }
+ clear_thread_flag(TIF_SYSCALL_TRACE);
+ current->instrumentation &= ~PTS_SELF;
+ task_unlock(current);
+ ret = 0;
+ goto out;
+ }
+
+ if (current->instrumentation) {
+ ret = -EPERM;
+ goto out;
+ }
+
if (request == PTRACE_TRACEME) {
ret = ptrace_traceme();
if (!ret)include/linux/sched.h +0 -0
@@ -1303,6 +1303,7 @@ struct task_struct {
int latency_record_count; struct latency_record latency_record[LT_SAVECOUNT];
#endif
+ u64 instrumentation;
};
/*
Quoted 27 lines · On Mon, 08 Sep 2008 14:02:01 +0200 Pierre Morel <[email protected]>…
Subject: [PATCH] system call notification with
self_ptraceFrom: Pierre Morel <[email protected]>
PTRACE SELF
This patch adds a new functionality to ptrace: system call notification to the current process.
When a process requests self ptrace, with the new requestPTRACE_SELF_ON:1. the next system call performed by the process will not be executed 2. self ptrace will be disabled for the process 3. a SIGSYS signal will be sent to the process.With an appropriate SIGSYS signal handler, the process can access its own data structures to
1. get the system call number from the siginfo structure 2. get the system call arguments from the stack 3. instrument the system call with other system calls 4. emulate the system call with other system calls 5. change the arguments of the system call 6. perform the system call for good 7. change the return value of the system call 8. request self ptrace again before returning.
Quoted 2 lines
The new request
PTRACE_SELF_OFFdisables self ptrace.
It sounds like it might be useful.
Are there any userspace tools available with which people can utilise this new functionality? Or plans to release them?
Quoted 5 lines
arch/s390/kernel/ptrace.c | 16 ++++++++++++++++ arch/s390/kernel/signal.c | 5 +++++ arch/x86/kernel/ptrace.c | 29 +++++++++++++++++++++++++++++ arch/x86/kernel/signal_32.c | 5 +++++ arch/x86/kernel/signal_64.c | 5 +++++
Maintainers of the other 30-odd architectures would appreciate a test application which they can use to develop and test their ports, please.
Michael Kerrisk will no doubt be looking for manpage assistance. Please cc him on this material.
It would be good to get suitable testcases integrated into LTP (if LTP has ptrace tests).
The patch title uses the term "self_ptrace", but the patch itself uses the term "ptrace_self". Let's get it consistent everywhere.
The patch adds a
+ u64 instrumentation;
to the task_struct but no explanation is provided as to why this was added, why it is a 64-bit field, what its locking rules are, etc. Please fix this.
-------------------------------------------------------------------------
This SF.Net email is sponsored by the Moblin Your Move Developer's challenge Build the coolest Linux based applications with Moblin SDK & win great prizes Grand prize is a trip for two to an Open Source event anywhere in the world
http://moblin-contest.org/redirect.php?banner_id=100&url=/
_______________________________________________
User-mode-linux-devel mailing list [email protected]
https://lists.sourceforge.net/lists/listinfo/user-mode-linux-devel
Hi,
Quoted 31 lines · Andrew Morton
Quoted 27 lines · On Mon, 08 Sep 2008 14:02:01 +0200 Pierre Morel <[email protected]>…
Subject: [PATCH] system call notification with
self_ptraceFrom: Pierre Morel <[email protected]>
PTRACE SELF
This patch adds a new functionality to ptrace: system call notification to the current process.
When a process requests self ptrace, with the new requestPTRACE_SELF_ON:1. the next system call performed by the process will not be executed 2. self ptrace will be disabled for the process 3. a SIGSYS signal will be sent to the process.With an appropriate SIGSYS signal handler, the process can access its own data structures to
1. get the system call number from the siginfo structure 2. get the system call arguments from the stack 3. instrument the system call with other system calls 4. emulate the system call with other system calls 5. change the arguments of the system call 6. perform the system call for good 7. change the return value of the system call 8. request self ptrace again before returning.
Quoted 6 lines
Quoted 3 lines
The new request
PTRACE_SELF_OFFdisables self ptrace.It sounds like it might be useful.
Thanks, yes I am sure it might.
Quoted 3 lines
Are there any userspace tools available with which people can utilise this new functionality? Or plans to release them?
Yes, we plan to release a tool to trace an application soon.
Quoted 11 lines
Quoted 7 lines
arch/s390/kernel/ptrace.c | 16 ++++++++++++++++ arch/s390/kernel/signal.c | 5 +++++ arch/x86/kernel/ptrace.c | 29 +++++++++++++++++++++++++++++ arch/x86/kernel/signal_32.c | 5 +++++ arch/x86/kernel/signal_64.c | 5 +++++Maintainers of the other 30-odd architectures would appreciate a test application which they can use to develop and test their ports, please.
Yes, of course I have one for x86 and one for s390. I am cleaning them to make them available.
Quoted 2 lines
Michael Kerrisk will no doubt be looking for manpage assistance. Please cc him on this material.
OK, I will prepare this.
Quoted 3 lines
It would be good to get suitable testcases integrated into LTP (if LTP has ptrace tests).
Yes, I will prepare this too.
Quoted 3 lines
The patch title uses the term "
self_ptrace", but the patch itself uses the term "ptrace_self". Let's get it consistent everywhere.
Right. It should be self_ptrace.
Quoted 7 lines
The patch adds a
(quoted diff) +1
-0+ u64 instrumentation;to the
task_structbut no explanation is provided as to why this was added, why it is a 64-bit field, what its locking rules are, etc. Please fix this.
I used to steal one bit in the ptrace bit-field of the task_struct but Oleg pointed out that the ptrace bit-field is used in a lot of places without any bit mask, so I chose another way to remember that I (the thread) am instrumenting myself.
Alternatively, I could also use the ptrace bit-field and modify every reference to use a mask for any test, set or reset of the bit-field.
I provision a 64 bit wide bit-field for future extensions of the instrumentation. I could of course use a smaller bit-field as only 1 bit is really useful for now. I used 64 bit to be memory aligned with most of the architectures.
There is no lock for the instrumentation bit-field because it is used for self tracing only, and only current ever accesses the flag.
Signature · 3 lines
============= Pierre Morel RTOS and Embedded Linux
Quoted 4 lines · Pierre Morel
arch/s390/kernel/signal.c handle_signal(unsigned long sig, struct · line 418
Quoted 10 lines
arch/s390/kernel/signal.c +5
-0@@ -409,6 +409,11 @@ handle_signal(unsigned long sig, structspin_unlock_irq (¤t->sighand->siglock); } +if (current->instrumentation) { +clear_thread_flag (TIF_SYSCALL_TRACE ); + current->instrumentation &= ~PTS_SELF ; + } +return ret; }
I still think this patch shouldn't change handle_signal().
Once again. The signal handler for SIGSYS can first do sys_ptrace(PTRACE_SELF_OFF) (which is filtered out), and then use any other syscall, so this change is not needed, afaics.
The overhead of the additional PTRACE_SELF_OFF syscall is very small, especially compared to signal delivery. I don't think this functionality will be widely used, but this change adds the unconditional overhead to handle_signal().
Btw, the check above looks wrong, shouldn't it be
if (current->instrumentation & PTS_SELF)
?
And. According to the prior discussion, this requires to hook every signal handler in user space, otherwise we can miss syscall. But every hook should start with PTRACE_SELF_ON, so I can't see any gain.
arch/s390/kernel/signal.c handle_signal(unsigned long sig, struct
Quoted 2 lines
arch/s390/kernel/signal.c +2
-0+#define PTS_INSTRUMENTED 0x00000001 +#define PTS_SELF 0x00000002
I don't really understand why do we need 2 flags, see also below,
Quoted 8 lines
kernel/ptrace.c +2
-0@@ -543,6 +543,38 @@ asmlinkage long sys_ptrace(long request,* This lock_kernel fixes a subtle race with suid exec */ lock_kernel (); +if (request ==PTRACE_SELF_ON ) { +task_lock (current);
kernel/ptrace.c asmlinkage long sys_ptrace(long request, · line 557
Quoted 10 lines
kernel/ptrace.c +10
-0+if (current->ptrace) { +task_unlock (current); + ret = -EPERM ; + goto out; + } +set_thread_flag (TIF_SYSCALL_TRACE ); + current->instrumentation |=PTS_INSTRUMENTED |PTS_SELF ; +task_unlock (current); + ret =0 ; + goto out;
The code looks strange. How about
if (request == PTRACE_SELF_ON) {
ret = -EPERM;
task_lock(current);
if (!current->ptrace) {
set_thread_flag(TIF_SYSCALL_TRACE);
current->instrumentation |= PTS_INSTRUMENTED|PTS_SELF;
ret = 0;
}
task_unlock(current);
goto out;
}?
I don't understand how task_lock() can help. This code runs under lock_kernel(), and without this lock the code is racy anyway.
kernel/ptrace.c asmlinkage long sys_ptrace(long request, · line 567
Quoted 10 lines
kernel/ptrace.c +10
-0+ } +if (request ==PTRACE_SELF_OFF ) { +task_lock (current); +if (current->ptrace) { +task_unlock (current); + ret = -EPERM ; + goto out; + } +clear_thread_flag (TIF_SYSCALL_TRACE ); + current->instrumentation &= ~PTS_SELF ;
So. PTRACE_SELF_OFF doesn't clear PTS_INSTRUMENTED? How can the task reset ->instrumentation ?
kernel/ptrace.c asmlinkage long sys_ptrace(long request, · line 571
Quoted 4 lines
kernel/ptrace.c +4
-0+if (current->instrumentation) { + ret = -EPERM ; + goto out; + }
So, PTRACE_SELF_XXX disables the "normal" ptrace. Not sure this is good.
Oleg.
Hello,
Quoted 19 lines · Oleg Nesterov
Quoted 4 lines · Pierre Morel
arch/s390/kernel/signal.c +0
-0@@ -409,6 +409,11 @@ handle_signal(unsigned long sig, structspin_unlock_irq (¤t->sighand->siglock);arch/s390/kernel/signal.c handle_signal(unsigned long sig, struct · line 419
Quoted 10 lines
arch/s390/kernel/signal.c +5
-0} +if (current->instrumentation) { +clear_thread_flag (TIF_SYSCALL_TRACE ); + current->instrumentation &= ~PTS_SELF ; + } +return ret; }I still think this patch shouldn't change
handle_signal().
Quoted 4 lines
Once again. The signal handler for SIGSYS can first do
sys_ptrace(PTRACE_SELF_OFF)(which is filtered out), and then use any other syscall, so this change is not needed, afaics.
Yes it can but what if the application forget to do it? It is a security so that the application do not bounce for ever.
Quoted 11 lines
The overhead of the additional
PTRACE_SELF_OFFsyscall is very small, especially compared to signal delivery. I don't think this functionality will be widely used, but this change adds the unconditional overhead tohandle_signal().Btw, the check above looks wrong, shouldn't it be
if (current->instrumentation & PTS_SELF)?
Yes you are right, in fact I do not need two flags, I will remove the PTS_INSTRUMENTED flag.
Quoted 11 lines
And. According to the prior discussion, this requires to hook every signal handler in user space, otherwise we can miss syscall. But every hook should start with
PTRACE_SELF_ON, so I can't see any gain.arch/s390/kernel/signal.c
Quoted 3 lines
arch/s390/kernel/signal.c +2
-0+#define PTS_INSTRUMENTED 0x00000001 +#define PTS_SELF 0x00000002I don't really understand why do we need 2 flags, see also below,
Yes, I remove PTS_INSTRUMENTED, a bad idea.
Quoted 35 lines
Quoted 10 lines
kernel/ptrace.c +3
-0@@ -543,6 +543,38 @@ asmlinkage long sys_ptrace(long request,* This lock_kernel fixes a subtle race with suid exec */ lock_kernel (); +if (request ==PTRACE_SELF_ON ) { +task_lock (current); +if (current->ptrace) {kernel/ptrace.c asmlinkage long sys_ptrace(long request, · line 558
Quoted 10 lines
kernel/ptrace.c +9
-0+task_unlock (current); + ret = -EPERM ; + goto out; + } +set_thread_flag (TIF_SYSCALL_TRACE ); + current->instrumentation |=PTS_INSTRUMENTED |PTS_SELF ; +task_unlock (current); + ret =0 ; + goto out;The code looks strange. How about
if (request == PTRACE_SELF_ON) { ret = -EPERM; task_lock(current); if (!current->ptrace) { set_thread_flag(TIF_SYSCALL_TRACE); current->instrumentation |= PTS_INSTRUMENTED|PTS_SELF; ret = 0; } task_unlock(current); goto out; }
Quoted 5 lines
?
I don't understand how
task_lock()can help. This code runs underlock_kernel(), and without this lock the code is racy anyway.
I use task_lock to protect the current->ptrace bit-field which can be accessed by another thread, like the one you pointed out previously. I agree it is not necessary with lock_kernel().
I will put the code before the lock_kernel() to be more efficient.
Quoted 16 lines
kernel/ptrace.c asmlinkage long sys_ptrace(long request, · line 569
Quoted 12 lines
kernel/ptrace.c +10
-0+ } +if (request ==PTRACE_SELF_OFF ) { +task_lock (current); +if (current->ptrace) { +task_unlock (current); + ret = -EPERM ; + goto out; + } +clear_thread_flag (TIF_SYSCALL_TRACE ); + current->instrumentation &= ~PTS_SELF ;So.
PTRACE_SELF_OFFdoesn't clearPTS_INSTRUMENTED? How can the task reset ->instrumentation ?
You are right again, I will remove the PTS_INSTRUMENTED flag.
Quoted 9 lines
kernel/ptrace.c asmlinkage long sys_ptrace(long request, · line 574
Quoted 6 lines
kernel/ptrace.c +4
-0+if (current->instrumentation) { + ret = -EPERM ; + goto out; + }So,
PTRACE_SELF_XXXdisables the "normal" ptrace. Not sure this is good.
I think that having two tracing system one over the other may be quite difficult to handle.
Pierre
Quoted 8 lines
Oleg.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Signature · 3 lines
============= Pierre Morel RTOS and Embedded Linux
Quoted 11 lines · Pierre Morel
Quoted 7 lines · Oleg Nesterov
I still think this patch shouldn't change
handle_signal().Once again. The signal handler for SIGSYS can first do
sys_ptrace(PTRACE_SELF_OFF)(which is filtered out), and then use any other syscall, so this change is not needed, afaics.Yes it can but what if the application forget to do it? It is a security so that the application do not bounce for ever.
The (buggy) task can be killed, this has nothing to do with security.
Quoted 1 line
From the security pov, this case doesn't differ from, say,
void sigh(int sig)
{
kill(getpid(), sig);
}
void main(void)
{
signal(SIGSYS, sigh);
kill(getpid(), SIGSYS);
}Or I missed something?
Quoted 4 lines
Quoted 2 lines
So,
PTRACE_SELF_XXXdisables the "normal" ptrace. Not sure this is good.I think that having two tracing system one over the other may be quite difficult to handle.
Yes I see.
But... well, I think we need Roland's opinion. I must admit, I am a bit sceptical about this patch ;) I mean, I don't really understand why it is useful. We can do the same with fork() + ptrace(). Yes, in that case we need an "extra" context switch for any traced syscall. But, do you have any "real life" example to demonstrate that the user-space solution sucks? We can even use CLONE_MM to speedup the context switch.
Pierre, don't get me wrong. I never used debuggers for myself, I will be happy to know I am wrong. I just don't understand.
As for ->instrumentation. If you are going to remove PTS_INSTRUMENTED, we need only one bit. We could use PF_PTS_SELF, but ->flags is already "contended". Perhaps you can do something like
--- include/linux/sched.h
+++ include/linux/sched.h
@@ -1088,6 +1088,7 @@ struct task_struct {
/* ??? */
unsigned int personality;
unsigned did_exec:1;
+ unsigned pts_self:1;
pid_t pid;
pid_t tgid;Both did_exec and pts_self can only be changed by current, so it is safe to share the same word. This way we don't enlarge task_struct.
Oleg.
Quoted 3 lines · Oleg Nesterov
But... well, I think we need Roland's opinion. I must admit, I am a bit sceptical about this patch ;) I mean, I don't really understand why it is useful
Yeah, I don't think they're completely coming clean on it, yet.
-- Dave
Hi Dave,
be optimistic, why not?
I bet we will ;)
Pierre
Quoted 17 lines · Dave Hansen
Quoted 4 lines · Oleg Nesterov
But... well, I think we need Roland's opinion. I must admit, I am a bit sceptical about this patch ;) I mean, I don't really understand why it is useful
Yeah, I don't think they're completely coming clean on it, yet.
-- Dave
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Signature · 3 lines
============= Pierre Morel RTOS and Embedded Linux
Hello Oleg,
You are right, the functionality can be implemented with the system call. But it means we have the overhead of a system call just to clear two bits, the TIF_SYSCALL_TRACE and the PTS_SELF.
On the other hand we have an overhead of one single "if" inside the handle_signal() function.
We can do the same with fork and ptrace, yes, but with a very big overhead on each system call and this is why this patch is so usefull: because with this patch you sit inside the thread when analysing it and have a direct access to all data without the need of IPC, ptrace or any task switch.
I will provide a test program and plan to release a tracing tool based on it.
I think I can reduce the task struct modification by using just a bit like you suggest if nobody seen any problem with this.
best regards,
Pierre
Quoted 74 lines · Oleg Nesterov
Quoted 4 lines · Pierre Morel
Quoted 2 lines · Oleg Nesterov
I still think this patch shouldn't change
handle_signal().Quoted 8 lines
Quoted 5 lines
Once again. The signal handler for SIGSYS can first do
sys_ptrace(PTRACE_SELF_OFF)(which is filtered out), and then use any other syscall, so this change is not needed, afaics.Yes it can but what if the application forget to do it? It is a security so that the application do not bounce for ever.
The (buggy) task can be killed, this has nothing to do with security.
From the security pov, this case doesn't differ from, say,
void sigh(int sig) { kill(getpid(), sig); } void main(void) { signal(SIGSYS, sigh); kill(getpid(), SIGSYS); }Or I missed something?
Quoted 6 lines
Quoted 3 lines
So,
PTRACE_SELF_XXXdisables the "normal" ptrace. Not sure this is good.I think that having two tracing system one over the other may be quite difficult to handle.
Yes I see.
But... well, I think we need Roland's opinion. I must admit, I am a bit sceptical about this patch ;) I mean, I don't really understand why it is useful. We can do the same with
fork()+ptrace(). Yes, in that case we need an "extra" context switch for any traced syscall. But, do you have any "real life" example to demonstrate that the user-space solution sucks? We can even useCLONE_MMto speedup the context switch.Pierre, don't get me wrong. I never used debuggers for myself, I will be happy to know I am wrong. I just don't understand.
As for ->instrumentation. If you are going to remove
PTS_INSTRUMENTED, we need only one bit. We could usePF_PTS_SELF, but ->flags is already "contended". Perhaps you can do something like--- include/linux/sched.h +++ include/linux/sched.h @@ -1088,6 +1088,7 @@ struct task_struct { /* ??? */ unsigned int personality; unsigned did_exec:1; + unsigned pts_self:1; pid_t pid; pid_t tgid;Both
did_execandpts_selfcan only be changed by current, so it is safe to share the same word. This way we don't enlargetask_struct.Oleg.
Signature · 3 lines
============= Pierre Morel RTOS and Embedded Linux
Hello Pierre,
Quoted 4 lines · Pierre Morel
You are right, the functionality can be implemented with the system call. But it means we have the overhead of a system call just to clear two bits, the
TIF_SYSCALL_TRACEand thePTS_SELF.
Yes.
So you want to optimize the code for the (imho very exotic) functionality. And again, the overhead of a system call is nothing compared to the signal delivery. I bet this overhead won't be visible with any benchmark.
Quoted 2 lines
On the other hand we have an overhead of one single "if" inside the
handle_signal()function.
What if everyone who wants to add the new functionality will add one single "if + code" to the core kernel just because he wants to add a very minor optimization for his needs?
And you forgot about the maintaince overhead. You forgot that this extra "if" uglifies/complicates the code.
This all is imho of course, and I'm not maintainer. But I promise I will argue against this change forever ;)
Quoted 8 lines
We can do the same with fork and ptrace, yes, but with a very big overhead on each system call and this is why this patch is so usefull:
because with this patch you sit inside the thread when analysing it and have a direct access to all data without the need of IPC, ptrace or any task switch.I will provide a test program and plan to release a tracing tool based on it.
Yes please, this would be very nice. Please do not count me, but I'm afraid I am not alone who needs to really understand why this patch is useful.
Oleg.
Quoted 5 lines · Pierre Morel
(quoted diff) +4
-0+ if (is_self_ptracing(regs->gprs[2])) { + if (!entryexit) { + struct siginfo info; +
Review
Quoted 10 lines
(quoted diff) +10
-0+ memset(&info, 0, sizeof(struct siginfo)); + info.si_signo = SIGSYS; + info.si_code = SYS_SYSCALL; + info.si_errno = regs->gprs[2]; + info.si_addr = (void *)regs->orig_gpr2; + send_sig_info(SIGSYS, &info, current); + regs->gprs[2] = -1; + } + return; + }
I see you didn't like my suggestions for consolidating some of these repetitive code bits across all the architectures. Did you give that a a shot? Would you like me to produce a patch on top of what you have here before this gets merged into mm?
-- Dave
Hi Dave,
Quoted 18 lines · Dave Hansen
Quoted 5 lines · Pierre Morel
(quoted diff) +4
-0+ if (is_self_ptracing(regs->gprs[2])) { + if (!entryexit) { + struct siginfo info; +Review
Quoted 10 lines
(quoted diff) +10
-0+ memset(&info, 0, sizeof(struct siginfo)); + info.si_signo = SIGSYS; + info.si_code = SYS_SYSCALL; + info.si_errno = regs->gprs[2]; + info.si_addr = (void *)regs->orig_gpr2; + send_sig_info(SIGSYS, &info, current); + regs->gprs[2] = -1; + } + return; + }
Quoted 3 lines
I see you didn't like my suggestions for consolidating some of these repetitive code bits across all the architectures. Did you give that a
I have read your suggestion, but the actual ptrace() implementation uses explicit reference to the different architecture dependent registers and I think that this portion of code is more readable if the patch keeps the actual conventions used by Roland.
Quoted 3 lines
a shot? Would you like me to produce a patch on top of what you have here before this gets merged into mm?
No Dave, thank you.
regards,
Pierre
Quoted 8 lines
-- Dave
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Signature · 14 lines
============= Pierre Morel RTOS and Embedded Linux ------------------------------------------------------------------------- This SF.Net email is sponsored by the Moblin Your Move Developer's challenge Build the coolest Linux based applications with Moblin SDK & win great prizes Grand prize is a trip for two to an Open Source event anywhere in the world http://moblin-contest.org/redirect.php?banner_id=100&url=/ _______________________________________________ User-mode-linux-devel mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/user-mode-linux-devel