ARM tracehook support

ARM tracehook support

[Wade Farnsworth]

Hi all,

A few months back, Steven Walter posted some patches that implemement 
syscall tracing support for ARM:

http://lists.arm.linux.org.uk/lurker/message/20111129.162812.bd17d9b5.en.html#linux-arm-kernel

I've been working on SystemTap support for ARM, and the tracehook patch 
in particular is interesting in that respect.  With that patch applied, 
it would allow utrace to be supported on ARM, which is necessary for 
userspace stap probes to function properly.

 From what testing I've done, the patches don't appear to be harmful. 
So, I'm wondering what's preventing this from being pushed into 
mainline?  Are there still outstanding issues that need to be addressed? 
  I'd be happy to help solve them if it would help this patch get into 
mainline quicker.

Thanks in advance!

Wade Farnsworth

ARM tracehook support

[Will Deacon]

On Mon, Feb 20, 2012 at 03:37:57PM +0000, Wade Farnsworth wrote:
> Hi all,

Hello Wade,

> A few months back, Steven Walter posted some patches that implemement 
> syscall tracing support for ARM:
> 
> http://lists.arm.linux.org.uk/lurker/message/20111129.162812.bd17d9b5.en.html#linux-arm-kernel
> 
> I've been working on SystemTap support for ARM, and the tracehook patch 
> in particular is interesting in that respect.  With that patch applied, 
> it would allow utrace to be supported on ARM, which is necessary for 
> userspace stap probes to function properly.
> 
>  From what testing I've done, the patches don't appear to be harmful. 
> So, I'm wondering what's preventing this from being pushed into 
> mainline?  Are there still outstanding issues that need to be addressed? 

Yup, I posted some questions here:

http://lists.infradead.org/pipermail/linux-arm-kernel/2011-December/074802.html

but I haven't heard anything from Steven since then, so it's all stalled at
the moment. They'll also obviously need rebasing onto latest mainline,
although that shouldn't be too hard since they're still fairly recent.

Will

ARM tracehook support

[Wade Farnsworth]

Will Deacon wrote:
> On Mon, Feb 20, 2012 at 03:37:57PM +0000, Wade Farnsworth wrote:
>> Hi all,
>
> Hello Wade,
>
>> A few months back, Steven Walter posted some patches that implemement
>> syscall tracing support for ARM:
>>
>> http://lists.arm.linux.org.uk/lurker/message/20111129.162812.bd17d9b5.en.html#linux-arm-kernel
>>
>> I've been working on SystemTap support for ARM, and the tracehook patch
>> in particular is interesting in that respect.  With that patch applied,
>> it would allow utrace to be supported on ARM, which is necessary for
>> userspace stap probes to function properly.
>>
>>   From what testing I've done, the patches don't appear to be harmful.
>> So, I'm wondering what's preventing this from being pushed into
>> mainline?  Are there still outstanding issues that need to be addressed?
>
> Yup, I posted some questions here:
>
> http://lists.infradead.org/pipermail/linux-arm-kernel/2011-December/074802.html

OK, so to continue that conversation:

