2003-02-24 13:55:19

by Frank Cornelis

[permalink] [raw]
Subject: [PATCH] ptrace PTRACE_READDATA/WRITEDATA, kernel 2.5.62

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;
}


2003-02-24 14:09:32

by Daniel Jacobowitz

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

On Mon, Feb 24, 2003 at 03:05:14PM +0100, [email protected] 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

2003-02-24 14:41:48

by Frank Cornelis

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

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.

2003-02-24 14:55:06

by Daniel Jacobowitz

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

On Mon, Feb 24, 2003 at 03:51:22PM +0100, [email protected] 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

2003-02-24 18:26:56

by Manfred Spraul

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

On Mon, Feb 24, 2003 at 03:05:14PM +0100, [email protected] 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

2003-02-25 10:27:13

by Frank Cornelis

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

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.


2003-02-25 17:26:39

by Manfred Spraul

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

[email protected] 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



2003-02-25 21:29:16

by Andrew Morton

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

Manfred Spraul <[email protected]> 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...