[PATCH 0/2] Consolidate redundant register/stack access code

[PATCH 0/2] Consolidate redundant register/stack access code

[David Long]

From: "David A. Long" <dave.long@linaro.org>

Move duplicate and functionally equivalent code for accessing registers
and stack (CONFIG_HAVE_REGS_AND_STACK_ACCESS_API) from arch subdirs into
common kernel files.

Note: Help regression testing s390, hexagon, and sh would be appreciated.
      Powerpc builds but I have not verified the functionality.

David A. Long (2):
  Move the pt_regs_offset struct definition from arch to common include
    file
  Consolidate redundant register/stack access code

 arch/arm/include/asm/ptrace.h          |  6 ---
 arch/arm/kernel/ptrace.c               | 72 +---------------------------------
 arch/hexagon/include/uapi/asm/ptrace.h |  3 --
 arch/powerpc/include/asm/ptrace.h      | 38 ------------------
 arch/powerpc/kernel/ptrace.c           | 39 +-----------------
 arch/s390/include/asm/ptrace.h         |  3 --
 arch/s390/kernel/ptrace.c              | 70 ++++++++++-----------------------
 arch/sh/include/asm/ptrace.h           | 44 ---------------------
 arch/sh/kernel/Makefile                |  2 +-
 arch/sh/kernel/ptrace.c                | 33 ----------------
 arch/sh/kernel/ptrace_32.c             |  2 +-
 arch/sh/kernel/ptrace_64.c             |  2 +-
 arch/x86/include/asm/ptrace.h          | 37 -----------------
 arch/x86/kernel/ptrace.c               | 39 +-----------------
 include/linux/ptrace.h                 | 51 ++++++++++++++++++++++++
 kernel/ptrace.c                        | 38 ++++++++++++++++++
 16 files changed, 116 insertions(+), 363 deletions(-)
 delete mode 100644 arch/sh/kernel/ptrace.c

-- 
1.8.1.2

[PATCH 1/2] Move the pt_regs_offset struct definition from arch to common include file

[David Long]

From: "David A. Long" <dave.long@linaro.org>

The pt_regs_offset structure is used for HAVE_REGS_AND_STACK_ACCESS_API
 feature and has identical definitions in four different arch ptrace.h
include files. It seems unlikely that definition would ever need to be
changed regardless of architecture so lets move it into
include/linux/ptrace.h.

Signed-off-by: David A. Long <dave.long@linaro.org>
---
 arch/arm/kernel/ptrace.c     | 5 -----
 arch/powerpc/kernel/ptrace.c | 5 -----
 arch/sh/include/asm/ptrace.h | 5 -----
 arch/x86/kernel/ptrace.c     | 5 -----
 include/linux/ptrace.h       | 9 +++++++++
 5 files changed, 9 insertions(+), 20 deletions(-)

diff --git a/arch/arm/kernel/ptrace.c b/arch/arm/kernel/ptrace.c
index ef9119f..fb45cf1 100644
--- a/arch/arm/kernel/ptrace.c
+++ b/arch/arm/kernel/ptrace.c
@@ -59,11 +59,6 @@
 #define BREAKINST_THUMB	0xde01
 #endif
 
