[PATCH] ARM: ptrace: fix ptrace_read_user for !CONFIG_MMU platforms

[PATCH] ARM: ptrace: fix ptrace_read_user for !CONFIG_MMU platforms

[Will Deacon]

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.

Cc: Paul Brook <paul@codesourcery.com>
Signed-off-by: Will Deacon <will.deacon@arm.com>
---
 arch/arm/kernel/ptrace.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/arch/arm/kernel/ptrace.c b/arch/arm/kernel/ptrace.c
index e33870f..b68b52b 100644
--- a/arch/arm/kernel/ptrace.c
+++ b/arch/arm/kernel/ptrace.c
@@ -256,7 +256,7 @@ static int ptrace_read_user(struct task_struct *tsk, unsigned long off,
 {
 	unsigned long tmp;
 
-	if (off & 3 || off >= sizeof(struct user))
+	if (off & 3)
 		return -EIO;
 
 	tmp = 0;
@@ -268,6 +268,8 @@ static int ptrace_read_user(struct task_struct *tsk, unsigned long off,
 		tmp = tsk->mm->end_code;
 	else if (off < sizeof(struct pt_regs))
 		tmp = get_user_reg(tsk, off >> 2);
+	else if (off >= sizeof(struct user))
+		return -EIO;
 
 	return put_user(tmp, ret);
 }
-- 
1.7.4.1

[PATCH] ARM: ptrace: fix ptrace_read_user for !CONFIG_MMU platforms

[Russell King - ARM Linux]

On Mon, Feb 20, 2012 at 06:37:09PM +0000, Will Deacon wrote:
> 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.

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.

[PATCH] ARM: ptrace: fix ptrace_read_user for !CONFIG_MMU platforms

[Paul Brook]

> On Mon, Feb 20, 2012 at 06:37:09PM +0000, Will Deacon wrote:
> > 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.
> 
> 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.

This (or equivalent) is necessary to do any debugging on an mmu-less (i.e. 
uclinux) system.

Paul

[PATCH] ARM: ptrace: fix ptrace_read_user for !CONFIG_MMU platforms

[Russell King - ARM Linux]

On Tue, Feb 21, 2012 at 01:24:22AM +0000, Paul Brook wrote:
> > On Mon, Feb 20, 2012 at 06:37:09PM +0000, Will Deacon wrote:
> > > 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.
> > 
> > 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.

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?

[PATCH] ARM: ptrace: fix ptrace_read_user for !CONFIG_MMU platforms

[Will Deacon]

On Tue, Feb 21, 2012 at 08:36:12AM +0000, Russell King - ARM Linux wrote:
> On Tue, Feb 21, 2012 at 01:24:22AM +0000, Paul Brook wrote:
> > 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.

Agreed, it does seem that this would always have failed, however a quick look
at GDB also suggests that it would at least try to access these guys on an
MMU-less system.

> Maybe no one uses a debugger for uclinux programs?

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

Will

[PATCH] ARM: ptrace: fix ptrace_read_user for !CONFIG_MMU platforms

[Russell King - ARM Linux]

On Tue, Feb 21, 2012 at 10:00:41AM +0000, Will Deacon wrote:
> On Tue, Feb 21, 2012 at 08:36:12AM +0000, Russell King - ARM Linux wrote:
> > 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.

[PATCH] ARM: ptrace: fix ptrace_read_user for !CONFIG_MMU platforms

[Will Deacon]

On Tue, Feb 21, 2012 at 10:10:52AM +0000, Russell King - ARM Linux wrote:
> On Tue, Feb 21, 2012 at 10:00:41AM +0000, Will Deacon wrote:
> > On Tue, Feb 21, 2012 at 08:36:12AM +0000, Russell King - ARM Linux wrote:
> > > 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.

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.

Will

[PATCH] ARM: ptrace: fix ptrace_read_user for !CONFIG_MMU platforms

[Russell King - ARM Linux]

On Tue, Feb 21, 2012 at 10:52:50AM +0000, Will Deacon wrote:
> On Tue, Feb 21, 2012 at 10:10:52AM +0000, Russell King - ARM Linux wrote:
> > On Tue, Feb 21, 2012 at 10:00:41AM +0000, Will Deacon wrote:
> > > On Tue, Feb 21, 2012 at 08:36:12AM +0000, Russell King - ARM Linux wrote:
> > > > 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.
> 
> 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.

In any case, I think we need to know whether this really is a problem or
not.  If no one has noticed that this is broken in the last 2.5 years,
that implies that no one is using this, and so there's no point either
the kernel or gdb carrying support which no one is using.

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.

[PATCH] ARM: ptrace: fix ptrace_read_user for !CONFIG_MMU platforms

[Will Deacon]

On Tue, Feb 21, 2012 at 11:35:48AM +0000, Russell King - ARM Linux wrote:
> The other thing is, as this has been broken right from the start, has
> gdb under uclinux even been tested?

I checked with the tools guys here and they don't yet do !MMU testing,
although I think it's on the list.

> 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.

Will

[PATCH] ARM: ptrace: fix ptrace_read_user for !CONFIG_MMU platforms

[Greg Ungerer]

On 21/02/12 20:10, Russell King - ARM Linux wrote:
> On Tue, Feb 21, 2012 at 10:00:41AM +0000, Will Deacon wrote:
>> On Tue, Feb 21, 2012 at 08:36:12AM +0000, Russell King - ARM Linux wrote:
>>> 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.

There is no separate uclinux kernels or separate patch sets, hasn't been
for a long time. No need, it is all in mainline. There are however plenty
of vendors and individuals using non-MMU platforms who do not push back
to mainline.


> 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.

What has historically been called uclinux includes quite a number
of architectures. The problem in this thread is quite clearly ARM
specific. I guess noone using non-MMU Linux on ARM uses gdb to debug.

Regards
Greg


------------------------------------------------------------------------
Greg Ungerer  --  Principal Engineer        EMAIL:     gerg at snapgear.com
SnapGear Group, McAfee                      PHONE:       +61 7 3435 2888
8 Gardner Close                             FAX:         +61 7 3217 5323
Milton, QLD, 4064, Australia                WEB: http://www.SnapGear.com

[PATCH] ARM: ptrace: fix ptrace_read_user for !CONFIG_MMU platforms

[Will Deacon]

On Tue, Feb 21, 2012 at 01:22:16PM +0000, Will Deacon wrote:
> On Tue, Feb 21, 2012 at 11:35:48AM +0000, Russell King - ARM Linux wrote:
> > 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.

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).

Will

[PATCH] ARM: ptrace: fix ptrace_read_user for !CONFIG_MMU platforms

[Russell King - ARM Linux]

On Fri, Feb 24, 2012 at 02:36:55PM +0000, Will Deacon wrote:
> On Tue, Feb 21, 2012 at 01:22:16PM +0000, Will Deacon wrote:
> > On Tue, Feb 21, 2012 at 11:35:48AM +0000, Russell King - ARM Linux wrote:
> > > 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.
> 
> 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).

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.

