Re: [PATCH] ptrace PTRACE_READDATA/WRITEDATA, kernel 2.5.62

Re: [PATCH] ptrace PTRACE_READDATA/WRITEDATA, kernel 2.5.62

[Manfred Spraul]

On Mon, Feb 24, 2003 at 03:05:14PM +0100, fcorneli@elis.rug.ac.be wrote:

>+		ret = 0;
>+		res = ptrace_readdata(child, addr, (void *)addr2, data);
>+		if (res == data)
>+			break;
>  
>
You mention sparc - have you tested if that works on sparc?
ptrace_readdata assumes that addr2 is a pointer to kernel space, not 
user space. It works by chance on i386, but that's not acceptable for 
merging.
You must double buffer, check mem_read in fs/proc/base.c

Daniel wrote:

>Thirdly, I was going to do this, but I ended up making GDB use pread64
>on /dev/mem instead.  It works with no kernel modifications, and is
>just as fast.
>  
>
I assume you mean /proc/<pid>/mem. Performance is identical, same 
implementation.

--
    Manfred

Re: [PATCH] ptrace PTRACE_READDATA/WRITEDATA, kernel 2.5.62

[fcorneli]

Hi,

> >+		ret = 0;
> >+		res = ptrace_readdata(child, addr, (void *)addr2, data);
> >+		if (res == data)
> >+			break;
> >  
> >
> You mention sparc - have you tested if that works on sparc?
> ptrace_readdata assumes that addr2 is a pointer to kernel space, not 
> user space. It works by chance on i386, but that's not acceptable for 
> merging.
> You must double buffer, check mem_read in fs/proc/base.c

I don't own a sparc so I couldn't test it. But since the ptrace_readdata 
lives in the kernel tree for some time now and nobody is complaining about 
it I assume the sparc usage of ptrace_readdata is OK. I did test it on 
i386 and it works just fine.

When I look at the implementation of ptrace_readdata the dst (3th arg) has 
to be a pointer to user space; see: copy_to_user(dst, buf, retval). Only 
access_process_vm wants a kernel pointer. Anyway access_process_vm has 
some known issues, see:
http://www.kernel.org/pub/linux/kernel/people/akpm/patches/2.5/2.5.62/2.5.62-mm3/broken-out/ptrace-flush.patch
but it's getting fixed I hope.

As for that double buffering, it's already there. See usage of: 
kernel/ptrace.c:ptrace_readdata:char buf[128];
Please notice this double buffering can be eliminated as done in my 
exptrace patch, available at:
http://www.elis.rug.ac.be/~fcorneli/downloads/devel/exptrace-0.3.6pre7-2.4.20.patch
This speeds up everything... but the code needs some more testing, and a 
port to 2.5.

Regards,
Frank.

Re: [PATCH] ptrace PTRACE_READDATA/WRITEDATA, kernel 2.5.62

[Manfred Spraul]

fcorneli@elis.rug.ac.be wrote:

>But since the ptrace_readdata 
>lives in the kernel tree for some time now and nobody is complaining about 
>it I assume the sparc usage of ptrace_readdata is OK. I did test it on 
>i386 and it works just fine.
>  
>
Sorry, my fault. I remembered that someone must do double buffering. 
I've overlooked that ptrace_readdata does the double buffering.

>When I look at the implementation of ptrace_readdata the dst (3th arg) has 
>to be a pointer to user space; see: copy_to_user(dst, buf, retval). Only 
>access_process_vm wants a kernel pointer. Anyway access_process_vm has 
>some known issues, see:
>http://www.kernel.org/pub/linux/kernel/people/akpm/patches/2.5/2.5.62/2.5.62-mm3/broken-out/ptrace-flush.patch
>but it's getting fixed I hope.
>  
>
This patch seems to be wrong.
flush_dcache_page is not a replacement for flush_page_to_ram(): It's an 
optimized cache flush function, only valid for page cache pages:
If a page is mapped only once, then no aliasing can occur and the flush 
is not required.
For page cache pages, "mapped once" is equivalient to an empty 
page->mapping->i_mmap{,_shared}. The pages are mapped once in the page 
cache, and _if_ they are mapped in user space, then 
page->mapping->i_mmap{,_shared} is not empty.

ptrace can be called on random addresses - sysv shm, anonymous pages, 
etc. page->mapping->i_mmap{,_shared} is meaningless.
If you think about it, if ptrace accesses a page, then it's guaranteed 
to be mapped twice: once in user space, once by the kmap_atomic() for 
the kernel space access.

--
    Manfred

Re: [PATCH] ptrace PTRACE_READDATA/WRITEDATA, kernel 2.5.62

[Andrew Morton]

Manfred Spraul <manfred@colorfullife.com> wrote:
>
> >http://www.kernel.org/pub/linux/kernel/people/akpm/patches/2.5/2.5.62/2.5.62-mm3/broken-out/ptrace-flush.patch
> >but it's getting fixed I hope.
> >  
> >
> This patch seems to be wrong.

