Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1444185 > unrolled thread

Re: [PATCH v15 01/10] arm64: Add HAVE_REGS_AND_STACK_ACCESS_API feature

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v15 01/10] arm64: Add HAVE_REGS_AND_STACK_ACCESS_API  feature Catalin Marinas <catalin.marinas@arm.com> - 2016-07-15 13:00 +0200
    Re: [PATCH v15 01/10] arm64: Add HAVE_REGS_AND_STACK_ACCESS_API  feature David Long <dave.long@linaro.org> - 2016-07-15 17:00 +0200
      Re: [PATCH v15 01/10] arm64: Add HAVE_REGS_AND_STACK_ACCESS_API  feature Catalin Marinas <catalin.marinas@arm.com> - 2016-07-15 17:20 +0200
        Re: [PATCH v15 01/10] arm64: Add HAVE_REGS_AND_STACK_ACCESS_API  feature David Long <dave.long@linaro.org> - 2016-07-15 20:00 +0200
          Re: [PATCH v15 01/10] arm64: Add HAVE_REGS_AND_STACK_ACCESS_API  feature Catalin Marinas <catalin.marinas@arm.com> - 2016-07-19 16:20 +0200

#1444185 — Re: [PATCH v15 01/10] arm64: Add HAVE_REGS_AND_STACK_ACCESS_API feature

On Fri, Jul 08, 2016 at 12:35:45PM -0400, David Long wrote:
> --- a/arch/arm64/include/asm/ptrace.h
> +++ b/arch/arm64/include/asm/ptrace.h
> @@ -74,6 +74,7 @@
>  #define COMPAT_PT_DATA_ADDR		0x10004
>  #define COMPAT_PT_TEXT_END_ADDR		0x10008
>  #ifndef __ASSEMBLY__
> +#include <linux/bug.h>
>  
>  /* sizeof(struct user) for AArch32 */
>  #define COMPAT_USER_SZ	296
> @@ -119,6 +120,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 +150,55 @@ 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);

Is regs_query_register_offset() used anywhere? I grep'ed the kernel with
these patches applied but couldn't find any use.

> +extern bool regs_within_kernel_stack(struct pt_regs *regs, unsigned long addr);

This one only seems to be used in arch/arm64/kernel/ptrace.c. Can we
make it static and remove the declaration?

-- 
Catalin

[toc] | [next] | [standalone]

#1444354

On 07/15/2016 06:57 AM, Catalin Marinas wrote:
> On Fri, Jul 08, 2016 at 12:35:45PM -0400, David Long wrote:
>> --- a/arch/arm64/include/asm/ptrace.h
>> +++ b/arch/arm64/include/asm/ptrace.h
>> @@ -74,6 +74,7 @@
>>   #define COMPAT_PT_DATA_ADDR		0x10004
>>   #define COMPAT_PT_TEXT_END_ADDR		0x10008
>>   #ifndef __ASSEMBLY__
>> +#include <linux/bug.h>
>>
>>   /* sizeof(struct user) for AArch32 */
>>   #define COMPAT_USER_SZ	296
>> @@ -119,6 +120,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 +150,55 @@ 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);
>
> Is regs_query_register_offset() used anywhere? I grep'ed the kernel with
> these patches applied but couldn't find any use.
>

It's referenced in kernel/trace/trace_probe.c.

>> +extern bool regs_within_kernel_stack(struct pt_regs *regs, unsigned long addr);
>
> This one only seems to be used in arch/arm64/kernel/ptrace.c. Can we
> make it static and remove the declaration?
>

OK.

Thanks,
-dl

[toc] | [prev] | [next] | [standalone]

#1444370

On Fri, Jul 15, 2016 at 10:51:23AM -0400, David Long wrote:
> On 07/15/2016 06:57 AM, Catalin Marinas wrote:
> > On Fri, Jul 08, 2016 at 12:35:45PM -0400, David Long wrote:
> > > --- a/arch/arm64/include/asm/ptrace.h
> > > +++ b/arch/arm64/include/asm/ptrace.h
> > > @@ -74,6 +74,7 @@
> > >   #define COMPAT_PT_DATA_ADDR		0x10004
> > >   #define COMPAT_PT_TEXT_END_ADDR		0x10008
> > >   #ifndef __ASSEMBLY__
> > > +#include <linux/bug.h>
> > > 
> > >   /* sizeof(struct user) for AArch32 */
> > >   #define COMPAT_USER_SZ	296
> > > @@ -119,6 +120,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 +150,55 @@ 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);
> > 
> > Is regs_query_register_offset() used anywhere? I grep'ed the kernel with
> > these patches applied but couldn't find any use.
> 
> It's referenced in kernel/trace/trace_probe.c.