[PATCH] ARM: ptrace: fix ptrace_read_user for !CONFIG_MMU platforms

[Will Deacon]

Hi Russell,

On Fri, Feb 24, 2012 at 06:16:21PM +0000, Russell King - ARM Linux wrote:
> On Fri, Feb 24, 2012 at 02:36:55PM +0000, Will Deacon wrote:
> > 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).
> 
> 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.

> 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.

I started looking at nommu recently and I've got a few fixes for the kernel
(I'll post them once it's all tidied up). Let's leave this in until I've
either (a) validated that recent GDB builds work or (b) lost the will to
live and come crawling back to my beloved MMU.

Will

[PATCH] ARM: ptrace: fix ptrace_read_user for !CONFIG_MMU platforms

[Will Deacon]

On Wed, Feb 29, 2012 at 06:52:37PM +0000, Will Deacon wrote:
> On Fri, Feb 24, 2012 at 06:16:21PM +0000, Russell King - ARM Linux wrote:
> > On Fri, Feb 24, 2012 at 02:36:55PM +0000, Will Deacon wrote:
> > > 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).
> > 
> > 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.

Ok, I have an update on this now. I had to hack GDB as well as the kernel
(!) since it wasn't picking up our PT_* definitions from asm/ptrace.h. With
that change in place, GDB can be persuaded to add on the offsets correctly
and, with a fixed kernel, breakpoints and symbol resolution start to work.

So now the question is: do I fix GDB or the kernel first? I can imagine the
GDB guys not wanting to merge anything until the kernel has a working ptrace
interface in this regard.

Will