Yes, it is wrong.  I discussed it with davem a while back and he was planning
on fixing it up for real.  I just keep the patch floating about as a reminder
that the thing which it doesn't fix needs fixing.

I'll update the changelog...

[PATCH] ptrace PTRACE_READDATA/WRITEDATA, kernel 2.5.62

[fcorneli]

Hi,

I ported some generic SunOS ptrace requests from my 2.4 exptrace kernel 
patch to the 2.5 tree. The PTRACE_READDATA/WRITEDATA requests have been 
available for a long time for the sparc architecture but I think they're 
also very useful on the i386 arch since PTRACE_PEEKDATA/POKEDATA are way 
too slow when handling large data blocks.

Frank.

diff -Nur linux-2.5.62.orig/arch/i386/kernel/ptrace.c linux-2.5.62/arch/i386/kernel/ptrace.c
--- linux-2.5.62.orig/arch/i386/kernel/ptrace.c	2003-02-17 23:56:10.000000000 +0100
+++ linux-2.5.62/arch/i386/kernel/ptrace.c	2003-02-24 14:12:02.000000000 +0100
@@ -229,7 +229,7 @@
 	return 0;
 }
 
-asmlinkage int sys_ptrace(long request, long pid, long addr, long data)
+asmlinkage int sys_ptrace(long request, long pid, long addr, long data, long addr2)
 {
 	struct task_struct *child;
 	struct user * dummy = NULL;
@@ -355,6 +355,34 @@
 		  }
 		  break;
 