-struct pt_regs_offset {
-	const char *name;
-	int offset;
-};
-
 #define REG_OFFSET_NAME(r) \
 	{.name = #r, .offset = offsetof(struct pt_regs, ARM_##r)}
 #define REG_OFFSET_END {.name = NULL, .offset = 0}
diff --git a/arch/powerpc/kernel/ptrace.c b/arch/powerpc/kernel/ptrace.c
index f21897b..ab81733 100644
--- a/arch/powerpc/kernel/ptrace.c
+++ b/arch/powerpc/kernel/ptrace.c
@@ -52,11 +52,6 @@
 #define PARAMETER_SAVE_AREA_OFFSET	48  /* bytes */
 #endif
 
-struct pt_regs_offset {
-	const char *name;
-	int offset;
-};
-
 #define STR(s)	#s			/* convert to string */
 #define REG_OFFSET_NAME(r) {.name = #r, .offset = offsetof(struct pt_regs, r)}
 #define GPR_OFFSET_NAME(num)	\
diff --git a/arch/sh/include/asm/ptrace.h b/arch/sh/include/asm/ptrace.h
index 2506c7d..677c72b 100644
--- a/arch/sh/include/asm/ptrace.h
+++ b/arch/sh/include/asm/ptrace.h
@@ -23,11 +23,6 @@
 /*
  * kprobe-based event tracer support
  */
-struct pt_regs_offset {
-	const char *name;
-	int offset;
-};
-
 #define REG_OFFSET_NAME(r) {.name = #r, .offset = offsetof(struct pt_regs, r)}
 #define REGS_OFFSET_NAME(num)	\
 	{.name = __stringify(r##num), .offset = offsetof(struct pt_regs, regs[num])}
diff --git a/arch/x86/kernel/ptrace.c b/arch/x86/kernel/ptrace.c
index a7bc794..f135d35 100644
--- a/arch/x86/kernel/ptrace.c
+++ b/arch/x86/kernel/ptrace.c
@@ -53,11 +53,6 @@ enum x86_regset {
 	REGSET_IOPERM32,
 };
 
-struct pt_regs_offset {
-	const char *name;
-	int offset;
-};
-
 #define REG_OFFSET_NAME(r) {.name = #r, .offset = offsetof(struct pt_regs, r)}
 #define REG_OFFSET_END {.name = NULL, .offset = 0}
 
diff --git a/include/linux/ptrace.h b/include/linux/ptrace.h
index 987a73a..b0b1ee6 100644
--- a/include/linux/ptrace.h
+++ b/include/linux/ptrace.h
@@ -383,4 +383,13 @@ extern int task_current_syscall(struct task_struct *target, long *callno,
 				unsigned long args[6], unsigned int maxargs,
 				unsigned long *sp, unsigned long *pc);
 
+#ifdef	CONFIG_HAVE_REGS_AND_STACK_ACCESS_API
+
+struct pt_regs_offset {
+	const char *name;
+	int offset;
+};
+
+#endif	/* CONFIG_HAVE_REGS_AND_STACK_ACCESS_API */
+
 #endif
-- 
1.8.1.2

Re: [PATCH 1/2] Move the pt_regs_offset struct definition from arch to common include file

[Rob Herring]

On Mon, Jun 15, 2015 at 11:42 AM, David Long <dave.long@linaro.org> wrote:
> From: "David A. Long" <dave.long@linaro.org>
>
> The pt_regs_offset structure is used for HAVE_REGS_AND_STACK_ACCESS_API
>  feature and has identical definitions in four different arch ptrace.h
> include files. It seems unlikely that definition would ever need to be
> changed regardless of architecture so lets move it into
> include/linux/ptrace.h.
>
> Signed-off-by: David A. Long <dave.long@linaro.org>
> ---
>  arch/arm/kernel/ptrace.c     | 5 -----
>  arch/powerpc/kernel/ptrace.c | 5 -----
>  arch/sh/include/asm/ptrace.h | 5 -----
>  arch/x86/kernel/ptrace.c     | 5 -----
>  include/linux/ptrace.h       | 9 +++++++++
>  5 files changed, 9 insertions(+), 20 deletions(-)
>
> diff --git a/arch/arm/kernel/ptrace.c b/arch/arm/kernel/ptrace.c
> index ef9119f..fb45cf1 100644
> --- a/arch/arm/kernel/ptrace.c
> +++ b/arch/arm/kernel/ptrace.c
> @@ -59,11 +59,6 @@
>  #define BREAKINST_THUMB        0xde01
>  #endif
>
> -struct pt_regs_offset {
> -       const char *name;
> -       int offset;
> -};
> -
>  #define REG_OFFSET_NAME(r) \
>         {.name = #r, .offset = offsetof(struct pt_regs, ARM_##r)}
>  #define REG_OFFSET_END {.name = NULL, .offset = 0}

Can't you also move these? ARM is complicated with the "ARM_"
prefixing, but the others appear to be the same. Maybe you can remove
the prefix or redefine the macro for ARM.

Rob

Re: [PATCH 1/2] Move the pt_regs_offset struct definition from arch to common include file

[David Long]

On 06/16/15 09:17, Rob Herring wrote:
> On Mon, Jun 15, 2015 at 11:42 AM, David Long <dave.long@linaro.org> wrote:
>> From: "David A. Long" <dave.long@linaro.org>
>>
>> The pt_regs_offset structure is used for HAVE_REGS_AND_STACK_ACCESS_API
>>   feature and has identical definitions in four different arch ptrace.h
>> include files. It seems unlikely that definition would ever need to be
>> changed regardless of architecture so lets move it into
>> include/linux/ptrace.h.
>>
>> Signed-off-by: David A. Long <dave.long@linaro.org>
>> ---
>>   arch/arm/kernel/ptrace.c     | 5 -----
>>   arch/powerpc/kernel/ptrace.c | 5 -----
>>   arch/sh/include/asm/ptrace.h | 5 -----
>>   arch/x86/kernel/ptrace.c     | 5 -----
>>   include/linux/ptrace.h       | 9 +++++++++
>>   5 files changed, 9 insertions(+), 20 deletions(-)
>>
>> diff --git a/arch/arm/kernel/ptrace.c b/arch/arm/kernel/ptrace.c
>> index ef9119f..fb45cf1 100644
>> --- a/arch/arm/kernel/ptrace.c
>> +++ b/arch/arm/kernel/ptrace.c
>> @@ -59,11 +59,6 @@
>>   #define BREAKINST_THUMB        0xde01
>>   #endif
>>
>> -struct pt_regs_offset {
>> -       const char *name;
>> -       int offset;
>> -};
>> -
>>   #define REG_OFFSET_NAME(r) \
>>          {.name = #r, .offset = offsetof(struct pt_regs, ARM_##r)}
>>   #define REG_OFFSET_END {.name = NULL, .offset = 0}
>
> Can't you also move these? ARM is complicated with the "ARM_"
> prefixing, but the others appear to be the same. Maybe you can remove
> the prefix or redefine the macro for ARM.
>
> Rob
>

That would mandate that all the architecture-specific pt_regs structures 
would have to use a top-level named field for each named register.  That 
seems to me like an unnecessary restriction when the point of 
regs_offset_table is to provide the offset of the register inside an 
arbitarily complex pt_regs struct.  The often redundant definition of 
these two macros doesn't seem to me that high a price for that.

-dl

Re: [PATCH 1/2] Move the pt_regs_offset struct definition from arch to common include file

[Michael Ellerman]

On Wed, 2015-06-17 at 14:30 -0400, David Long wrote:
> On 06/16/15 09:17, Rob Herring wrote:
> > On Mon, Jun 15, 2015 at 11:42 AM, David Long <dave.long@linaro.org> wrote:
> >>
> >>   #define REG_OFFSET_NAME(r) \
> >>          {.name = #r, .offset = offsetof(struct pt_regs, ARM_##r)}
> >>   #define REG_OFFSET_END {.name = NULL, .offset = 0}
> >
> > Can't you also move these? ARM is complicated with the "ARM_"
> > prefixing, but the others appear to be the same. Maybe you can remove
> > the prefix or redefine the macro for ARM.
> 
> That would mandate that all the architecture-specific pt_regs structures 
> would have to use a top-level named field for each named register.

Why does it mandate that?

See eg. powerpc where we use REG_OFFSET_NAME for the top-level named fields and
then a different macro for the array elements:

  #define REG_OFFSET_NAME(r) {.name = #r, .offset = offsetof(struct pt_regs, r)}
  #define GPR_OFFSET_NAME(num)	\
  	{.name = STR(gpr##num), .offset = offsetof(struct pt_regs, gpr[num])}
  
  static const struct pt_regs_offset regoffset_table[] = {
  	GPR_OFFSET_NAME(0),
  	GPR_OFFSET_NAME(1),
  	GPR_OFFSET_NAME(2),
  	GPR_OFFSET_NAME(3),
  	...
  	REG_OFFSET_NAME(nip),
  	REG_OFFSET_NAME(msr),


So I don't see why REG_OFFSET_NAME couldn't be common.

cheers

Re: [PATCH 1/2] Move the pt_regs_offset struct definition from arch to common include file

[David Long]

On 06/29/15 23:29, Michael Ellerman wrote:
> On Wed, 2015-06-17 at 14:30 -0400, David Long wrote:
>> On 06/16/15 09:17, Rob Herring wrote:
>>> On Mon, Jun 15, 2015 at 11:42 AM, David Long <dave.long@linaro.org> wrote:
>>>>
>>>>    #define REG_OFFSET_NAME(r) \
>>>>           {.name = #r, .offset = offsetof(struct pt_regs, ARM_##r)}
>>>>    #define REG_OFFSET_END {.name = NULL, .offset = 0}
>>>
>>> Can't you also move these? ARM is complicated with the "ARM_"
>>> prefixing, but the others appear to be the same. Maybe you can remove
>>> the prefix or redefine the macro for ARM.
>>
>> That would mandate that all the architecture-specific pt_regs structures
>> would have to use a top-level named field for each named register.
>
> Why does it mandate that?
>
> See eg. powerpc where we use REG_OFFSET_NAME for the top-level named fields and
> then a different macro for the array elements:
>
>    #define REG_OFFSET_NAME(r) {.name = #r, .offset = offsetof(struct pt_regs, r)}
>    #define GPR_OFFSET_NAME(num)	\
>    	{.name = STR(gpr##num), .offset = offsetof(struct pt_regs, gpr[num])}
>
>    static const struct pt_regs_offset regoffset_table[] = {
>    	GPR_OFFSET_NAME(0),
>    	GPR_OFFSET_NAME(1),
>    	GPR_OFFSET_NAME(2),
>    	GPR_OFFSET_NAME(3),
>    	...
>    	REG_OFFSET_NAME(nip),
>    	REG_OFFSET_NAME(msr),
>
>
> So I don't see why REG_OFFSET_NAME couldn't be common.
>

Sorry for the delay in responding to this.

OK, so you're saying architectures that don't want this constraint can 
make their own macro.  Seems to make this whole exercise slightly less 
useful, but whatever.

I see three ways to go here:

1) Leave it as is.
2) Force all architectures to use a common definition.
3) Provide a common definition that all architectures (except "arm") 
currently using this functionality will use.

I have a v2 patch to implement #3, ready to post.  Do we think this is 
the way to go? I don't like #2 because I really don't want to rename all 
uses of the current register fields for arm since this is 
architecture-specific code to begin with and since it affects code in 39 
arm source files.

> cheers
>


Thanks,
-dl

Re: [PATCH 1/2] Move the pt_regs_offset struct definition from arch to common include file

[Michael Ellerman]

On Wed, 2015-07-22 at 00:46 -0400, David Long wrote:
> On 06/29/15 23:29, Michael Ellerman wrote:
> > On Wed, 2015-06-17 at 14:30 -0400, David Long wrote:
> >> On 06/16/15 09:17, Rob Herring wrote:
> >>> On Mon, Jun 15, 2015 at 11:42 AM, David Long <dave.long@linaro.org> wrote:
> >>>>
> >>>>    #define REG_OFFSET_NAME(r) \
> >>>>           {.name = #r, .offset = offsetof(struct pt_regs, ARM_##r)}
> >>>>    #define REG_OFFSET_END {.name = NULL, .offset = 0}
> >>>
> >>> Can't you also move these? ARM is complicated with the "ARM_"
> >>> prefixing, but the others appear to be the same. Maybe you can remove
> >>> the prefix or redefine the macro for ARM.
> >>
> >> That would mandate that all the architecture-specific pt_regs structures
> >> would have to use a top-level named field for each named register.
> >
> > Why does it mandate that?
> >
> > See eg. powerpc where we use REG_OFFSET_NAME for the top-level named fields and
> > then a different macro for the array elements:
> >
> >    #define REG_OFFSET_NAME(r) {.name = #r, .offset = offsetof(struct pt_regs, r)}
> >    #define GPR_OFFSET_NAME(num)	\
> >    	{.name = STR(gpr##num), .offset = offsetof(struct pt_regs, gpr[num])}
> >
> >    static const struct pt_regs_offset regoffset_table[] = {
> >    	GPR_OFFSET_NAME(0),
> >    	GPR_OFFSET_NAME(1),
> >    	GPR_OFFSET_NAME(2),
> >    	GPR_OFFSET_NAME(3),
> >    	...
> >    	REG_OFFSET_NAME(nip),
> >    	REG_OFFSET_NAME(msr),
> >
> >
> > So I don't see why REG_OFFSET_NAME couldn't be common.
> >
> 
> Sorry for the delay in responding to this.
> 
> OK, so you're saying architectures that don't want this constraint can 
> make their own macro.  Seems to make this whole exercise slightly less 
> useful, but whatever.

Well yeah.

In fact of the 4 arches that use REG_OFFSET_NAME, 2 already have another macro
for specially named registers (powerpc & sh).

> I see three ways to go here:
> 
> 1) Leave it as is.
> 2) Force all architectures to use a common definition.
> 3) Provide a common definition that all architectures (except "arm") 
> currently using this functionality will use.
> 
> I have a v2 patch to implement #3, ready to post.  Do we think this is 
> the way to go?

Yeah I think it is. How are you making it conditional? Just #ifndef REG_OFFSET_NAME?

> I don't like #2 because I really don't want to rename all 
> uses of the current register fields for arm since this is 
> architecture-specific code to begin with and since it affects code in 39 
> arm source files.

I guess you're talking about renaming all the ARM_x regs to x. That would
likely cause problems because they're implemented as #defines,
eg. #define r0 uregs[0] would probably confuse your assembler.

The clean thing to do would be to have the in-kernel struct pt_regs have actual
named members, but that would still be an intrusive change.

cheers

Re: [PATCH 1/2] Move the pt_regs_offset struct definition from arch to common include file

[David Long]

On 07/22/15 01:11, Michael Ellerman wrote:
> On Wed, 2015-07-22 at 00:46 -0400, David Long wrote:
>> On 06/29/15 23:29, Michael Ellerman wrote:
>>> On Wed, 2015-06-17 at 14:30 -0400, David Long wrote:
>>>> On 06/16/15 09:17, Rob Herring wrote:
>>>>> On Mon, Jun 15, 2015 at 11:42 AM, David Long <dave.long@linaro.org> wrote:
>>>>>>
>>>>>>     #define REG_OFFSET_NAME(r) \
>>>>>>            {.name = #r, .offset = offsetof(struct pt_regs, ARM_##r)}
>>>>>>     #define REG_OFFSET_END {.name = NULL, .offset = 0}
>>>>>
>>>>> Can't you also move these? ARM is complicated with the "ARM_"
>>>>> prefixing, but the others appear to be the same. Maybe you can remove
>>>>> the prefix or redefine the macro for ARM.
>>>>
>>>> That would mandate that all the architecture-specific pt_regs structures
>>>> would have to use a top-level named field for each named register.
>>>
>>> Why does it mandate that?
>>>
>>> See eg. powerpc where we use REG_OFFSET_NAME for the top-level named fields and
>>> then a different macro for the array elements:
>>>
>>>     #define REG_OFFSET_NAME(r) {.name = #r, .offset = offsetof(struct pt_regs, r)}
>>>     #define GPR_OFFSET_NAME(num)	\
>>>     	{.name = STR(gpr##num), .offset = offsetof(struct pt_regs, gpr[num])}
>>>
>>>     static const struct pt_regs_offset regoffset_table[] = {
>>>     	GPR_OFFSET_NAME(0),
>>>     	GPR_OFFSET_NAME(1),
>>>     	GPR_OFFSET_NAME(2),
>>>     	GPR_OFFSET_NAME(3),
>>>     	...
>>>     	REG_OFFSET_NAME(nip),
>>>     	REG_OFFSET_NAME(msr),
>>>
>>>
>>> So I don't see why REG_OFFSET_NAME couldn't be common.
>>>
>>
>> Sorry for the delay in responding to this.
>>
>> OK, so you're saying architectures that don't want this constraint can
>> make their own macro.  Seems to make this whole exercise slightly less
>> useful, but whatever.
>
> Well yeah.
>
> In fact of the 4 arches that use REG_OFFSET_NAME, 2 already have another macro
> for specially named registers (powerpc & sh).
>
>> I see three ways to go here:
>>
>> 1) Leave it as is.
>> 2) Force all architectures to use a common definition.
>> 3) Provide a common definition that all architectures (except "arm")
>> currently using this functionality will use.
>>
>> I have a v2 patch to implement #3, ready to post.  Do we think this is
>> the way to go?
>
> Yeah I think it is. How are you making it conditional? Just #ifndef REG_OFFSET_NAME?
>

I'm just defining a new macro for arm.  The macro is only invoked in one 
arm file.  Then the REG_OFFSET_NAME macro goes unused for this architecture.

>> I don't like #2 because I really don't want to rename all
>> uses of the current register fields for arm since this is
>> architecture-specific code to begin with and since it affects code in 39
>> arm source files.
>
> I guess you're talking about renaming all the ARM_x regs to x. That would
> likely cause problems because they're implemented as #defines,
> eg. #define r0 uregs[0] would probably confuse your assembler.
>

Yeah, and I had not looked further to the implications of doing that but 
I see you've found where it is a genuine problem.

> The clean thing to do would be to have the in-kernel struct pt_regs have actual
> named members, but that would still be an intrusive change.
>
> cheers
>
>

Thanks,
-dl

Re: [PATCH 1/2] Move the pt_regs_offset struct definition from arch to common include file

[Michael Ellerman]

On Mon, 2015-06-15 at 12:42 -0400, David Long wrote:
> From: "David A. Long" <dave.long@linaro.org>
> 
> The pt_regs_offset structure is used for HAVE_REGS_AND_STACK_ACCESS_API
>  feature and has identical definitions in four different arch ptrace.h
> include files. It seems unlikely that definition would ever need to be
> changed regardless of architecture so lets move it into
> include/linux/ptrace.h.
> 
> Signed-off-by: David A. Long <dave.long@linaro.org>
> ---
>  arch/powerpc/kernel/ptrace.c | 5 -----

Built and booted on powerpc, but is there an easy way to actually test the code
paths in question?

Acked-by: Michael Ellerman <mpe@ellerman.id.au>

cheers

Re: [PATCH 1/2] Move the pt_regs_offset struct definition from arch to common include file

[David Long]

On 06/19/15 00:19, Michael Ellerman wrote:
> On Mon, 2015-06-15 at 12:42 -0400, David Long wrote:
>> From: "David A. Long" <dave.long@linaro.org>
>>
>> The pt_regs_offset structure is used for HAVE_REGS_AND_STACK_ACCESS_API
>>   feature and has identical definitions in four different arch ptrace.h
>> include files. It seems unlikely that definition would ever need to be
>> changed regardless of architecture so lets move it into
>> include/linux/ptrace.h.
>>
>> Signed-off-by: David A. Long <dave.long@linaro.org>
>> ---
>>   arch/powerpc/kernel/ptrace.c | 5 -----
>
> Built and booted on powerpc, but is there an easy way to actually test the code
> paths in question?
>

There is an easy way to "smoke test" it on all archiectures that also 
implement kprobes (which powerpc does).  If I'm understanding the 
powerpc code correctly (WRT register naming conventions) just do the 
following:

cd /sys/kernel/debug/tracing
echo 'p do_fork %gpr0' > kprobe_events
echo 1 > events/kprobes/enable
ls
cat trace
echo 0 > events/kprobes/enable

Every fork() call done on the system between those two echo commands 
(hence the "ls") should append a line to the trace file.  For a more 
exhaustive test one could repeat this sequence for every register in the 
architecture.

This should work the same on all architectures supporting kprobes.  You 
just have to use the appropriate register names for your architecture 
after the "%".

> Acked-by: Michael Ellerman <mpe@ellerman.id.au>
>
> cheers
>
>

Thanks,
-dl

Re: [PATCH 1/2] Move the pt_regs_offset struct definition from arch to common include file

[Kees Cook]

On Fri, Jun 19, 2015 at 7:12 AM, David Long <dave.long@linaro.org> wrote:
> On 06/19/15 00:19, Michael Ellerman wrote:
>>
>> On Mon, 2015-06-15 at 12:42 -0400, David Long wrote:
>>>
>>> From: "David A. Long" <dave.long@linaro.org>
>>>
>>> The pt_regs_offset structure is used for HAVE_REGS_AND_STACK_ACCESS_API
>>>   feature and has identical definitions in four different arch ptrace.h
>>> include files. It seems unlikely that definition would ever need to be
>>> changed regardless of architecture so lets move it into
>>> include/linux/ptrace.h.
>>>
>>> Signed-off-by: David A. Long <dave.long@linaro.org>
>>> ---
>>>   arch/powerpc/kernel/ptrace.c | 5 -----
>>
>>
>> Built and booted on powerpc, but is there an easy way to actually test the
>> code
>> paths in question?
>>
>
> There is an easy way to "smoke test" it on all archiectures that also
> implement kprobes (which powerpc does).  If I'm understanding the powerpc
> code correctly (WRT register naming conventions) just do the following:
>
> cd /sys/kernel/debug/tracing
> echo 'p do_fork %gpr0' > kprobe_events
> echo 1 > events/kprobes/enable
> ls
> cat trace
> echo 0 > events/kprobes/enable
>
> Every fork() call done on the system between those two echo commands (hence
> the "ls") should append a line to the trace file.  For a more exhaustive
> test one could repeat this sequence for every register in the architecture.
>
> This should work the same on all architectures supporting kprobes.  You just
> have to use the appropriate register names for your architecture after the
> "%".

Is this something we could codify into the selftests directory? It
seems like a great thing to capture in a single place somewhere (the
register lists, that is).

-Kees

>
>> Acked-by: Michael Ellerman <mpe@ellerman.id.au>
>>
>> cheers
>>
>>
>
> Thanks,
> -dl
>



-- 
Kees Cook
Chrome OS Security

Re: [PATCH 1/2] Move the pt_regs_offset struct definition from arch to common include file

[David Long]

On 06/19/15 12:58, Kees Cook wrote:
> On Fri, Jun 19, 2015 at 7:12 AM, David Long <dave.long@linaro.org> wrote:
>> On 06/19/15 00:19, Michael Ellerman wrote:
>>>
>>> On Mon, 2015-06-15 at 12:42 -0400, David Long wrote:
>>>>
>>>> From: "David A. Long" <dave.long@linaro.org>
>>>>
>>>> The pt_regs_offset structure is used for HAVE_REGS_AND_STACK_ACCESS_API
>>>>    feature and has identical definitions in four different arch ptrace.h
>>>> include files. It seems unlikely that definition would ever need to be
>>>> changed regardless of architecture so lets move it into
>>>> include/linux/ptrace.h.
>>>>
>>>> Signed-off-by: David A. Long <dave.long@linaro.org>
>>>> ---
>>>>    arch/powerpc/kernel/ptrace.c | 5 -----
>>>
>>>
>>> Built and booted on powerpc, but is there an easy way to actually test the
>>> code
>>> paths in question?
>>>
>>
>> There is an easy way to "smoke test" it on all archiectures that also
>> implement kprobes (which powerpc does).  If I'm understanding the powerpc
>> code correctly (WRT register naming conventions) just do the following:
>>
>> cd /sys/kernel/debug/tracing
>> echo 'p do_fork %gpr0' > kprobe_events
>> echo 1 > events/kprobes/enable
>> ls
>> cat trace
>> echo 0 > events/kprobes/enable
>>
>> Every fork() call done on the system between those two echo commands (hence
>> the "ls") should append a line to the trace file.  For a more exhaustive
>> test one could repeat this sequence for every register in the architecture.
>>
>> This should work the same on all architectures supporting kprobes.  You just
>> have to use the appropriate register names for your architecture after the
>> "%".
>
> Is this something we could codify into the selftests directory? It
> seems like a great thing to capture in a single place somewhere (the
> register lists, that is).
> e
> -Kees
>

Due to the architecture-specific naming of registers this would have to 
be added to the architecture subdirectories.  I only see powerpc and x86 
subdirs at this time so extending that infrastructure would have to be 
part of this.  Verifying the register contents would also require some 
change to the kernel, possibly a simple test module, which would have to 
be unique to each architecture.  Without that we could only check for 
recognition of the register name, although maybe that's good enough.

>>
>>> Acked-by: Michael Ellerman <mpe@ellerman.id.au>
>>>
>>> cheers
>>>
>>>
>>
>> Thanks,
>> -dl
>>


-dl

Re: [PATCH 1/2] Move the pt_regs_offset struct definition from arch to common include file

[Michael Ellerman]

On Fri, 2015-06-19 at 10:12 -0400, David Long wrote:
> On 06/19/15 00:19, Michael Ellerman wrote:
> > On Mon, 2015-06-15 at 12:42 -0400, David Long wrote:
> >> From: "David A. Long" <dave.long@linaro.org>
> >>
> >> The pt_regs_offset structure is used for HAVE_REGS_AND_STACK_ACCESS_API
> >>   feature and has identical definitions in four different arch ptrace.h
> >> include files. It seems unlikely that definition would ever need to be
> >> changed regardless of architecture so lets move it into
> >> include/linux/ptrace.h.
> >>
> >> Signed-off-by: David A. Long <dave.long@linaro.org>
> >> ---
> >>   arch/powerpc/kernel/ptrace.c | 5 -----
> >
> > Built and booted on powerpc, but is there an easy way to actually test the code
> > paths in question?
> >
> 
> There is an easy way to "smoke test" it on all archiectures that also 
> implement kprobes (which powerpc does).  If I'm understanding the 
> powerpc code correctly (WRT register naming conventions) just do the 
> following:
> 
> cd /sys/kernel/debug/tracing
> echo 'p do_fork %gpr0' > kprobe_events
> echo 1 > events/kprobes/enable
> ls
> cat trace
> echo 0 > events/kprobes/enable
> 
> Every fork() call done on the system between those two echo commands 
> (hence the "ls") should append a line to the trace file.  For a more 
> exhaustive test one could repeat this sequence for every register in the 
> architecture.

OK, so I went the whole hog and did:

$ echo 'p do_fork %gpr0 %gpr1 %gpr2 %gpr3 %gpr4 %gpr5 %gpr6 %gpr7 %gpr8 %gpr9 %gpr10 %gpr11 %gpr12 %gpr13 %gpr14 %gpr15 %gpr16 %gpr17 %gpr18 %gpr19 %gpr20 %gpr21 %gpr22 %gpr23 %gpr24 %gpr25 %gpr26 %gpr27 %gpr28 %gpr29 %gpr30 %gpr31 %nip %msr %ctr %link %xer %ccr %softe %trap %dar %dsisr' > kprobe_events

And I get:

            bash-2057  [001] d...   535.433941: p_do_fork_0: (do_fork+0x8/0x490) arg1=0xc0000000000094d0 arg2=0xc0000001fbe9be30 arg3=0xc000000001133bb8 arg4=0x1200011 arg5=0x0 arg6=0x0 arg7=0x0 arg8=0x3fff7c885940 arg9=0x1 arg10=0xc0000001fbe9bea0 arg11=0x0 arg12=0xc01 arg13=0xc0000000000094c8 arg14=0xc00000000fdc0480 arg15=0x0 arg16=0x22000000 arg17=0x1016d6e8 arg18=0x0 arg19=0x44000000 arg20=0x0 arg21=0x10037c82208 arg22=0x1017b008 arg23=0x10143d18 arg24=0x10178854 arg25=0x10144f90 arg26=0x10037c821e8 arg27=0x0 arg28=0x0 arg29=0x0 arg30=0x0 arg31=0x809 arg32=0x3ffff788c010 arg33=0xc0000000000a7fe8 arg34=0x8000000000029033 arg35=0xc0000000000094c8 arg36=0xc0000000000094d0 arg37=0x0 arg38=0x42222844 arg39=0x1 arg40=0x700 arg41=0xc0000001fbe9bd50 arg42=0xc0000001fbe9bd30

Which is ugly as hell, but appears unchanged since before your patch.

I take it it's expected that the names are not decoded in the output?

Also I wonder why we choose "gpr" when "r" is the more usual prefix on powerpc.
I guess we can add new aliases to the table.

cheers

Re: [PATCH 1/2] Move the pt_regs_offset struct definition from arch to common include file

[David Long]

On 06/22/15 23:32, Michael Ellerman wrote:
> On Fri, 2015-06-19 at 10:12 -0400, David Long wrote:
>> On 06/19/15 00:19, Michael Ellerman wrote:
>>> On Mon, 2015-06-15 at 12:42 -0400, David Long wrote:
>>>> From: "David A. Long" <dave.long@linaro.org>
>>>>
>>>> The pt_regs_offset structure is used for HAVE_REGS_AND_STACK_ACCESS_API
>>>>    feature and has identical definitions in four different arch ptrace.h
>>>> include files. It seems unlikely that definition would ever need to be
>>>> changed regardless of architecture so lets move it into
>>>> include/linux/ptrace.h.
>>>>
>>>> Signed-off-by: David A. Long <dave.long@linaro.org>
>>>> ---
>>>>    arch/powerpc/kernel/ptrace.c | 5 -----
>>>
>>> Built and booted on powerpc, but is there an easy way to actually test the code
>>> paths in question?
>>>
>>
>> There is an easy way to "smoke test" it on all archiectures that also
>> implement kprobes (which powerpc does).  If I'm understanding the
>> powerpc code correctly (WRT register naming conventions) just do the
>> following:
>>
>> cd /sys/kernel/debug/tracing
>> echo 'p do_fork %gpr0' > kprobe_events
>> echo 1 > events/kprobes/enable
>> ls
>> cat trace
>> echo 0 > events/kprobes/enable
>>
>> Every fork() call done on the system between those two echo commands
>> (hence the "ls") should append a line to the trace file.  For a more
>> exhaustive test one could repeat this sequence for every register in the
>> architecture.
>
> OK, so I went the whole hog and did:
>
> $ echo 'p do_fork %gpr0 %gpr1 %gpr2 %gpr3 %gpr4 %gpr5 %gpr6 %gpr7 %gpr8 %gpr9 %gpr10 %gpr11 %gpr12 %gpr13 %gpr14 %gpr15 %gpr16 %gpr17 %gpr18 %gpr19 %gpr20 %gpr21 %gpr22 %gpr23 %gpr24 %gpr25 %gpr26 %gpr27 %gpr28 %gpr29 %gpr30 %gpr31 %nip %msr %ctr %link %xer %ccr %softe %trap %dar %dsisr' > kprobe_events
>
> And I get:
>
>              bash-2057  [001] d...   535.433941: p_do_fork_0: (do_fork+0x8/0x490) arg1=0xc0000000000094d0 arg2=0xc0000001fbe9be30 arg3=0xc000000001133bb8 arg4=0x1200011 arg5=0x0 arg6=0x0 arg7=0x0 arg8=0x3fff7c885940 arg9=0x1 arg10=0xc0000001fbe9bea0 arg11=0x0 arg12=0xc01 arg13=0xc0000000000094c8 arg14=0xc00000000fdc0480 arg15=0x0 arg16=0x22000000 arg17=0x1016d6e8 arg18=0x0 arg19=0x44000000 arg20=0x0 arg21=0x10037c82208 arg22=0x1017b008 arg23=0x10143d18 arg24=0x10178854 arg25=0x10144f90 arg26=0x10037c821e8 arg27=0x0 arg28=0x0 arg29=0x0 arg30=0x0 arg31=0x809 arg32=0x3ffff788c010 arg33=0xc0000000000a7fe8 arg34=0x8000000000029033 arg35=0xc0000000000094c8 arg36=0xc0000000000094d0 arg37=0x0 arg38=0x42222844 arg39=0x1 arg40=0x700 arg41=0xc0000001fbe9bd50 arg42=0xc0000001fbe9bd30
>
> Which is ugly as hell, but appears unchanged since before your patch.
>

Excellent.  Many thanks.

> I take it it's expected that the names are not decoded in the output?
>

Yes.

> Also I wonder why we choose "gpr" when "r" is the more usual prefix on powerpc.
> I guess we can add new aliases to the table.
>

Yeah I can't answer that, this is just what the preexisting code is 
written to do. I believe you could add aliases to the table, perhaps as 
a step in migrating to supporting only the more common naming.  The 
reverse translation would have to return one or the other though.

-dl

Re: [PATCH 1/2] Move the pt_regs_offset struct definition from arch to common include file

[Michael Ellerman]

On Tue, 2015-06-23 at 09:48 -0400, David Long wrote:
> On 06/22/15 23:32, Michael Ellerman wrote:
> > On Fri, 2015-06-19 at 10:12 -0400, David Long wrote:
> >> On 06/19/15 00:19, Michael Ellerman wrote:
> >>> On Mon, 2015-06-15 at 12:42 -0400, David Long wrote:
> >>>> From: "David A. Long" <dave.long@linaro.org>
> >>>>
> >>>> The pt_regs_offset structure is used for HAVE_REGS_AND_STACK_ACCESS_API
> >>>>    feature and has identical definitions in four different arch ptrace.h
> >>>> include files. It seems unlikely that definition would ever need to be
> >>>> changed regardless of architecture so lets move it into
> >>>> include/linux/ptrace.h.
> >>>>
> >>>> Signed-off-by: David A. Long <dave.long@linaro.org>
> >>>> ---
> >>>>    arch/powerpc/kernel/ptrace.c | 5 -----
> >>>
> >>> Built and booted on powerpc, but is there an easy way to actually test the code
> >>> paths in question?
> >>
> >> There is an easy way to "smoke test" it on all archiectures that also
> >> implement kprobes (which powerpc does).  If I'm understanding the
> >> powerpc code correctly (WRT register naming conventions) just do the
> >> following:
> >>
> >> cd /sys/kernel/debug/tracing
> >> echo 'p do_fork %gpr0' > kprobe_events
> >> echo 1 > events/kprobes/enable
> >> ls
> >> cat trace
> >> echo 0 > events/kprobes/enable
> >>
> >> Every fork() call done on the system between those two echo commands
> >> (hence the "ls") should append a line to the trace file.  For a more
> >> exhaustive test one could repeat this sequence for every register in the
> >> architecture.
> >
> > OK, so I went the whole hog and did:
> >
> > $ echo 'p do_fork %gpr0 %gpr1 %gpr2 %gpr3 %gpr4 %gpr5 %gpr6 %gpr7 %gpr8 %gpr9 %gpr10 %gpr11 %gpr12 %gpr13 %gpr14 %gpr15 %gpr16 %gpr17 %gpr18 %gpr19 %gpr20 %gpr21 %gpr22 %gpr23 %gpr24 %gpr25 %gpr26 %gpr27 %gpr28 %gpr29 %gpr30 %gpr31 %nip %msr %ctr %link %xer %ccr %softe %trap %dar %dsisr' > kprobe_events
> >
> > And I get:
> >
> >              bash-2057  [001] d...   535.433941: p_do_fork_0: (do_fork+0x8/0x490) arg1=0xc0000000000094d0 arg2=0xc0000001fbe9be30 arg3=0xc000000001133bb8 arg4=0x1200011 arg5=0x0 arg6=0x0 arg7=0x0 arg8=0x3fff7c885940 arg9=0x1 arg10=0xc0000001fbe9bea0 arg11=0x0 arg12=0xc01 arg13=0xc0000000000094c8 arg14=0xc00000000fdc0480 arg15=0x0 arg16=0x22000000 arg17=0x1016d6e8 arg18=0x0 arg19=0x44000000 arg20=0x0 arg21=0x10037c82208 arg22=0x1017b008 arg23=0x10143d18 arg24=0x10178854 arg25=0x10144f90 arg26=0x10037c821e8 arg27=0x0 arg28=0x0 arg29=0x0 arg30=0x0 arg31=0x809 arg32=0x3ffff788c010 arg33=0xc0000000000a7fe8 arg34=0x8000000000029033 arg35=0xc0000000000094c8 arg36=0xc0000000000094d0 arg37=0x0 arg38=0x42222844 arg39=0x1 arg40=0x700 arg41=0xc0000001fbe9bd50 arg42=0xc0000001fbe9bd30
> >
> > Which is ugly as hell, but appears unchanged since before your patch.
> >
> 
> Excellent.  Many thanks.

No worries.

Did I already send you an ack? Have another one in case:

Acked-by: Michael Ellerman <mpe@ellerman.id.au>


> > I take it it's expected that the names are not decoded in the output?
>
> Yes.

In fact I don't see anywhere that uses the reverse decoding, ie.
regs_query_register_name().

cheers

Re: [PATCH 1/2] Move the pt_regs_offset struct definition from arch to common include file

[David Long]

On 06/24/15 00:07, Michael Ellerman wrote:
> On Tue, 2015-06-23 at 09:48 -0400, David Long wrote:
>> On 06/22/15 23:32, Michael Ellerman wrote:
>>> On Fri, 2015-06-19 at 10:12 -0400, David Long wrote:
>>>> On 06/19/15 00:19, Michael Ellerman wrote:
>>>>> On Mon, 2015-06-15 at 12:42 -0400, David Long wrote:
>>>>>> From: "David A. Long" <dave.long@linaro.org>
>>>>>>
>>>>>> The pt_regs_offset structure is used for HAVE_REGS_AND_STACK_ACCESS_API
>>>>>>     feature and has identical definitions in four different arch ptrace.h
>>>>>> include files. It seems unlikely that definition would ever need to be
>>>>>> changed regardless of architecture so lets move it into
>>>>>> include/linux/ptrace.h.
>>>>>>
>>>>>> Signed-off-by: David A. Long <dave.long@linaro.org>
>>>>>> ---
>>>>>>     arch/powerpc/kernel/ptrace.c | 5 -----
>>>>>
>>>>> Built and booted on powerpc, but is there an easy way to actually test the code
>>>>> paths in question?
>>>>
>>>> There is an easy way to "smoke test" it on all archiectures that also
>>>> implement kprobes (which powerpc does).  If I'm understanding the
>>>> powerpc code correctly (WRT register naming conventions) just do the
>>>> following:
>>>>
>>>> cd /sys/kernel/debug/tracing
>>>> echo 'p do_fork %gpr0' > kprobe_events
>>>> echo 1 > events/kprobes/enable
>>>> ls
>>>> cat trace
>>>> echo 0 > events/kprobes/enable
>>>>
>>>> Every fork() call done on the system between those two echo commands
>>>> (hence the "ls") should append a line to the trace file.  For a more
>>>> exhaustive test one could repeat this sequence for every register in the
>>>> architecture.
>>>
>>> OK, so I went the whole hog and did:
>>>
>>> $ echo 'p do_fork %gpr0 %gpr1 %gpr2 %gpr3 %gpr4 %gpr5 %gpr6 %gpr7 %gpr8 %gpr9 %gpr10 %gpr11 %gpr12 %gpr13 %gpr14 %gpr15 %gpr16 %gpr17 %gpr18 %gpr19 %gpr20 %gpr21 %gpr22 %gpr23 %gpr24 %gpr25 %gpr26 %gpr27 %gpr28 %gpr29 %gpr30 %gpr31 %nip %msr %ctr %link %xer %ccr %softe %trap %dar %dsisr' > kprobe_events
>>>
>>> And I get:
>>>
>>>               bash-2057  [001] d...   535.433941: p_do_fork_0: (do_fork+0x8/0x490) arg1=0xc0000000000094d0 arg2=0xc0000001fbe9be30 arg3=0xc000000001133bb8 arg4=0x1200011 arg5=0x0 arg6=0x0 arg7=0x0 arg8=0x3fff7c885940 arg9=0x1 arg10=0xc0000001fbe9bea0 arg11=0x0 arg12=0xc01 arg13=0xc0000000000094c8 arg14=0xc00000000fdc0480 arg15=0x0 arg16=0x22000000 arg17=0x1016d6e8 arg18=0x0 arg19=0x44000000 arg20=0x0 arg21=0x10037c82208 arg22=0x1017b008 arg23=0x10143d18 arg24=0x10178854 arg25=0x10144f90 arg26=0x10037c821e8 arg27=0x0 arg28=0x0 arg29=0x0 arg30=0x0 arg31=0x809 arg32=0x3ffff788c010 arg33=0xc0000000000a7fe8 arg34=0x8000000000029033 arg35=0xc0000000000094c8 arg36=0xc0000000000094d0 arg37=0x0 arg38=0x42222844 arg39=0x1 arg40=0x700 arg41=0xc0000001fbe9bd50 arg42=0xc0000001fbe9bd30
>>>
>>> Which is ugly as hell, but appears unchanged since before your patch.
>>>
>>
>> Excellent.  Many thanks.
>
> No worries.
>
> Did I already send you an ack? Have another one in case:
>
> Acked-by: Michael Ellerman <mpe@ellerman.id.au>
>
>

Thanks.

>>> I take it it's expected that the names are not decoded in the output?
>>
>> Yes.
>
> In fact I don't see anywhere that uses the reverse decoding, ie.
> regs_query_register_name().
>

Neither did I.  I assumed it was intended to support either future 
kernel code or custom debug modules.

> cheers
>
>

-dl

[PATCH 2/2] Consolidate redundant register/stack access code

[David Long]

From: "David A. Long" <dave.long@linaro.org>

Several architectures have identical or functionally equivalent code
implementing parts of the HAVE_REGS_AND_STACK_ACCESS_API feature.  Move
that code out of the architecture directories.

Signed-off-by: David A. Long <dave.long@linaro.org>
---
 arch/arm/include/asm/ptrace.h          |  6 ---
 arch/arm/kernel/ptrace.c               | 67 +-------------------------------
 arch/hexagon/include/uapi/asm/ptrace.h |  3 --
 arch/powerpc/include/asm/ptrace.h      | 38 ------------------
 arch/powerpc/kernel/ptrace.c           | 34 +----------------
 arch/s390/include/asm/ptrace.h         |  3 --
 arch/s390/kernel/ptrace.c              | 70 ++++++++++------------------------
 arch/sh/include/asm/ptrace.h           | 39 -------------------
 arch/sh/kernel/Makefile                |  2 +-
 arch/sh/kernel/ptrace.c                | 33 ----------------
 arch/sh/kernel/ptrace_32.c             |  2 +-
 arch/sh/kernel/ptrace_64.c             |  2 +-
 arch/x86/include/asm/ptrace.h          | 37 ------------------
 arch/x86/kernel/ptrace.c               | 34 +----------------
 include/linux/ptrace.h                 | 42 ++++++++++++++++++++
 kernel/ptrace.c                        | 38 ++++++++++++++++++
 16 files changed, 107 insertions(+), 343 deletions(-)
 delete mode 100644 arch/sh/kernel/ptrace.c

diff --git a/arch/arm/include/asm/ptrace.h b/arch/arm/include/asm/ptrace.h
index 51622ba..84a0ea4 100644
--- a/arch/arm/include/asm/ptrace.h
+++ b/arch/arm/include/asm/ptrace.h
@@ -120,12 +120,6 @@ extern unsigned long profile_pc(struct pt_regs *regs);
 #include <linux/types.h>
 #define MAX_REG_OFFSET (offsetof(struct pt_regs, ARM_ORIG_r0))
 
-extern int regs_query_register_offset(const char *name);
-extern const char *regs_query_register_name(unsigned int offset);
-extern bool regs_within_kernel_stack(struct pt_regs *regs, unsigned long addr);
-extern unsigned long regs_get_kernel_stack_nth(struct pt_regs *regs,
-					       unsigned int n);
-
 /**
  * regs_get_register() - get register value from its offset
  * @regs:	   pt_regs from which register value is gotten
diff --git a/arch/arm/kernel/ptrace.c b/arch/arm/kernel/ptrace.c
index fb45cf1..d63ad99 100644
--- a/arch/arm/kernel/ptrace.c
+++ b/arch/arm/kernel/ptrace.c
@@ -63,7 +63,7 @@
 	{.name = #r, .offset = offsetof(struct pt_regs, ARM_##r)}
 #define REG_OFFSET_END {.name = NULL, .offset = 0}
 
-static const struct pt_regs_offset regoffset_table[] = {
+const struct pt_regs_offset regs_offset_table[] = {
 	REG_OFFSET_NAME(r0),
 	REG_OFFSET_NAME(r1),
 	REG_OFFSET_NAME(r2),
@@ -85,71 +85,6 @@ static const struct pt_regs_offset regoffset_table[] = {
 	REG_OFFSET_END,
 };
 
-/**
- * regs_query_register_offset() - query register offset from its name
- * @name:	the name of a register
- *
- * regs_query_register_offset() returns the offset of a register in struct
- * pt_regs from its name. If the name is invalid, this returns -EINVAL;
- */
-int regs_query_register_offset(const char *name)
-{
-	const struct pt_regs_offset *roff;
-	for (roff = regoffset_table; roff->name != NULL; roff++)
-		if (!strcmp(roff->name, name))
-			return roff->offset;
-	return -EINVAL;
-}
-
-/**
- * regs_query_register_name() - query register name from its offset
- * @offset:	the offset of a register in struct pt_regs.
- *
- * regs_query_register_name() returns the name of a register from its
- * offset in struct pt_regs. If the @offset is invalid, this returns NULL;
- */
-const char *regs_query_register_name(unsigned int offset)
-{
-	const struct pt_regs_offset *roff;
-	for (roff = regoffset_table; roff->name != NULL; roff++)
-		if (roff->offset == offset)
-			return roff->name;
-	return NULL;
-}
-
-/**
- * regs_within_kernel_stack() - check the address in the stack
- * @regs:      pt_regs which contains kernel stack pointer.
- * @addr:      address which is checked.
- *
- * regs_within_kernel_stack() checks @addr is within the kernel stack page(s).
- * If @addr is within the kernel stack, it returns true. If not, returns false.
- */
-bool regs_within_kernel_stack(struct pt_regs *regs, unsigned long addr)
-{
-	return ((addr & ~(THREAD_SIZE - 1))  ==
-		(kernel_stack_pointer(regs) & ~(THREAD_SIZE - 1)));
-}
-
-/**
- * regs_get_kernel_stack_nth() - get Nth entry of the stack
- * @regs:	pt_regs which contains kernel stack pointer.
- * @n:		stack entry number.
- *
- * regs_get_kernel_stack_nth() returns @n th entry of the kernel stack which
- * is specified by @regs. If the @n th entry is NOT in the kernel stack,
- * this returns 0.
- */
-unsigned long regs_get_kernel_stack_nth(struct pt_regs *regs, unsigned int n)
-{
-	unsigned long *addr = (unsigned long *)kernel_stack_pointer(regs);
-	addr += n;
-	if (regs_within_kernel_stack(regs, (unsigned long)addr))
-		return *addr;
-	else
-		return 0;
-}
-
 /*
  * this routine will get a word off of the processes privileged stack.
  * the offset is how far from the base addr as stored in the THREAD.
diff --git a/arch/hexagon/include/uapi/asm/ptrace.h b/arch/hexagon/include/uapi/asm/ptrace.h
index 065e5b3..0afb664 100644
--- a/arch/hexagon/include/uapi/asm/ptrace.h
+++ b/arch/hexagon/include/uapi/asm/ptrace.h
@@ -29,9 +29,6 @@
 #define profile_pc(regs) instruction_pointer(regs)
 
 /* kprobe-based event tracer support */
-extern int regs_query_register_offset(const char *name);
-extern const char *regs_query_register_name(unsigned int offset);
-
 #define current_pt_regs() \
 	((struct pt_regs *) \
 	 ((unsigned long)current_thread_info() + THREAD_SIZE) - 1)
diff --git a/arch/powerpc/include/asm/ptrace.h b/arch/powerpc/include/asm/ptrace.h
index c0c61fa..64b9b3d 100644
--- a/arch/powerpc/include/asm/ptrace.h
+++ b/arch/powerpc/include/asm/ptrace.h
@@ -156,8 +156,6 @@ do {									      \
 
 #include <linux/stddef.h>
 #include <linux/thread_info.h>
-extern int regs_query_register_offset(const char *name);
-extern const char *regs_query_register_name(unsigned int offset);
 #define MAX_REG_OFFSET (offsetof(struct pt_regs, dsisr))
 
 /**
@@ -177,42 +175,6 @@ static inline unsigned long regs_get_register(struct pt_regs *regs,
 	return *(unsigned long *)((unsigned long)regs + offset);
 }
 
-/**
- * regs_within_kernel_stack() - check the address in the stack
- * @regs:      pt_regs which contains kernel stack pointer.
- * @addr:      address which is checked.
- *
- * regs_within_kernel_stack() checks @addr is within the kernel stack page(s).
- * If @addr is within the kernel stack, it returns true. If not, returns false.
- */
-
-static inline bool regs_within_kernel_stack(struct pt_regs *regs,
-						unsigned long addr)
-{
-	return ((addr & ~(THREAD_SIZE - 1))  ==
-		(kernel_stack_pointer(regs) & ~(THREAD_SIZE - 1)));
-}
-
-/**
- * regs_get_kernel_stack_nth() - get Nth entry of the stack
- * @regs:	pt_regs which contains kernel stack pointer.
- * @n:		stack entry number.
- *
- * regs_get_kernel_stack_nth() returns @n th entry of the kernel stack which
- * is specified by @regs. If the @n th entry is NOT in the kernel stack,
- * this returns 0.
- */
-static inline unsigned long regs_get_kernel_stack_nth(struct pt_regs *regs,
-						      unsigned int n)
-{
-	unsigned long *addr = (unsigned long *)kernel_stack_pointer(regs);
-	addr += n;
-	if (regs_within_kernel_stack(regs, (unsigned long)addr))
-		return *addr;
-	else
-		return 0;
-}
-
 #endif /* __ASSEMBLY__ */
 
 #ifndef __powerpc64__
diff --git a/arch/powerpc/kernel/ptrace.c b/arch/powerpc/kernel/ptrace.c
index ab81733..e35c731 100644
--- a/arch/powerpc/kernel/ptrace.c
+++ b/arch/powerpc/kernel/ptrace.c
@@ -58,7 +58,7 @@
 	{.name = STR(gpr##num), .offset = offsetof(struct pt_regs, gpr[num])}
 #define REG_OFFSET_END {.name = NULL, .offset = 0}
 
-static const struct pt_regs_offset regoffset_table[] = {
+const struct pt_regs_offset regs_offset_table[] = {
 	GPR_OFFSET_NAME(0),
 	GPR_OFFSET_NAME(1),
 	GPR_OFFSET_NAME(2),
@@ -108,38 +108,6 @@ static const struct pt_regs_offset regoffset_table[] = {
 	REG_OFFSET_END,
 };
 
-/**
- * regs_query_register_offset() - query register offset from its name
- * @name:	the name of a register
- *
- * regs_query_register_offset() returns the offset of a register in struct
- * pt_regs from its name. If the name is invalid, this returns -EINVAL;
- */
-int regs_query_register_offset(const char *name)
-{
-	const struct pt_regs_offset *roff;
-	for (roff = regoffset_table; roff->name != NULL; roff++)
-		if (!strcmp(roff->name, name))
-			return roff->offset;
-	return -EINVAL;
-}
-
-/**
- * regs_query_register_name() - query register name from its offset
- * @offset:	the offset of a register in struct pt_regs.
- *
- * regs_query_register_name() returns the name of a register from its
- * offset in struct pt_regs. If the @offset is invalid, this returns NULL;
- */
-const char *regs_query_register_name(unsigned int offset)
-{
-	const struct pt_regs_offset *roff;
-	for (roff = regoffset_table; roff->name != NULL; roff++)
-		if (roff->offset == offset)
-			return roff->name;
-	return NULL;
-}
-
 /*
  * does not yet catch signals sent when the child dies.
  * in exit.c or in signal.c.
diff --git a/arch/s390/include/asm/ptrace.h b/arch/s390/include/asm/ptrace.h
index 6feda25..d4f9ad3 100644
--- a/arch/s390/include/asm/ptrace.h
+++ b/arch/s390/include/asm/ptrace.h
@@ -163,10 +163,7 @@ static inline void instruction_pointer_set(struct pt_regs *regs,
 	regs->psw.addr = val | PSW_ADDR_AMODE;
 }
 
-int regs_query_register_offset(const char *name);
-const char *regs_query_register_name(unsigned int offset);
 unsigned long regs_get_register(struct pt_regs *regs, unsigned int offset);
-unsigned long regs_get_kernel_stack_nth(struct pt_regs *regs, unsigned int n);
 
 static inline unsigned long kernel_stack_pointer(struct pt_regs *regs)
 {
diff --git a/arch/s390/kernel/ptrace.c b/arch/s390/kernel/ptrace.c
index d363c9c..a7eb15f 100644
--- a/arch/s390/kernel/ptrace.c
+++ b/arch/s390/kernel/ptrace.c
@@ -1474,9 +1474,27 @@ const struct user_regset_view *task_user_regset_view(struct task_struct *task)
 	return &user_s390_view;
 }
 
-static const char *gpr_names[NUM_GPRS] = {
-	"r0", "r1",  "r2",  "r3",  "r4",  "r5",  "r6",  "r7",
-	"r8", "r9", "r10", "r11", "r12", "r13", "r14", "r15",
+#define REG_OFFSET_NAME(r) {.name = "r" #r, .offset = r}
+#define REG_OFFSET_END {.name = NULL, .offset = 0}
+
+const struct pt_regs_offset regs_offset_table[NUM_GPRS+1] = {
+	REG_OFFSET_NAME(0),
+	REG_OFFSET_NAME(1),
+	REG_OFFSET_NAME(2),
+	REG_OFFSET_NAME(3),
+	REG_OFFSET_NAME(4),
+	REG_OFFSET_NAME(5),
+	REG_OFFSET_NAME(6),
+	REG_OFFSET_NAME(7),
+	REG_OFFSET_NAME(8),
+	REG_OFFSET_NAME(9),
+	REG_OFFSET_NAME(10),
+	REG_OFFSET_NAME(11),
+	REG_OFFSET_NAME(12),
+	REG_OFFSET_NAME(13),
+	REG_OFFSET_NAME(14),
+	REG_OFFSET_NAME(15),
+	REG_OFFSET_END
 };
 
 unsigned long regs_get_register(struct pt_regs *regs, unsigned int offset)
@@ -1485,49 +1503,3 @@ unsigned long regs_get_register(struct pt_regs *regs, unsigned int offset)
 		return 0;
 	return regs->gprs[offset];
 }
-
-int regs_query_register_offset(const char *name)
-{
-	unsigned long offset;
-
-	if (!name || *name != 'r')
-		return -EINVAL;
-	if (kstrtoul(name + 1, 10, &offset))
-		return -EINVAL;
-	if (offset >= NUM_GPRS)
-		return -EINVAL;
-	return offset;
-}
-
-const char *regs_query_register_name(unsigned int offset)
-{
-	if (offset >= NUM_GPRS)
-		return NULL;
-	return gpr_names[offset];
-}
-
-static int regs_within_kernel_stack(struct pt_regs *regs, unsigned long addr)
-{
-	unsigned long ksp = kernel_stack_pointer(regs);
-
-	return (addr & ~(THREAD_SIZE - 1)) == (ksp & ~(THREAD_SIZE - 1));
-}
-
-/**
- * regs_get_kernel_stack_nth() - get Nth entry of the stack
- * @regs:pt_regs which contains kernel stack pointer.
- * @n:stack entry number.
- *
- * regs_get_kernel_stack_nth() returns @n th entry of the kernel stack which
- * is specifined by @regs. If the @n th entry is NOT in the kernel stack,
- * this returns 0.
- */
-unsigned long regs_get_kernel_stack_nth(struct pt_regs *regs, unsigned int n)
-{
-	unsigned long addr;
-
-	addr = kernel_stack_pointer(regs) + n * sizeof(long);
-	if (!regs_within_kernel_stack(regs, addr))
-		return 0;
-	return *(unsigned long *)addr;
-}
diff --git a/arch/sh/include/asm/ptrace.h b/arch/sh/include/asm/ptrace.h
index 677c72b..acfdc81 100644
--- a/arch/sh/include/asm/ptrace.h
+++ b/arch/sh/include/asm/ptrace.h
@@ -31,10 +31,6 @@
 #define REG_OFFSET_END {.name = NULL, .offset = 0}
 
 /* Query offset/name of register from its name/offset */
-extern int regs_query_register_offset(const char *name);
-extern const char *regs_query_register_name(unsigned int offset);
-
-extern const struct pt_regs_offset regoffset_table[];
 
 /**
  * regs_get_register() - get register value from its offset
@@ -53,41 +49,6 @@ static inline unsigned long regs_get_register(struct pt_regs *regs,
 	return *(unsigned long *)((unsigned long)regs + offset);
 }
 
-/**
- * regs_within_kernel_stack() - check the address in the stack
- * @regs:	pt_regs which contains kernel stack pointer.
- * @addr:	address which is checked.
- *
- * regs_within_kernel_stack() checks @addr is within the kernel stack page(s).
- * If @addr is within the kernel stack, it returns true. If not, returns false.
- */
-static inline int regs_within_kernel_stack(struct pt_regs *regs,
-					   unsigned long addr)
-{
-	return ((addr & ~(THREAD_SIZE - 1))  ==
-		(kernel_stack_pointer(regs) & ~(THREAD_SIZE - 1)));
-}
-
-/**
- * regs_get_kernel_stack_nth() - get Nth entry of the stack
- * @regs:	pt_regs which contains kernel stack pointer.
- * @n:		stack entry number.
- *
- * regs_get_kernel_stack_nth() returns @n th entry of the kernel stack which
- * is specified by @regs. If the @n th entry is NOT in the kernel stack,
- * this returns 0.
- */
-static inline unsigned long regs_get_kernel_stack_nth(struct pt_regs *regs,
-						      unsigned int n)
-{
-	unsigned long *addr = (unsigned long *)kernel_stack_pointer(regs);
-	addr += n;
-	if (regs_within_kernel_stack(regs, (unsigned long)addr))
-		return *addr;
-	else
-		return 0;
-}
-
 struct perf_event;
 struct perf_sample_data;
 
diff --git a/arch/sh/kernel/Makefile b/arch/sh/kernel/Makefile
index 2ccf36c..4ae9a11 100644
--- a/arch/sh/kernel/Makefile
+++ b/arch/sh/kernel/Makefile
@@ -14,7 +14,7 @@ CFLAGS_REMOVE_return_address.o = -pg
 obj-y	:= debugtraps.o dma-nommu.o dumpstack.o 		\
 	   idle.o io.o irq.o irq_$(BITS).o kdebugfs.o			\
 	   machvec.o nmi_debug.o process.o				\
-	   process_$(BITS).o ptrace.o ptrace_$(BITS).o			\
+	   process_$(BITS).o ptrace_$(BITS).o				\
 	   reboot.o return_address.o					\
 	   setup.o signal_$(BITS).o sys_sh.o 				\
 	   syscalls_$(BITS).o time.o topology.o traps.o			\
diff --git a/arch/sh/kernel/ptrace.c b/arch/sh/kernel/ptrace.c
deleted file mode 100644
index 0a05983..0000000
--- a/arch/sh/kernel/ptrace.c
+++ /dev/null
@@ -1,33 +0,0 @@
-#include <linux/ptrace.h>
-
-/**
- * regs_query_register_offset() - query register offset from its name
- * @name:	the name of a register
- *
- * regs_query_register_offset() returns the offset of a register in struct
- * pt_regs from its name. If the name is invalid, this returns -EINVAL;
- */
-int regs_query_register_offset(const char *name)
-{
-	const struct pt_regs_offset *roff;
-	for (roff = regoffset_table; roff->name != NULL; roff++)
-		if (!strcmp(roff->name, name))
-			return roff->offset;
-	return -EINVAL;
-}
-
-/**
- * regs_query_register_name() - query register name from its offset
- * @offset:	the offset of a register in struct pt_regs.
- *
- * regs_query_register_name() returns the name of a register from its
- * offset in struct pt_regs. If the @offset is invalid, this returns NULL;
- */
-const char *regs_query_register_name(unsigned int offset)
-{
-	const struct pt_regs_offset *roff;
-	for (roff = regoffset_table; roff->name != NULL; roff++)
-		if (roff->offset == offset)
-			return roff->name;
-	return NULL;
-}
diff --git a/arch/sh/kernel/ptrace_32.c b/arch/sh/kernel/ptrace_32.c
index c1a6b89..839f4e7 100644
--- a/arch/sh/kernel/ptrace_32.c
+++ b/arch/sh/kernel/ptrace_32.c
@@ -276,7 +276,7 @@ static int dspregs_active(struct task_struct *target,
 }
 #endif
 
-const struct pt_regs_offset regoffset_table[] = {
+const struct pt_regs_offset regs_offset_table[] = {
 	REGS_OFFSET_NAME(0),
 	REGS_OFFSET_NAME(1),
 	REGS_OFFSET_NAME(2),
diff --git a/arch/sh/kernel/ptrace_64.c b/arch/sh/kernel/ptrace_64.c
index 5cea973..68faa1f 100644
--- a/arch/sh/kernel/ptrace_64.c
+++ b/arch/sh/kernel/ptrace_64.c
@@ -252,7 +252,7 @@ static int fpregs_active(struct task_struct *target,
 }
 #endif
 
-const struct pt_regs_offset regoffset_table[] = {
+const struct pt_regs_offset regs_offset_table[] = {
 	REG_OFFSET_NAME(pc),
 	REG_OFFSET_NAME(sr),
 	REG_OFFSET_NAME(syscall_nr),
diff --git a/arch/x86/include/asm/ptrace.h b/arch/x86/include/asm/ptrace.h
index 5fabf13..c01247c 100644
--- a/arch/x86/include/asm/ptrace.h
+++ b/arch/x86/include/asm/ptrace.h
@@ -157,8 +157,6 @@ static inline unsigned long kernel_stack_pointer(struct pt_regs *regs)
 #include <asm-generic/ptrace.h>
 
 /* Query offset/name of register from its name/offset */
-extern int regs_query_register_offset(const char *name);
-extern const char *regs_query_register_name(unsigned int offset);
 #define MAX_REG_OFFSET (offsetof(struct pt_regs, ss))
 
 /**
@@ -187,41 +185,6 @@ static inline unsigned long regs_get_register(struct pt_regs *regs,
 	return *(unsigned long *)((unsigned long)regs + offset);
 }
 
-/**
- * regs_within_kernel_stack() - check the address in the stack
- * @regs:	pt_regs which contains kernel stack pointer.
- * @addr:	address which is checked.
- *
- * regs_within_kernel_stack() checks @addr is within the kernel stack page(s).
- * If @addr is within the kernel stack, it returns true. If not, returns false.
- */
-static inline int regs_within_kernel_stack(struct pt_regs *regs,
-					   unsigned long addr)
-{
-	return ((addr & ~(THREAD_SIZE - 1))  ==
-		(kernel_stack_pointer(regs) & ~(THREAD_SIZE - 1)));
-}
-
-/**
- * regs_get_kernel_stack_nth() - get Nth entry of the stack
- * @regs:	pt_regs which contains kernel stack pointer.
- * @n:		stack entry number.
- *
- * regs_get_kernel_stack_nth() returns @n th entry of the kernel stack which
- * is specified by @regs. If the @n th entry is NOT in the kernel stack,
- * this returns 0.
- */
-static inline unsigned long regs_get_kernel_stack_nth(struct pt_regs *regs,
-						      unsigned int n)
-{
-	unsigned long *addr = (unsigned long *)kernel_stack_pointer(regs);
-	addr += n;
-	if (regs_within_kernel_stack(regs, (unsigned long)addr))
-		return *addr;
-	else
-		return 0;
-}
-
 #define arch_has_single_step()	(1)
 #ifdef CONFIG_X86_DEBUGCTLMSR
 #define arch_has_block_step()	(1)
diff --git a/arch/x86/kernel/ptrace.c b/arch/x86/kernel/ptrace.c
index f135d35..9daab75 100644
--- a/arch/x86/kernel/ptrace.c
+++ b/arch/x86/kernel/ptrace.c
@@ -56,7 +56,7 @@ enum x86_regset {
 #define REG_OFFSET_NAME(r) {.name = #r, .offset = offsetof(struct pt_regs, r)}
 #define REG_OFFSET_END {.name = NULL, .offset = 0}
 
-static const struct pt_regs_offset regoffset_table[] = {
+const struct pt_regs_offset regs_offset_table[] = {
 #ifdef CONFIG_X86_64
 	REG_OFFSET_NAME(r15),
 	REG_OFFSET_NAME(r14),
@@ -89,38 +89,6 @@ static const struct pt_regs_offset regoffset_table[] = {
 	REG_OFFSET_END,
 };
 
-/**
- * regs_query_register_offset() - query register offset from its name
- * @name:	the name of a register
- *
- * regs_query_register_offset() returns the offset of a register in struct
- * pt_regs from its name. If the name is invalid, this returns -EINVAL;
- */
-int regs_query_register_offset(const char *name)
-{
-	const struct pt_regs_offset *roff;
-	for (roff = regoffset_table; roff->name != NULL; roff++)
-		if (!strcmp(roff->name, name))
-			return roff->offset;
-	return -EINVAL;
-}
-
-/**
- * regs_query_register_name() - query register name from its offset
- * @offset:	the offset of a register in struct pt_regs.
- *
- * regs_query_register_name() returns the name of a register from its
- * offset in struct pt_regs. If the @offset is invalid, this returns NULL;
- */
-const char *regs_query_register_name(unsigned int offset)
-{
-	const struct pt_regs_offset *roff;
-	for (roff = regoffset_table; roff->name != NULL; roff++)
-		if (roff->offset == offset)
-			return roff->name;
-	return NULL;
-}
-
 static const int arg_offs_table[] = {
 #ifdef CONFIG_X86_32
 	[0] = offsetof(struct pt_regs, ax),
diff --git a/include/linux/ptrace.h b/include/linux/ptrace.h
index b0b1ee6..055d9ac 100644
--- a/include/linux/ptrace.h
+++ b/include/linux/ptrace.h
@@ -390,6 +390,48 @@ struct pt_regs_offset {
 	int offset;
 };
 
+extern const struct pt_regs_offset regs_offset_table[];
+
+extern int regs_query_register_offset(const char *name);
+extern const char *regs_query_register_name(unsigned int offset);
+
+/**
+ * regs_within_kernel_stack() - check the address in the stack
+ * @regs:      pt_regs which contains kernel stack pointer.
+ * @addr:      address which is checked.
+ *
+ * regs_within_kernel_stack() checks @addr is within the kernel stack page(s).
+ * If @addr is within the kernel stack, it returns true. If not, returns false.
+ */
+
+static inline bool regs_within_kernel_stack(struct pt_regs *regs,
+						unsigned long addr)
+{
+	return ((addr & ~(THREAD_SIZE - 1))  ==
+		(kernel_stack_pointer(regs) & ~(THREAD_SIZE - 1)));
+}
+
+/**
+ * regs_get_kernel_stack_nth() - get Nth entry of the stack
+ * @regs:	pt_regs which contains kernel stack pointer.
+ * @n:		stack entry number.
+ *
+ * regs_get_kernel_stack_nth() returns @n th entry of the kernel stack which
+ * is specified by @regs. If the @n th entry is NOT in the kernel stack,
+ * this returns 0.
+ */
+static inline unsigned long regs_get_kernel_stack_nth(struct pt_regs *regs,
+						      unsigned int n)
+{
+	unsigned long *addr = (unsigned long *)kernel_stack_pointer(regs);
+
+	addr += n;
+	if (regs_within_kernel_stack(regs, (unsigned long)addr))
+		return *addr;
+	else
+		return 0;
+}
+
 #endif	/* CONFIG_HAVE_REGS_AND_STACK_ACCESS_API */
 
 #endif
diff --git a/kernel/ptrace.c b/kernel/ptrace.c
index c8e0e05..20c08d9 100644
--- a/kernel/ptrace.c
+++ b/kernel/ptrace.c
@@ -1217,3 +1217,41 @@ COMPAT_SYSCALL_DEFINE4(ptrace, compat_long_t, request, compat_long_t, pid,
 	return ret;
 }
 #endif	/* CONFIG_COMPAT */
+
+#ifdef CONFIG_HAVE_REGS_AND_STACK_ACCESS_API
+
+/**
+ * regs_query_register_offset() - query register offset from its name
+ * @name:	the name of a register
+ *
+ * regs_query_register_offset() returns the offset of a register in struct
+ * pt_regs from its name. If the name is invalid, this returns -EINVAL;
+ */
+int regs_query_register_offset(const char *name)
+{
+	const struct pt_regs_offset *roff;
+
+	for (roff = regs_offset_table; roff->name != NULL; roff++)
+		if (!strcmp(roff->name, name))
+			return roff->offset;
+	return -EINVAL;
+}
+
+/**
+ * regs_query_register_name() - query register name from its offset
+ * @offset:	the offset of a register in struct pt_regs.
+ *
+ * regs_query_register_name() returns the name of a register from its
+ * offset in struct pt_regs. If the @offset is invalid, this returns NULL;
+ */
+const char *regs_query_register_name(unsigned int offset)
+{
+	const struct pt_regs_offset *roff;
+
+	for (roff = regs_offset_table; roff->name != NULL; roff++)
+		if (roff->offset == offset)
+			return roff->name;
+	return NULL;
+}
+
+#endif /* CONFIG_HAVE_REGS_AND_STACK_ACCESS_API */
-- 
1.8.1.2

Re: [PATCH 2/2] Consolidate redundant register/stack access code

[rkuo]

On Mon, Jun 15, 2015 at 12:42:59PM -0400, David Long wrote:
> From: "David A. Long" <dave.long@linaro.org>
> 
> Several architectures have identical or functionally equivalent code
> implementing parts of the HAVE_REGS_AND_STACK_ACCESS_API feature.  Move
> that code out of the architecture directories.
> 
> Signed-off-by: David A. Long <dave.long@linaro.org>
> ---
>  arch/arm/include/asm/ptrace.h          |  6 ---
>  arch/arm/kernel/ptrace.c               | 67 +-------------------------------
>  arch/hexagon/include/uapi/asm/ptrace.h |  3 --
>  arch/powerpc/include/asm/ptrace.h      | 38 ------------------
>  arch/powerpc/kernel/ptrace.c           | 34 +----------------
>  arch/s390/include/asm/ptrace.h         |  3 --
>  arch/s390/kernel/ptrace.c              | 70 ++++++++++------------------------
>  arch/sh/include/asm/ptrace.h           | 39 -------------------
>  arch/sh/kernel/Makefile                |  2 +-
>  arch/sh/kernel/ptrace.c                | 33 ----------------
>  arch/sh/kernel/ptrace_32.c             |  2 +-
>  arch/sh/kernel/ptrace_64.c             |  2 +-
>  arch/x86/include/asm/ptrace.h          | 37 ------------------
>  arch/x86/kernel/ptrace.c               | 34 +----------------
>  include/linux/ptrace.h                 | 42 ++++++++++++++++++++
>  kernel/ptrace.c                        | 38 ++++++++++++++++++
>  16 files changed, 107 insertions(+), 343 deletions(-)
>  delete mode 100644 arch/sh/kernel/ptrace.c
> 

(And in some cases, the architecture didn't have the code in the first place!)

For Hexagon:

Acked-by: Richard Kuo <rkuo@codeaurora.org>

-- 
Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, 
a Linux Foundation Collaborative Project

Re: [PATCH 0/2] Consolidate redundant register/stack access code

[Kees Cook]

On Mon, Jun 15, 2015 at 9:42 AM, David Long <dave.long@linaro.org> wrote:
> From: "David A. Long" <dave.long@linaro.org>
>
> Move duplicate and functionally equivalent code for accessing registers
> and stack (CONFIG_HAVE_REGS_AND_STACK_ACCESS_API) from arch subdirs into
> common kernel files.
>
> Note: Help regression testing s390, hexagon, and sh would be appreciated.
>       Powerpc builds but I have not verified the functionality.
>
> David A. Long (2):
>   Move the pt_regs_offset struct definition from arch to common include
>     file
>   Consolidate redundant register/stack access code
>
>  arch/arm/include/asm/ptrace.h          |  6 ---
>  arch/arm/kernel/ptrace.c               | 72 +---------------------------------
>  arch/hexagon/include/uapi/asm/ptrace.h |  3 --
>  arch/powerpc/include/asm/ptrace.h      | 38 ------------------
>  arch/powerpc/kernel/ptrace.c           | 39 +-----------------
>  arch/s390/include/asm/ptrace.h         |  3 --
>  arch/s390/kernel/ptrace.c              | 70 ++++++++++-----------------------
>  arch/sh/include/asm/ptrace.h           | 44 ---------------------
>  arch/sh/kernel/Makefile                |  2 +-
>  arch/sh/kernel/ptrace.c                | 33 ----------------
>  arch/sh/kernel/ptrace_32.c             |  2 +-
>  arch/sh/kernel/ptrace_64.c             |  2 +-
>  arch/x86/include/asm/ptrace.h          | 37 -----------------
>  arch/x86/kernel/ptrace.c               | 39 +-----------------
>  include/linux/ptrace.h                 | 51 ++++++++++++++++++++++++
>  kernel/ptrace.c                        | 38 ++++++++++++++++++
>  16 files changed, 116 insertions(+), 363 deletions(-)
>  delete mode 100644 arch/sh/kernel/ptrace.c

I love the deletions:insertions ratio! :)

Reviewed-by: Kees Cook <keescook@chromium.org>

I wonder why arm64 doesn't define CONFIG_HAVE_REGS_AND_STACK_ACCESS_API?

Thanks!

-Kees

-- 
Kees Cook
Chrome OS Security

Re: [PATCH 0/2] Consolidate redundant register/stack access code

[David Long]

On 06/15/15 16:44, Kees Cook wrote:
> On Mon, Jun 15, 2015 at 9:42 AM, David Long <dave.long@linaro.org> wrote:
>> From: "David A. Long" <dave.long@linaro.org>
>>
>> Move duplicate and functionally equivalent code for accessing registers
>> and stack (CONFIG_HAVE_REGS_AND_STACK_ACCESS_API) from arch subdirs into
>> common kernel files.
>>
>> Note: Help regression testing s390, hexagon, and sh would be appreciated.
>>        Powerpc builds but I have not verified the functionality.
>>
>> David A. Long (2):
>>    Move the pt_regs_offset struct definition from arch to common include
>>      file
>>    Consolidate redundant register/stack access code
>>
>>   arch/arm/include/asm/ptrace.h          |  6 ---
>>   arch/arm/kernel/ptrace.c               | 72 +---------------------------------
>>   arch/hexagon/include/uapi/asm/ptrace.h |  3 --
>>   arch/powerpc/include/asm/ptrace.h      | 38 ------------------
>>   arch/powerpc/kernel/ptrace.c           | 39 +-----------------
>>   arch/s390/include/asm/ptrace.h         |  3 --
>>   arch/s390/kernel/ptrace.c              | 70 ++++++++++-----------------------
>>   arch/sh/include/asm/ptrace.h           | 44 ---------------------
>>   arch/sh/kernel/Makefile                |  2 +-
>>   arch/sh/kernel/ptrace.c                | 33 ----------------
>>   arch/sh/kernel/ptrace_32.c             |  2 +-
>>   arch/sh/kernel/ptrace_64.c             |  2 +-
>>   arch/x86/include/asm/ptrace.h          | 37 -----------------
>>   arch/x86/kernel/ptrace.c               | 39 +-----------------
>>   include/linux/ptrace.h                 | 51 ++++++++++++++++++++++++
>>   kernel/ptrace.c                        | 38 ++++++++++++++++++
>>   16 files changed, 116 insertions(+), 363 deletions(-)
>>   delete mode 100644 arch/sh/kernel/ptrace.c
>
> I love the deletions:insertions ratio! :)
>
> Reviewed-by: Kees Cook <keescook@chromium.org>
>
> I wonder why arm64 doesn't define CONFIG_HAVE_REGS_AND_STACK_ACCESS_API?
>

That support is added in the patch I sent out after this one (to a 
different set of recipients).

Thanks.

> Thanks!
>
> -Kees
>

Re: [PATCH 0/2] Consolidate redundant register/stack access code

[Martin Schwidefsky]

On Mon, 15 Jun 2015 12:42:57 -0400
David Long <dave.long@linaro.org> wrote:

> From: "David A. Long" <dave.long@linaro.org>
> 
> Move duplicate and functionally equivalent code for accessing registers
> and stack (CONFIG_HAVE_REGS_AND_STACK_ACCESS_API) from arch subdirs into
> common kernel files.
> 
> Note: Help regression testing s390, hexagon, and sh would be appreciated.
>       Powerpc builds but I have not verified the functionality.
> 
> David A. Long (2):
>   Move the pt_regs_offset struct definition from arch to common include
>     file
>   Consolidate redundant register/stack access code

Compiles & boots on a s390 box. The code for regs_query_register_offset
and regs_query_register_name is a bit more complex for s390 compared
to the current state of things. Should be equivalent though.

-- 
blue skies,
   Martin.

"Reality continues to ruin my life." - Calvin.

Re: [PATCH 0/2] Consolidate redundant register/stack access code

[Will Deacon]

Hi David,

On Mon, Jun 15, 2015 at 05:42:57PM +0100, David Long wrote:
> From: "David A. Long" <dave.long@linaro.org>
> 
> Move duplicate and functionally equivalent code for accessing registers
> and stack (CONFIG_HAVE_REGS_AND_STACK_ACCESS_API) from arch subdirs into
> common kernel files.
> 
> Note: Help regression testing s390, hexagon, and sh would be appreciated.
>       Powerpc builds but I have not verified the functionality.
> 
> David A. Long (2):
>   Move the pt_regs_offset struct definition from arch to common include
>     file
>   Consolidate redundant register/stack access code
> 
>  arch/arm/include/asm/ptrace.h          |  6 ---
>  arch/arm/kernel/ptrace.c               | 72 +---------------------------------
>  arch/hexagon/include/uapi/asm/ptrace.h |  3 --
>  arch/powerpc/include/asm/ptrace.h      | 38 ------------------
>  arch/powerpc/kernel/ptrace.c           | 39 +-----------------
>  arch/s390/include/asm/ptrace.h         |  3 --
>  arch/s390/kernel/ptrace.c              | 70 ++++++++++-----------------------
>  arch/sh/include/asm/ptrace.h           | 44 ---------------------
>  arch/sh/kernel/Makefile                |  2 +-
>  arch/sh/kernel/ptrace.c                | 33 ----------------
>  arch/sh/kernel/ptrace_32.c             |  2 +-
>  arch/sh/kernel/ptrace_64.c             |  2 +-
>  arch/x86/include/asm/ptrace.h          | 37 -----------------
>  arch/x86/kernel/ptrace.c               | 39 +-----------------
>  include/linux/ptrace.h                 | 51 ++++++++++++++++++++++++
>  kernel/ptrace.c                        | 38 ++++++++++++++++++
>  16 files changed, 116 insertions(+), 363 deletions(-)

Nice cleanup! The ARMy bits all look fine to me.

  Acked-by: Will Deacon <will.deacon@arm.com>

Will