I meant regs_query_register_name() (vim completion wrote the first one).

> > > +extern bool regs_within_kernel_stack(struct pt_regs *regs, unsigned long addr);
> > 
> > This one only seems to be used in arch/arm64/kernel/ptrace.c. Can we
> > make it static and remove the declaration?
> 
> OK.

I can change it locally.

Are these going to be used in the future by uprobes?

-- 
Catalin

[toc] | [prev] | [next] | [standalone]

#1444491

On 07/15/2016 11:13 AM, Catalin Marinas wrote:
> On Fri, Jul 15, 2016 at 10:51:23AM -0400, David Long wrote:
>> On 07/15/2016 06:57 AM, Catalin Marinas wrote:
>>> On Fri, Jul 08, 2016 at 12:35:45PM -0400, David Long wrote:
>>>> --- a/arch/arm64/include/asm/ptrace.h
>>>> +++ b/arch/arm64/include/asm/ptrace.h
>>>> @@ -74,6 +74,7 @@
>>>>    #define COMPAT_PT_DATA_ADDR		0x10004
>>>>    #define COMPAT_PT_TEXT_END_ADDR		0x10008
>>>>    #ifndef __ASSEMBLY__
>>>> +#include <linux/bug.h>
>>>>
>>>>    /* sizeof(struct user) for AArch32 */
>>>>    #define COMPAT_USER_SZ	296
>>>> @@ -119,6 +120,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 +150,55 @@ 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);
>>>
>>> Is regs_query_register_offset() used anywhere? I grep'ed the kernel with
>>> these patches applied but couldnperf_regs.c't find any use.
>>
>> It's referenced in kernel/trace/trace_probe.c.
>
> I meant regs_query_register_name() (vim completion wrote the first one).
>

I had assumed it was used by kgdb to provide human-readable register 
names for debugger output, but apparently that is handled inside the 
kgdb.c stub for each architecture. I can only assume this is currently 
provided in all (or at least most) architectures because some code 
outside the kernel tree needs (or used to need?) to be able to do the 
reverse of regs_query_register_offset(). It's easy enough to remove 
this, but that will make arm64 unlike the other architectures in its 
CONFIG_HAVE_REGS_AND_STACK_ACCESS_API support.

>>>> +extern bool regs_within_kernel_stack(struct pt_regs *regs, unsigned long addr);
>>>
>>> This one only seems to be used in arch/arm64/kernel/ptrace.c. Can we
>>> make it static and remove the declaration?
>>
>> OK.
>
> I can change it locally.

It's looking like there will need to be another iteration of the patch 
for a few small things anyway, although those changes could also be done 
as subsequent improvements.

>
> Are these going to be used in the future by uprobes?
>

It would appear not.

Thanks,
-dl

[toc] | [prev] | [next] | [standalone]

#1446510

On Fri, Jul 15, 2016 at 01:51:02PM -0400, David Long wrote:
> On 07/15/2016 11:13 AM, Catalin Marinas wrote:
> >On Fri, Jul 15, 2016 at 10:51:23AM -0400, David Long wrote:
> >>On 07/15/2016 06:57 AM, Catalin Marinas wrote:
> >>>On Fri, Jul 08, 2016 at 12:35:45PM -0400, David Long wrote:
> >>>>+extern bool regs_within_kernel_stack(struct pt_regs *regs, unsigned long addr);
> >>>
> >>>This one only seems to be used in arch/arm64/kernel/ptrace.c. Can we
> >>>make it static and remove the declaration?
> >>
> >>OK.
> >
> >I can change it locally.
> 
> It's looking like there will need to be another iteration of the patch for a
> few small things anyway, although those changes could also be done as
> subsequent improvements.

I've pushed the branch below with my fixups on this series. Please post
any minor changes you have as additional patches on top. Thanks.

git://git.kernel.org/pub/scm/linux/kernel/git/arm64/linux.git for-next/kprobes

-- 
Catalin

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web