| Message ID | 1461783185-9056-2-git-send-email-dave.long@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 27/04/16 19:52, David Long wrote: > From: "David A. Long" <dave.long@linaro.org> > > Add HAVE_REGS_AND_STACK_ACCESS_API feature for arm64. And clearly a lot more. > > Signed-off-by: David A. Long <dave.long@linaro.org> > --- > arch/arm64/Kconfig | 1 + > arch/arm64/include/asm/ptrace.h | 33 ++++++++++- > arch/arm64/kernel/ptrace.c | 118 ++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 151 insertions(+), 1 deletion(-) > > diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig > index 4f43622..8f662fd 100644 > --- a/arch/arm64/Kconfig > +++ b/arch/arm64/Kconfig > @@ -80,6 +80,7 @@ config ARM64 > select HAVE_PERF_EVENTS > select HAVE_PERF_REGS > select HAVE_PERF_USER_STACK_DUMP > + select HAVE_REGS_AND_STACK_ACCESS_API > select HAVE_RCU_TABLE_FREE > select HAVE_SYSCALL_TRACEPOINTS > select IOMMU_DMA if IOMMU_SUPPORT > diff --git a/arch/arm64/include/asm/ptrace.h b/arch/arm64/include/asm/ptrace.h > index a307eb6..ee02637 100644 > --- a/arch/arm64/include/asm/ptrace.h > +++ b/arch/arm64/include/asm/ptrace.h > @@ -119,6 +119,8 @@ struct pt_regs { > u64 syscallno; > }; > > +#define MAX_REG_OFFSET offsetof(struct pt_regs, pstate) > + > #define arch_has_single_step() (1) > > #ifdef CONFIG_COMPAT > @@ -147,6 +149,35 @@ struct pt_regs { > #define user_stack_pointer(regs) \ > (!compat_user_mode(regs) ? (regs)->sp : (regs)->compat_sp) > > +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 > + * @offset: offset number of the register. Is it the offset? or the number? > + * > + * regs_get_register returns the value of a register whose offset from @regs. > + * The @offset is the offset of the register in struct pt_regs. > + * If @offset is bigger than MAX_REG_OFFSET, this returns 0. > + */ > +static inline u64 regs_get_register(struct pt_regs *regs, > + unsigned int offset) > +{ > + if (unlikely(offset > MAX_REG_OFFSET)) > + return 0; > + return *(u64 *)((u64)regs + offset); So clearly it is the offset. But is 3 a valid value? I don't think so. How about something slightly more type safe: u64 val = 0; WARN_ON(offset & 7); offset >>= 3; switch (offset) { case 0 ... 30: val = regs->reg[offset]; break; case 31: val = regs->sp; break; case 32: val = regs->pc; break; case 33: val = regs->pstate; break; } return val; I'm pretty sure you could replace 31/32/33 with macros using offsetof(). The compiler may even optimize this to something similar to what you already have. Thanks, M.
On 04/28/2016 12:08 PM, Marc Zyngier wrote: > On 27/04/16 19:52, David Long wrote: >> From: "David A. Long" <dave.long@linaro.org> >> >> Add HAVE_REGS_AND_STACK_ACCESS_API feature for arm64. > > And clearly a lot more. > >> >> Signed-off-by: David A. Long <dave.long@linaro.org> >> --- >> arch/arm64/Kconfig | 1 + >> arch/arm64/include/asm/ptrace.h | 33 ++++++++++- >> arch/arm64/kernel/ptrace.c | 118 ++++++++++++++++++++++++++++++++++++++++ >> 3 files changed, 151 insertions(+), 1 deletion(-) >> >> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig >> index 4f43622..8f662fd 100644 >> --- a/arch/arm64/Kconfig >> +++ b/arch/arm64/Kconfig >> @@ -80,6 +80,7 @@ config ARM64 >> select HAVE_PERF_EVENTS >> select HAVE_PERF_REGS >> select HAVE_PERF_USER_STACK_DUMP >> + select HAVE_REGS_AND_STACK_ACCESS_API >> select HAVE_RCU_TABLE_FREE >> select HAVE_SYSCALL_TRACEPOINTS >> select IOMMU_DMA if IOMMU_SUPPORT >> diff --git a/arch/arm64/include/asm/ptrace.h b/arch/arm64/include/asm/ptrace.h >> index a307eb6..ee02637 100644 >> --- a/arch/arm64/include/asm/ptrace.h >> +++ b/arch/arm64/include/asm/ptrace.h >> @@ -119,6 +119,8 @@ struct pt_regs { >> u64 syscallno; >> }; >> >> +#define MAX_REG_OFFSET offsetof(struct pt_regs, pstate) >> + >> #define arch_has_single_step() (1) >> >> #ifdef CONFIG_COMPAT >> @@ -147,6 +149,35 @@ struct pt_regs { >> #define user_stack_pointer(regs) \ >> (!compat_user_mode(regs) ? (regs)->sp : (regs)->compat_sp) >> >> +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 >> + * @offset: offset number of the register. > > Is it the offset? or the number? > I've removed "number" from the description. >> + * >> + * regs_get_register returns the value of a register whose offset from @regs. >> + * The @offset is the offset of the register in struct pt_regs. >> + * If @offset is bigger than MAX_REG_OFFSET, this returns 0. >> + */ >> +static inline u64 regs_get_register(struct pt_regs *regs, >> + unsigned int offset) >> +{ >> + if (unlikely(offset > MAX_REG_OFFSET)) >> + return 0; >> + return *(u64 *)((u64)regs + offset); > > So clearly it is the offset. But is 3 a valid value? I don't think so. > How about something slightly more type safe: > > u64 val = 0; > > WARN_ON(offset & 7); > > offset >>= 3; > switch (offset) { > case 0 ... 30: > val = regs->reg[offset]; > break; > case 31: > val = regs->sp; > break; > case 32: > val = regs->pc; > break; > case 33: > val = regs->pstate; > break; > } > > return val; > > I'm pretty sure you could replace 31/32/33 with macros using offsetof(). > The compiler may even optimize this to something similar to what you > already have. > > Thanks, > > M. > I'm not sure this makes sense to me since my best understanding of this suite of functions makes the "offset" argument effectively an opaque type with a value returned from looking up the register by name in a compiled in table that reliably determines the offset within the pt_regs structure (i.e.: regs_query_register_offset() and regs_query_register_name()). Now this change introduces the assumption that the gp registers start at the beginning of the structure, presumably for the future use of someone hardcoding their own number for "offset". Nonetheless in the interest of moving forward I currently have this change sitting in my next patch set. Thanks, -dl
On Wed, Apr 27, 2016 at 02:52:56PM -0400, David Long wrote: > From: "David A. Long" <dave.long@linaro.org> > +/** > + * 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))) || > + on_irq_stack(addr, raw_smp_processor_id()); > +} > + > +/** > + * 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)) If the @addr fall in the interrupt stack, the regs_within_kernel_stack() will return true. But Is it what we want? thanks Huang Shijie
On 05/17/2016 05:14 AM, Huang Shijie wrote: > On Wed, Apr 27, 2016 at 02:52:56PM -0400, David Long wrote: >> From: "David A. Long" <dave.long@linaro.org> >> +/** >> + * 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))) || >> + on_irq_stack(addr, raw_smp_processor_id()); >> +} >> + >> +/** >> + * 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)) > If the @addr fall in the interrupt stack, the regs_within_kernel_stack() > will return true. But Is it what we want? > Yes, I think it is. The function is used in regs_get_kernel_stack_nth() to make sure the data being asked for (based on the pt_regs saved stack pointer) is actually on the stack, whether it's "kernel" stack or "irq" stack. Thanks, -dl
diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig index 4f43622..8f662fd 100644 --- a/arch/arm64/Kconfig +++ b/arch/arm64/Kconfig @@ -80,6 +80,7 @@ config ARM64 select HAVE_PERF_EVENTS select HAVE_PERF_REGS select HAVE_PERF_USER_STACK_DUMP + select HAVE_REGS_AND_STACK_ACCESS_API select HAVE_RCU_TABLE_FREE select HAVE_SYSCALL_TRACEPOINTS select IOMMU_DMA if IOMMU_SUPPORT diff --git a/arch/arm64/include/asm/ptrace.h b/arch/arm64/include/asm/ptrace.h index a307eb6..ee02637 100644 --- a/arch/arm64/include/asm/ptrace.h +++ b/arch/arm64/include/asm/ptrace.h @@ -119,6 +119,8 @@ struct pt_regs { u64 syscallno; }; +#define MAX_REG_OFFSET offsetof(struct pt_regs, pstate) + #define arch_has_single_step() (1) #ifdef CONFIG_COMPAT @@ -147,6 +149,35 @@ struct pt_regs { #define user_stack_pointer(regs) \ (!compat_user_mode(regs) ? (regs)->sp : (regs)->compat_sp) +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 + * @offset: offset number of the register. + * + * regs_get_register returns the value of a register whose offset from @regs. + * The @offset is the offset of the register in struct pt_regs. + * If @offset is bigger than MAX_REG_OFFSET, this returns 0. + */ +static inline u64 regs_get_register(struct pt_regs *regs, + unsigned int offset) +{ + if (unlikely(offset > MAX_REG_OFFSET)) + return 0; + return *(u64 *)((u64)regs + offset); +} + +/* Valid only for Kernel mode traps. */ +static inline unsigned long kernel_stack_pointer(struct pt_regs *regs) +{ + return regs->sp; +} + static inline unsigned long regs_return_value(struct pt_regs *regs) { return regs->regs[0]; @@ -156,7 +187,7 @@ static inline unsigned long regs_return_value(struct pt_regs *regs) struct task_struct; int valid_user_regs(struct user_pt_regs *regs, struct task_struct *task); -#define instruction_pointer(regs) ((unsigned long)(regs)->pc) +#define instruction_pointer(regs) ((regs)->pc) extern unsigned long profile_pc(struct pt_regs *regs); diff --git a/arch/arm64/kernel/ptrace.c b/arch/arm64/kernel/ptrace.c index 3f6cd5c..2c88c33 100644 --- a/arch/arm64/kernel/ptrace.c +++ b/arch/arm64/kernel/ptrace.c @@ -48,6 +48,124 @@ #define CREATE_TRACE_POINTS #include <trace/events/syscalls.h> +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} +#define GPR_OFFSET_NAME(r) \ + {.name = "x" #r, .offset = offsetof(struct pt_regs, regs[r])} + +static const struct pt_regs_offset regoffset_table[] = { + GPR_OFFSET_NAME(0), + GPR_OFFSET_NAME(1), + GPR_OFFSET_NAME(2), + GPR_OFFSET_NAME(3), + GPR_OFFSET_NAME(4), + GPR_OFFSET_NAME(5), + GPR_OFFSET_NAME(6), + GPR_OFFSET_NAME(7), + GPR_OFFSET_NAME(8), + GPR_OFFSET_NAME(9), + GPR_OFFSET_NAME(10), + GPR_OFFSET_NAME(11), + GPR_OFFSET_NAME(12), + GPR_OFFSET_NAME(13), + GPR_OFFSET_NAME(14), + GPR_OFFSET_NAME(15), + GPR_OFFSET_NAME(16), + GPR_OFFSET_NAME(17), + GPR_OFFSET_NAME(18), + GPR_OFFSET_NAME(19), + GPR_OFFSET_NAME(20), + GPR_OFFSET_NAME(21), + GPR_OFFSET_NAME(22), + GPR_OFFSET_NAME(23), + GPR_OFFSET_NAME(24), + GPR_OFFSET_NAME(25), + GPR_OFFSET_NAME(26), + GPR_OFFSET_NAME(27), + GPR_OFFSET_NAME(28), + GPR_OFFSET_NAME(29), + GPR_OFFSET_NAME(30), + {.name = "lr", .offset = offsetof(struct pt_regs, regs[30])}, + REG_OFFSET_NAME(sp), + REG_OFFSET_NAME(pc), + REG_OFFSET_NAME(pstate), + 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))) || + on_irq_stack(addr, raw_smp_processor_id()); +} + +/** + * 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; +} + /* * TODO: does not yet catch signals sent when the child dies. * in exit.c or in signal.c.