ARM: ptrace: fix ptrace_read_user for !CONFIG_MMU platforms

Commit 68b7f715 ("nommu: ptrace support") added definitions for PT_TEXT_ADDR and friends, as well as adding ptrace support for reading from these magic offsets.

Unfortunately, this has probably never worked, since ptrace_read_user predicates reading on off < sizeof(struct user), returning -EOI otherwise.

This patch moves the offset size check until after we have tried to match it against either a magic value or an offset into pt_regs.

Quoted 8 lines

Commit 68b7f715 ("nommu: ptrace support") added definitions for PT_TEXT_ADDR and friends, as well as adding ptrace support for reading from these magic offsets.

Unfortunately, this has probably never worked, since ptrace_read_user predicates reading on off < sizeof(struct user), returning -EOI otherwise.

Quoted 2 lines

This patch moves the offset size check until after we have tried to match it against either a magic value or an offset into pt_regs.

Does this actually get used? The fact that it's been broken from day one and no one's raised the issue in 2.5 years suggests that it's dead code.

Quoted 12 lines · Will Deacon

Quoted 10 lines

Commit 68b7f715 ("nommu: ptrace support") added definitions for PT_TEXT_ADDR and friends, as well as adding ptrace support for reading from these magic offsets.

Unfortunately, this has probably never worked, since ptrace_read_user predicates reading on off < sizeof(struct user), returning -EOI otherwise.

This patch moves the offset size check until after we have tried to match it against either a magic value or an offset into pt_regs.

Quoted 2 lines

Does this actually get used? The fact that it's been broken from day one and no one's raised the issue in 2.5 years suggests that it's dead code.

I suspect I submitted the original patch. I don't remember the details, but
it definitely worked at the time. IIRC some other targets (m68k?) used an
even less palatable hack.

Quoted 3 lines · Paul Brook

I suspect I submitted the original patch. I don't remember the details, but
it definitely worked at the time. IIRC some other targets (m68k?) used an
even less palatable hack.

I don't believe that - looking at the history in git, since it was merged the code has been:

static int ptrace_read_user(struct task_struct *tsk, unsigned long off,
unsigned long __user *ret)