+	case PTRACE_READDATA:
+	case PTRACE_READTEXT: {
+		int res;
+		ret = 0;
+		res = ptrace_readdata(child, addr, (void *)addr2, data);
+		if (res == data)
+			break;
+		if (res >= 0)
+			ret = -EIO;
+		else
+			ret = res;
+		break;
+	}
+
+	case PTRACE_WRITEDATA:
+	case PTRACE_WRITETEXT: {
+		int res;
+		ret = 0;
+		res = ptrace_writedata(child, (void *)addr2, addr, data);
+		if (res == data)
+			break;
+		if (res >= 0)
+			ret = -EIO;
+		else
+			ret = res;
+		break;
+	}
+
 	case PTRACE_SYSCALL: /* continue and stop at next (return from) syscall */
 	case PTRACE_CONT: { /* restart after signal. */
 		long tmp;
diff -Nur linux-2.5.62.orig/include/asm-i386/ptrace.h linux-2.5.62/include/asm-i386/ptrace.h
--- linux-2.5.62.orig/include/asm-i386/ptrace.h	2003-02-17 23:55:51.000000000 +0100
+++ linux-2.5.62/include/asm-i386/ptrace.h	2003-02-24 13:54:33.000000000 +0100
@@ -53,6 +53,10 @@
 
 #define PTRACE_GET_THREAD_AREA    25
 #define PTRACE_SET_THREAD_AREA    26
+#define PTRACE_READDATA           27
+#define PTRACE_WRITEDATA          28
+#define PTRACE_READTEXT           29
+#define PTRACE_WRITETEXT          30
 
 #ifdef __KERNEL__
 #define user_mode(regs) ((VM_MASK & (regs)->eflags) || (3 & (regs)->xcs))

-----------------------------------------------------------------------
Demo:
#include <stdio.h>
#include <stdlib.h>
#include <sys/ptrace.h>
#include <asm/ptrace.h>
#include <sys/wait.h>
#include <sched.h>
#include <syscall.h>
#include <errno.h>

#define STACK_SIZE (1024*2)

#define DATA_SIZE (1024*64)

#ifndef __NR_ptrace5
#define __NR_ptrace5 __NR_ptrace
#endif

// we need to pass 5 parameters to PTRACE_READDATA/WRITEDATA
_syscall5 (int, ptrace5, int, request, pid_t, pid, void *, addr, void *, data,
		void *, addr2);

static int child_func(void *param);
static volatile int go = 0;
static char *common_data;

int main()
{
	int status, pid, i;
	char *stack, *buf;
	stack = malloc(STACK_SIZE);
	common_data = malloc(DATA_SIZE);
	srand(0);
	for (i = 0; i < DATA_SIZE; i++)
		common_data[i] = (int)(255.0 * rand() / (RAND_MAX + 1.0));
	pid = clone(child_func, stack + STACK_SIZE, SIGCHLD | CLONE_VM, NULL);
	ptrace(PTRACE_ATTACH, pid, NULL, NULL);
	waitpid(pid, &status, 0);
	go = 1;
	buf = malloc(DATA_SIZE);
	for (i = 0; i < DATA_SIZE; i++)
		buf[i] = '\0';
	while (1) {
		ptrace(PTRACE_SYSCALL, pid, NULL, NULL);
		waitpid(pid, &status, 0);
		if (WIFEXITED(status))
			exit(0);
		if (WIFSTOPPED(status) && WSTOPSIG(status) == SIGTRAP) {
			printf("syscall: %d (EAX: %d)\n", 
					(int)ptrace(PTRACE_PEEKUSER, pid, ORIG_EAX * 4, NULL),
					(int)ptrace(PTRACE_PEEKUSER, pid, EAX * 4, NULL));
			if (ptrace5(PTRACE_READDATA, pid, common_data, (void *)DATA_SIZE, buf)) {
				perror("PTRACE_READDATA");
				exit(1);
			}
			for (i = 0; i < DATA_SIZE; i++)
				if (buf[i] != common_data[i]) {
					fprintf(stderr, "PTRACE_READDATA error.\n");
					exit(1);
				}
		}
	}
}

static int child_func(void *param)
{
	while (!go) ;
	getpid();
	getppid();
	return 0;
}

Re: [PATCH] ptrace PTRACE_READDATA/WRITEDATA, kernel 2.5.62

[Daniel Jacobowitz]

On Mon, Feb 24, 2003 at 03:05:14PM +0100, fcorneli@elis.rug.ac.be wrote:
> Hi,
> 
> I ported some generic SunOS ptrace requests from my 2.4 exptrace kernel 
> patch to the 2.5 tree. The PTRACE_READDATA/WRITEDATA requests have been 
> available for a long time for the sparc architecture but I think they're 
> also very useful on the i386 arch since PTRACE_PEEKDATA/POKEDATA are way 
> too slow when handling large data blocks.

FYI Frank, three things.  First of all, I really don't like the
interface of adding a second address to ptrace; I believe it interferes
with PIC on x86, since IIRC the extra argument would go in %ebx.  The
BSDs have a nice interface involving passing a request structure. 
Secondly, the implementation should be in kernel/ptrace.c not under
i386, we're trying to stop doing that.

Thirdly, I was going to do this, but I ended up making GDB use pread64
on /dev/mem instead.  It works with no kernel modifications, and is
just as fast.

-- 
Daniel Jacobowitz
MontaVista Software                         Debian GNU/Linux Developer

Re: [PATCH] ptrace PTRACE_READDATA/WRITEDATA, kernel 2.5.62

[fcorneli]

Hi,

> FYI Frank, three things.  First of all, I really don't like the
> interface of adding a second address to ptrace; I believe it interferes
> with PIC on x86, since IIRC the extra argument would go in %ebx.  
> The BSDs have a nice interface involving passing a request structure. 

I don't see the problem since we can pass up to 6 parameters on the i386 
architecture. The extra argument will be passed on using the stack as the 
other arguments do because of the asmlinkage directive. Using a structure 
slows everything down too much; if you can use the stack I think it's 
better to do so. What about that PIC?

> Secondly, the implementation should be in kernel/ptrace.c not under
> i386, we're trying to stop doing that.

The implementation is already in kernel/ptrace.c, only the usage lives 
under the arch-dependent directories since there the sys_ptrace entries 
are located.

> Thirdly, I was going to do this, but I ended up making GDB use pread64
> on /dev/mem instead.  It works with no kernel modifications, and is
> just as fast.

mmm... I thought it would be convenient to use ptrace for all the trace 
work.

Frank.

Re: [PATCH] ptrace PTRACE_READDATA/WRITEDATA, kernel 2.5.62

[Daniel Jacobowitz]

On Mon, Feb 24, 2003 at 03:51:22PM +0100, fcorneli@elis.rug.ac.be wrote:
> Hi,
> 
> > FYI Frank, three things.  First of all, I really don't like the
> > interface of adding a second address to ptrace; I believe it interferes
> > with PIC on x86, since IIRC the extra argument would go in %ebx.  
> > The BSDs have a nice interface involving passing a request structure. 
> 
> I don't see the problem since we can pass up to 6 parameters on the i386 
> architecture. The extra argument will be passed on using the stack as the 
> other arguments do because of the asmlinkage directive. Using a structure 
> slows everything down too much; if you can use the stack I think it's 
> better to do so. What about that PIC?

I seem to remember this (five-arg syscalls) causing problems before. 
Maybe it was on a different platform.

> > Secondly, the implementation should be in kernel/ptrace.c not under
> > i386, we're trying to stop doing that.
> 
> The implementation is already in kernel/ptrace.c, only the usage lives 
> under the arch-dependent directories since there the sys_ptrace entries 
> are located.

Not any more; it should be in ptrace_request for anything new.  Yes, if
you're adding an argument, that makes this more work.

> > Thirdly, I was going to do this, but I ended up making GDB use pread64
> > on /dev/mem instead.  It works with no kernel modifications, and is
> > just as fast.
> 
> mmm... I thought it would be convenient to use ptrace for all the trace 
> work.

I've found it really doesn't make a difference.

-- 
Daniel Jacobowitz
MontaVista Software                         Debian GNU/Linux Developer