Will Deacon wrote:
 > On Wed, Nov 30, 2011 at 02:46:20PM +0000, Steven Walter wrote:
 >> +static inline void syscall_get_arguments(struct task_struct *task,
 >> +					 struct pt_regs *regs,
 >> +					 unsigned int i, unsigned int n,
 >> +					 unsigned long *args)
 >> +{
 >> +	BUG_ON(i + n > 6);
 >
 > So I guess 6 is the maximum number of registers that are used for
 > syscall passing. That sounds about right to me, but I wondered how
 > you worked it out (and whether or not it should be defined
 > somewhere?).

I believe the 6 argument constraint is a specific to 
syscall_get_arguments().  Notice the comment in 
include/asm-generic/syscall.h:

/*
[...]
  *
  * It's only valid to call this when @task is stopped for tracing on
  * entry to a system call, due to %TIF_SYSCALL_TRACE or %TIF_SYSCALL_AUDIT.
  * It's invalid to call this with @i + @n > 6; we only support system calls
  * taking up to 6 arguments.
  */

Additionally, if you'll look at the other architectures' implementations 
you'll see similar code.

 > In fact, how are these things supposed to deal with 64-bit arguments
 > that straddle two registers? I think we always pack arguments such
 > that we don't get holes in the register layout, but it might be worth 
 > checking (EABI requires 64-bit arguments to be passed in even
 > registers).

Hmm, I do believe that 32-bit powerpc has similar alignment issues 
(64-bit args must be passed in odd/even pairs), but I don't see any 
special handling of this in that architecture's 
syscall_get/set_arguments().  So I'm wondering if the handling of this 
is or should be handled elsewhere.  I'll keep digging on this.

>
> but I haven't heard anything from Steven since then, so it's all stalled at
> the moment. They'll also obviously need rebasing onto latest mainline,
> although that shouldn't be too hard since they're still fairly recent.
>

Yes, it does not appear to be too difficult.  I've already made some 
progress here, and will post my patches once the other issues have been 
resolved.

Thanks again,

-Wade

ARM tracehook support

[Wade Farnsworth]

Wade Farnsworth wrote:
> Will Deacon wrote:
>> On Mon, Feb 20, 2012 at 03:37:57PM +0000, Wade Farnsworth wrote:
>>> Hi all,
>>
>> Hello Wade,
>>
>>> A few months back, Steven Walter posted some patches that implemement
>>> syscall tracing support for ARM:
>>>
>>> http://lists.arm.linux.org.uk/lurker/message/20111129.162812.bd17d9b5.en.html#linux-arm-kernel
>>>
>>>
>>> I've been working on SystemTap support for ARM, and the tracehook patch
>>> in particular is interesting in that respect. With that patch applied,
>>> it would allow utrace to be supported on ARM, which is necessary for
>>> userspace stap probes to function properly.
>>>
>>> From what testing I've done, the patches don't appear to be harmful.
>>> So, I'm wondering what's preventing this from being pushed into
>>> mainline? Are there still outstanding issues that need to be addressed?
>>
>> Yup, I posted some questions here:
>>
>> http://lists.infradead.org/pipermail/linux-arm-kernel/2011-December/074802.html
>>
>
> OK, so to continue that conversation:
>
> Will Deacon wrote:
>  > On Wed, Nov 30, 2011 at 02:46:20PM +0000, Steven Walter wrote:
>  >> +static inline void syscall_get_arguments(struct task_struct *task,
>  >> + struct pt_regs *regs,
>  >> + unsigned int i, unsigned int n,
>  >> + unsigned long *args)
>  >> +{
>  >> + BUG_ON(i + n > 6);
>  >
>  > So I guess 6 is the maximum number of registers that are used for
>  > syscall passing. That sounds about right to me, but I wondered how
>  > you worked it out (and whether or not it should be defined
>  > somewhere?).
>
> I believe the 6 argument constraint is a specific to
> syscall_get_arguments(). Notice the comment in
> include/asm-generic/syscall.h:
>
> /*
> [...]
> *
> * It's only valid to call this when @task is stopped for tracing on
> * entry to a system call, due to %TIF_SYSCALL_TRACE or %TIF_SYSCALL_AUDIT.
> * It's invalid to call this with @i + @n > 6; we only support system calls
> * taking up to 6 arguments.
> */
>
> Additionally, if you'll look at the other architectures' implementations
> you'll see similar code.
>
>  > In fact, how are these things supposed to deal with 64-bit arguments
>  > that straddle two registers? I think we always pack arguments such
>  > that we don't get holes in the register layout, but it might be worth
>  > checking (EABI requires 64-bit arguments to be passed in even
>  > registers).
>
> Hmm, I do believe that 32-bit powerpc has similar alignment issues
> (64-bit args must be passed in odd/even pairs), but I don't see any
> special handling of this in that architecture's
> syscall_get/set_arguments(). So I'm wondering if the handling of this is
> or should be handled elsewhere. I'll keep digging on this.
>

This question also came up from an older attempt at ARM tracehook by 
Roland McGrath:

http://lists.openwall.net/linux-kernel/2009/06/24/150

In a nutshell, the argument is that syscall_get_arguments() doesn't need 
to know about any of the argument semantics, they just pass all 
registers that may contain arguments back up the stack.  It's then up to 
the tracer to interpret the arguments.  The argument is similar for 
syscall_set_arguments().  Since the same register set is used for both 
EABI and OABI, there doesn't need to be any specific handling of one 
versus the other here.

Thanks,

-Wade

ARM tracehook support

[Will Deacon]

Hi Wade,

Thanks for following this up.

On Tue, Feb 21, 2012 at 05:06:20PM +0000, Wade Farnsworth wrote:
> >
> > I believe the 6 argument constraint is a specific to
> > syscall_get_arguments(). Notice the comment in
> > include/asm-generic/syscall.h:
> >
> > /*
> > [...]
> > *
> > * It's only valid to call this when @task is stopped for tracing on
> > * entry to a system call, due to %TIF_SYSCALL_TRACE or %TIF_SYSCALL_AUDIT.
> > * It's invalid to call this with @i + @n > 6; we only support system calls
> > * taking up to 6 arguments.
> > */
> >
> > Additionally, if you'll look at the other architectures' implementations
> > you'll see similar code.

In which case could we #define this somewhere to make it more obvious?

> In a nutshell, the argument is that syscall_get_arguments() doesn't need 
> to know about any of the argument semantics, they just pass all 
> registers that may contain arguments back up the stack.  It's then up to 
> the tracer to interpret the arguments.  The argument is similar for 
> syscall_set_arguments().  Since the same register set is used for both 
> EABI and OABI, there doesn't need to be any specific handling of one 
> versus the other here.

Ok, if the low-level code doesn't need to interpret arguments then that
should be fine. sys_arm_fadvise64_64 would be the tricky case, but the
argument ordering seems to have been chosen explicitly to place the 64-bit
offsets naturally on even registers, fitting it neatly into 6 registers.

Once you've rebased the code, please post a new version so it can be
reviewed again.

Cheers,

Will

ARM tracehook support

[Russell King - ARM Linux]

On Tue, Feb 21, 2012 at 05:24:09PM +0000, Will Deacon wrote:
> Ok, if the low-level code doesn't need to interpret arguments then that
> should be fine. sys_arm_fadvise64_64 would be the tricky case, but the
> argument ordering seems to have been chosen explicitly to place the 64-bit
> offsets naturally on even registers, fitting it neatly into 6 registers.

That was done because the standard argument order was impossible to pass
in the available register set with EABI.  It would have meant having to
move the syscall number into r8 or higher, which would've been rather
unfriendly to Thumb code.

ARM tracehook support

[Russell King - ARM Linux]

On Mon, Feb 20, 2012 at 11:08:57AM -0700, Wade Farnsworth wrote:
> Will Deacon wrote:
> > On Wed, Nov 30, 2011 at 02:46:20PM +0000, Steven Walter wrote:
> >> +static inline void syscall_get_arguments(struct task_struct *task,
> >> +					 struct pt_regs *regs,
> >> +					 unsigned int i, unsigned int n,
> >> +					 unsigned long *args)
> >> +{
> >> +	BUG_ON(i + n > 6);
> >
> > So I guess 6 is the maximum number of registers that are used for
> > syscall passing. That sounds about right to me, but I wondered how
> > you worked it out (and whether or not it should be defined
> > somewhere?).
>
> I believe the 6 argument constraint is a specific to  
> syscall_get_arguments().  Notice the comment in  
> include/asm-generic/syscall.h:

Well, there's two things here.  Is a BUG_ON() really suitable here?
What controls how many arguments are fetched?  Userspace?  If so, that's
a nice way to oops the kernel.

Secondly, there is a 7 argument syscall - sys_syscall, which we use on
OABI to deal with calling a syscall by number.  That really does show
up as a unique syscall there, so if you want to parse the last argument
to such a syscall you need to be able to read up to and including ARM
register 7.

> /*
> [...]
>  *
>  * It's only valid to call this when @task is stopped for tracing on
>  * entry to a system call, due to %TIF_SYSCALL_TRACE or %TIF_SYSCALL_AUDIT.
>  * It's invalid to call this with @i + @n > 6; we only support system calls
>  * taking up to 6 arguments.
>  */
>
> Additionally, if you'll look at the other architectures' implementations  
> you'll see similar code.
>
> > In fact, how are these things supposed to deal with 64-bit arguments
> > that straddle two registers? I think we always pack arguments such
> > that we don't get holes in the register layout, but it might be worth  
> > checking (EABI requires 64-bit arguments to be passed in even
> > registers).
>
> Hmm, I do believe that 32-bit powerpc has similar alignment issues  
> (64-bit args must be passed in odd/even pairs), but I don't see any  
> special handling of this in that architecture's  
> syscall_get/set_arguments().  So I'm wondering if the handling of this  
> is or should be handled elsewhere.  I'll keep digging on this.

I did point that issue out when it first came up, and I think Roland had
an answer for it, though I forget what it was.

The big stumbling block to this is OABI, and as I continue to be wholely
OABI based here, it's extremely important that nothing in OABI land gets
broken.  I don't see there's any chance of me ever getting off OABI given
the range of platforms I have, and the restriction that EABI was designed
to be impossible on ARMv4 architectures.

ARM tracehook support

[Wade Farnsworth]

Russell King - ARM Linux wrote:
> On Mon, Feb 20, 2012 at 11:08:57AM -0700, Wade Farnsworth wrote:
>> Will Deacon wrote:
>>> On Wed, Nov 30, 2011 at 02:46:20PM +0000, Steven Walter wrote:
>>>> +static inline void syscall_get_arguments(struct task_struct *task,
>>>> +					 struct pt_regs *regs,
>>>> +					 unsigned int i, unsigned int n,
>>>> +					 unsigned long *args)
>>>> +{
>>>> +	BUG_ON(i + n>  6);
>>>
>>> So I guess 6 is the maximum number of registers that are used for
>>> syscall passing. That sounds about right to me, but I wondered how
>>> you worked it out (and whether or not it should be defined
>>> somewhere?).
>>
>> I believe the 6 argument constraint is a specific to
>> syscall_get_arguments().  Notice the comment in
>> include/asm-generic/syscall.h:
>
> Well, there's two things here.  Is a BUG_ON() really suitable here?
> What controls how many arguments are fetched?  Userspace?

Yeah, it could be userspace, or a kernel tracer such as ftrace.

> If so, that's
> a nice way to oops the kernel.

I agree that a BUG_ON is probably not ideal, though I note that the 
other arch's tend to BUG as well.  Since there's no way to return an 
error from this function, what if we just used a pr_warning() and 
backfill the bogus elements of args[] with zero?

>
> Secondly, there is a 7 argument syscall - sys_syscall, which we use on
> OABI to deal with calling a syscall by number.  That really does show
> up as a unique syscall there, so if you want to parse the last argument
> to such a syscall you need to be able to read up to and including ARM
> register 7.

I think we can change it to allow up to 7 arguments in this function 
easily enough.  I think changing the tracers to support an additional 
argument should be done separatels from this patchset, however.

>
>> /*
>> [...]
>>   *
>>   * It's only valid to call this when @task is stopped for tracing on
>>   * entry to a system call, due to %TIF_SYSCALL_TRACE or %TIF_SYSCALL_AUDIT.
>>   * It's invalid to call this with @i + @n>  6; we only support system calls
>>   * taking up to 6 arguments.
>>   */
>>
>> Additionally, if you'll look at the other architectures' implementations
>> you'll see similar code.
>>
>>> In fact, how are these things supposed to deal with 64-bit arguments
>>> that straddle two registers? I think we always pack arguments such
>>> that we don't get holes in the register layout, but it might be worth
>>> checking (EABI requires 64-bit arguments to be passed in even
>>> registers).
>>
>> Hmm, I do believe that 32-bit powerpc has similar alignment issues
>> (64-bit args must be passed in odd/even pairs), but I don't see any
>> special handling of this in that architecture's
>> syscall_get/set_arguments().  So I'm wondering if the handling of this
>> is or should be handled elsewhere.  I'll keep digging on this.
>
> I did point that issue out when it first came up, and I think Roland had
> an answer for it, though I forget what it was.

Yes, see my other mail for a link to Roland's answer.

>
> The big stumbling block to this is OABI, and as I continue to be wholely
> OABI based here, it's extremely important that nothing in OABI land gets
> broken.  I don't see there's any chance of me ever getting off OABI given
> the range of platforms I have, and the restriction that EABI was designed
> to be impossible on ARMv4 architectures.

Right, I think most if not all of the issues w.r.t. OABI that were 
present in Steven's original patches have been ironed out.

I'll post the patches once I've addressed the current crop of concerns.

Thanks, Russell and Will, for the comments.

-Wade