{
        unsigned long tmp;

        if (off & 3 || off >= sizeof(struct user))
                return -EIO;

        tmp = 0;
        if (off == PT_TEXT_ADDR)
                tmp = tsk->mm->start_code;
        else if (off == PT_DATA_ADDR)
                tmp = tsk->mm->start_data;
        else if (off == PT_TEXT_END_ADDR)
                tmp = tsk->mm->end_code;
        else if (off < sizeof(struct pt_regs))
                tmp = get_user_reg(tsk, off >> 2);

And since PT_TEXT_ADDR is 0x10000, this will fail with -EIO. So, there's no way this could have been used successfully in the last 2.5 years.

Maybe no one uses a debugger for uclinux programs?

Quoted 1 line · Russell King - ARM Linux

Maybe no one uses a debugger for uclinux programs?

I confess to finding this by inspection rather than a debugging failure.

Quoted 4 lines · Will Deacon

Quoted 1 line · Russell King - ARM Linux

Maybe no one uses a debugger for uclinux programs?

I confess to finding this by inspection rather than a debugging failure.

There is another explanation - that is no one in the uclinux world uses mainline kernels, and this bug has been fixed ages ago in some uclinux kernel tree.

That directly raises the question of the value of having the uclinux baggage in the mainline kernel if no one is using mainline kernels for uclinux work. If the uclinux folk aren't willing to pass up bug fixes, then having it in mainline is, frankly, a waste of space.

Quoted 6 lines · Russell King - ARM Linux

Quoted 4 lines · Will Deacon

Quoted 1 line · Russell King - ARM Linux

Maybe no one uses a debugger for uclinux programs?

I confess to finding this by inspection rather than a debugging failure.

Quoted 8 lines

There is another explanation - that is no one in the uclinux world uses mainline kernels, and this bug has been fixed ages ago in some uclinux kernel tree.

That directly raises the question of the value of having the uclinux baggage in the mainline kernel if no one is using mainline kernels for uclinux work. If the uclinux folk aren't willing to pass up bug fixes, then having it in mainline is, frankly, a waste of space.

I know the validation guys in ARM use mainline kernels for bringup on MMU-less CPUs (ok, they have extra patches on top but these tend to be platform-specific hacks since they're running on an RTL emulator. Plus they do periodically rebase onto new release kernels.). Debugging tends to be at a much lower level than GDB can provide though (i.e. waveforms), so this would have gone un-noticed.

I think it's worth keeping the support, particularly in light of the recent interest in M-class CPUs on the list.

The other thing is, as this has been broken right from the start, has gdb under uclinux even been tested?

I don't think fixing this in mainline until we know the full story behind this is the right thing to be doing. There's no point fixing a feature which no one's using.

Quoted 3 lines · Russell King - ARM Linux

I don't think fixing this in mainline until we know the full story behind this is the right thing to be doing. There's no point fixing a feature which no one's using.

Understood. Paul - would you please be able to confirm that:

(a) GDB is currently broken on uclinux? (it certainly looks that way)
(b) My proposed patch fixes the problem?

If you don't have an environment set up, I wonder if there's somebody else we can poke who's playing with this stuff.

Quoted 5 lines · Will Deacon

Quoted 3 lines · Russell King - ARM Linux

I don't think fixing this in mainline until we know the full story behind this is the right thing to be doing. There's no point fixing a feature which no one's using.

Quoted 7 lines

Understood. Paul - would you please be able to confirm that:

(a) GDB is currently broken on uclinux? (it certainly looks that way)
(b) My proposed patch fixes the problem?

If you don't have an environment set up, I wonder if there's somebody else we can poke who's playing with this stuff.

Well in the meantime I had a play with the latest CodeSourcery tools:

GNU gdbserver (Sourcery G++ Lite 2011.03-46) 7.2.50.20100908-cvs Copyright (C) 2010 Free Software Foundation, Inc.
gdbserver is free software, covered by the GNU General Public License. This gdbserver was configured as "arm-uclinuxeabi"

on the target and

GNU gdb (Sourcery G++ Lite 2011.03-46) 7.2.50.20100908-cvs Copyright (C) 2010 Free Software Foundation, Inc.
License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html>
This is free software: you are free to change and redistribute it. There is NO WARRANTY, to the extent permitted by law. Type "show copying" and "show warranty" for details.
This GDB was configured as "--host=i686-pc-linux-gnu --target=arm-uclinuxeabi".

on the host.

It seems as though my ptrace patch makes *no difference* because these tools don't even use the PT_ADDR_TEXT etc magic offsets! As a result, trying to set a breakpoint by symbol fails miserably because it tries to poke the symbol offset directly, without adding on the base address of the text.

Are there any tools available that use these magic numbers or are mine just too old? Given that the whole thing dies after a while with:

[  909.062821] [430] gdbserver: obsolete system call 02b37558.

I'm not entirely convinced by the stability of what I'm using (that syscall number looks like an address to me).

Quoted 9 lines · Will Deacon

It seems as though my ptrace patch makes *no difference* because these tools don't even use the PT_ADDR_TEXT etc magic offsets! As a result, trying to set a breakpoint by symbol fails miserably because it tries to poke the symbol offset directly, without adding on the base address of the text.

Are there any tools available that use these magic numbers or are mine just too old? Given that the whole thing dies after a while with:

Quoted 4 lines

[  909.062821] [430] gdbserver: obsolete system call 02b37558.

I'm not entirely convinced by the stability of what I'm using (that syscall number looks like an address to me).

That suggests that this stuff definitely hasn't been tested, and there's no users of it (if there were I'm sure someone - either gdb folk or some kernel people) would've seen a bug report.

Therefore, I propose that we remove this code from the kernel unless there's someone out there who can positively test this stuff as definitely working.

Quoted 15 lines · Russell King - ARM Linux

Quoted 13 lines · Will Deacon

It seems as though my ptrace patch makes *no difference* because these tools don't even use the PT_ADDR_TEXT etc magic offsets! As a result, trying to set a breakpoint by symbol fails miserably because it tries to poke the symbol offset directly, without adding on the base address of the text.

Are there any tools available that use these magic numbers or are mine just too old? Given that the whole thing dies after a while with:

[  909.062821] [430] gdbserver: obsolete system call 02b37558.

I'm not entirely convinced by the stability of what I'm using (that syscall number looks like an address to me).

Quoted 3 lines

That suggests that this stuff definitely hasn't been tested, and there's no users of it (if there were I'm sure someone - either gdb folk or some kernel people) would've seen a bug report.

My glancing at the latest GDB sources does suggest that this stuff ought to be getting used, so I should probably try building a nommu configuration from a more recent codebase. It seems that you're right that nobody is using this stuff though.

Quoted 4 lines · Will Deacon

Quoted 1 line · Russell King - ARM Linux

Maybe no one uses a debugger for uclinux programs?

I confess to finding this by inspection rather than a debugging failure.

There is another explanation - that is no one in the uclinux world uses mainline kernels, and this bug has been fixed ages ago in some uclinux kernel tree.

That directly raises the question of the value of having the uclinux baggage in the mainline kernel if no one is using mainline kernels for uclinux work. If the uclinux folk aren't willing to pass up bug fixes, then having it in mainline is, frankly, a waste of space.