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

Groups > linux.kernel > #1481709 > unrolled thread

Re: [RFC PATCH 2/3] tracing/syscalls: add handling for compat tasks

Started byAndy Lutomirski <luto@amacapital.net>
First post2016-09-12 19:40 +0200
Last post2016-09-13 21:10 +0200
Articles 3 — 2 participants

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: [RFC PATCH 2/3] tracing/syscalls: add handling for compat tasks Andy Lutomirski <luto@amacapital.net> - 2016-09-12 19:40 +0200
    Re: [RFC PATCH 2/3] tracing/syscalls: add handling for compat tasks Marcin Nowakowski <marcin.nowakowski@imgtec.com> - 2016-09-13 07:50 +0200
      Re: [RFC PATCH 2/3] tracing/syscalls: add handling for compat tasks Andy Lutomirski <luto@amacapital.net> - 2016-09-13 21:10 +0200

#1481709 — Re: [RFC PATCH 2/3] tracing/syscalls: add handling for compat tasks

FromAndy Lutomirski <luto@amacapital.net>
Date2016-09-12 19:40 +0200
SubjectRe: [RFC PATCH 2/3] tracing/syscalls: add handling for compat tasks
Message-ID<sgDn4-6mO-15@gated-at.bofh.it>
On Sep 9, 2016 1:04 AM, "Marcin Nowakowski"
<marcin.nowakowski@imgtec.com> wrote:
>
> Extend the syscall tracing subsystem by adding a handler for compat
> tasks. For some architectures, where compat tasks' syscall numbers have
> an exclusive set of syscall numbers, this already works since the
> removal of syscall_nr.
> Architectures where the same syscall may use a different syscall number
> for compat tasks need to define ARCH_COMPAT_SYSCALL_NUMBERS_OVERLAP and
> define a method arch_trace_is_compat_syscall(struct pt_regs*) that tells
> if a current task is a compat one.
> For architectures that define ARCH_COMPAT_SYSCALL_NUMBERS_OVERLAP the
> number of trace event files is doubled and all syscall trace events are
> identified by the syscall number offset by NR_syscalls.
>
> Note that as this patch series is posted as an RFC, this currently only
> includes arch updates for MIPS and x86 (and has only been tested on
> MIPS and x86_64). I will work on updating other arch trees after this
> solution is reviewed.

I bet you didn't test x32 -- see below :)

>
> Signed-off-by: Marcin Nowakowski <marcin.nowakowski@imgtec.com>
>
> ---
>  arch/mips/kernel/ftrace.c     |   4 +-
>  arch/x86/include/asm/ftrace.h |  10 +---
>  arch/x86/kernel/ftrace.c      |  14 ++++++
>  include/linux/ftrace.h        |   2 +-
>  kernel/trace/trace.h          |  11 +++-
>  kernel/trace/trace_syscalls.c | 113 +++++++++++++++++++++++++-----------------
>  6 files changed, 94 insertions(+), 60 deletions(-)
>
> diff --git a/arch/mips/kernel/ftrace.c b/arch/mips/kernel/ftrace.c
> index 937c54b..e150cf6 100644
> --- a/arch/mips/kernel/ftrace.c
> +++ b/arch/mips/kernel/ftrace.c
> @@ -412,7 +412,7 @@ out:
>  #ifdef CONFIG_FTRACE_SYSCALLS
>
>  #ifdef CONFIG_32BIT
> -unsigned long __init arch_syscall_addr(int nr)
> +unsigned long __init arch_syscall_addr(int nr, int compat)
>  {
>         return (unsigned long)sys_call_table[nr - __NR_O32_Linux];
>  }
> @@ -420,7 +420,7 @@ unsigned long __init arch_syscall_addr(int nr)
>
>  #ifdef CONFIG_64BIT
>
> -unsigned long __init arch_syscall_addr(int nr)
> +unsigned long __init arch_syscall_addr(int nr, int compat)

bool compat?

>  {
>  #ifdef CONFIG_MIPS32_N32
>         if (nr >= __NR_N32_Linux && nr <= __NR_N32_Linux + __NR_N32_Linux_syscalls)
> diff --git a/arch/x86/include/asm/ftrace.h b/arch/x86/include/asm/ftrace.h
> index a4820d4..a24a21c 100644
> --- a/arch/x86/include/asm/ftrace.h
> +++ b/arch/x86/include/asm/ftrace.h
> @@ -47,15 +47,7 @@ int ftrace_int3_handler(struct pt_regs *regs);
>  #if defined(CONFIG_FTRACE_SYSCALLS) && defined(CONFIG_IA32_EMULATION)
>  #include <asm/compat.h>
>
> -/*
> - * Because ia32 syscalls do not map to x86_64 syscall numbers
> - * this screws up the trace output when tracing a ia32 task.
> - * Instead of reporting bogus syscalls, just do not trace them.
> - *
> - * If the user really wants these, then they should use the
> - * raw syscall tracepoints with filtering.
> - */
> -#define ARCH_TRACE_IGNORE_COMPAT_SYSCALLS 1
> +#define ARCH_COMPAT_SYSCALL_NUMBERS_OVERLAP 1
>  static inline bool arch_trace_is_compat_syscall(struct pt_regs *regs)
>  {
>         if (in_compat_syscall())

This isn't your fault obviously, but shouldn't that be in_ia32_syscall()?

[toc] | [next] | [standalone]

#1482145

FromMarcin Nowakowski <marcin.nowakowski@imgtec.com>
Date2016-09-13 07:50 +0200
Message-ID<sgOLv-5S2-1@gated-at.bofh.it>
In reply to#1481709
Hi Andy,

Thanks for your review and the comments, I'll address them in a next 
iteration. Do you have any other comments on the complete patchset?

On 12.09.2016 19:35, Andy Lutomirski wrote:
> On Sep 9, 2016 1:04 AM, "Marcin Nowakowski"
> <marcin.nowakowski@imgtec.com> wrote:
>>
>> Extend the syscall tracing subsystem by adding a handler for compat
>> tasks. For some architectures, where compat tasks' syscall numbers have
>> an exclusive set of syscall numbers, this already works since the
>> removal of syscall_nr.
>> Architectures where the same syscall may use a different syscall number
>> for compat tasks need to define ARCH_COMPAT_SYSCALL_NUMBERS_OVERLAP and
>> define a method arch_trace_is_compat_syscall(struct pt_regs*) that tells
>> if a current task is a compat one.
>> For architectures that define ARCH_COMPAT_SYSCALL_NUMBERS_OVERLAP the
>> number of trace event files is doubled and all syscall trace events are
>> identified by the syscall number offset by NR_syscalls.
>>
>> Note that as this patch series is posted as an RFC, this currently only
>> includes arch updates for MIPS and x86 (and has only been tested on
>> MIPS and x86_64). I will work on updating other arch trees after this
>> solution is reviewed.
>
> I bet you didn't test x32 -- see below :)

Indeed ... I've tried with x86_64 and 32-bit x86, but not x32 syscalls. 
I will review that part.

>>
>> Signed-off-by: Marcin Nowakowski <marcin.nowakowski@imgtec.com>
>>
>> ---
>>  arch/mips/kernel/ftrace.c     |   4 +-
>>  arch/x86/include/asm/ftrace.h |  10 +---
>>  arch/x86/kernel/ftrace.c      |  14 ++++++
>>  include/linux/ftrace.h        |   2 +-
>>  kernel/trace/trace.h          |  11 +++-
>>  kernel/trace/trace_syscalls.c | 113 +++++++++++++++++++++++++-----------------
>>  6 files changed, 94 insertions(+), 60 deletions(-)
>>
>> diff --git a/arch/mips/kernel/ftrace.c b/arch/mips/kernel/ftrace.c
>> index 937c54b..e150cf6 100644
>> --- a/arch/mips/kernel/ftrace.c
>> +++ b/arch/mips/kernel/ftrace.c
>> @@ -412,7 +412,7 @@ out:
>>  #ifdef CONFIG_FTRACE_SYSCALLS
>>
>>  #ifdef CONFIG_32BIT
>> -unsigned long __init arch_syscall_addr(int nr)
>> +unsigned long __init arch_syscall_addr(int nr, int compat)
>>  {
>>         return (unsigned long)sys_call_table[nr - __NR_O32_Linux];
>>  }
>> @@ -420,7 +420,7 @@ unsigned long __init arch_syscall_addr(int nr)
>>
>>  #ifdef CONFIG_64BIT
>>
>> -unsigned long __init arch_syscall_addr(int nr)
>> +unsigned long __init arch_syscall_addr(int nr, int compat)
>
> bool compat?

Yes, that should make the intention more clear.

>>  {
>>  #ifdef CONFIG_MIPS32_N32
>>         if (nr >= __NR_N32_Linux && nr <= __NR_N32_Linux + __NR_N32_Linux_syscalls)
>> diff --git a/arch/x86/include/asm/ftrace.h b/arch/x86/include/asm/ftrace.h
>> index a4820d4..a24a21c 100644
>> --- a/arch/x86/include/asm/ftrace.h
>> +++ b/arch/x86/include/asm/ftrace.h
>> @@ -47,15 +47,7 @@ int ftrace_int3_handler(struct pt_regs *regs);
>>  #if defined(CONFIG_FTRACE_SYSCALLS) && defined(CONFIG_IA32_EMULATION)
>>  #include <asm/compat.h>
>>
>> -/*
>> - * Because ia32 syscalls do not map to x86_64 syscall numbers
>> - * this screws up the trace output when tracing a ia32 task.
>> - * Instead of reporting bogus syscalls, just do not trace them.
>> - *
>> - * If the user really wants these, then they should use the
>> - * raw syscall tracepoints with filtering.
>> - */
>> -#define ARCH_TRACE_IGNORE_COMPAT_SYSCALLS 1
>> +#define ARCH_COMPAT_SYSCALL_NUMBERS_OVERLAP 1
>>  static inline bool arch_trace_is_compat_syscall(struct pt_regs *regs)
>>  {
>>         if (in_compat_syscall())
>
> This isn't your fault obviously, but shouldn't that be in_ia32_syscall()?

Thanks for pointing this out - I'll need to review this part of code a 
bit more.

Marcin

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

#1482718

FromAndy Lutomirski <luto@amacapital.net>
Date2016-09-13 21:10 +0200
Message-ID<sh1fI-61g-9@gated-at.bofh.it>
In reply to#1482145
On Mon, Sep 12, 2016 at 10:41 PM, Marcin Nowakowski
<marcin.nowakowski@imgtec.com> wrote:
> Hi Andy,
>
> Thanks for your review and the comments, I'll address them in a next
> iteration. Do you have any other comments on the complete patchset?

It seems reasonable to me.

>
> On 12.09.2016 19:35, Andy Lutomirski wrote:
>>
>> On Sep 9, 2016 1:04 AM, "Marcin Nowakowski"
>> <marcin.nowakowski@imgtec.com> wrote:
>>>
>>>
>>> Extend the syscall tracing subsystem by adding a handler for compat
>>> tasks. For some architectures, where compat tasks' syscall numbers have
>>> an exclusive set of syscall numbers, this already works since the
>>> removal of syscall_nr.
>>> Architectures where the same syscall may use a different syscall number
>>> for compat tasks need to define ARCH_COMPAT_SYSCALL_NUMBERS_OVERLAP and
>>> define a method arch_trace_is_compat_syscall(struct pt_regs*) that tells
>>> if a current task is a compat one.
>>> For architectures that define ARCH_COMPAT_SYSCALL_NUMBERS_OVERLAP the
>>> number of trace event files is doubled and all syscall trace events are
>>> identified by the syscall number offset by NR_syscalls.
>>>
>>> Note that as this patch series is posted as an RFC, this currently only
>>> includes arch updates for MIPS and x86 (and has only been tested on
>>> MIPS and x86_64). I will work on updating other arch trees after this
>>> solution is reviewed.
>>
>>
>> I bet you didn't test x32 -- see below :)
>
>
> Indeed ... I've tried with x86_64 and 32-bit x86, but not x32 syscalls. I
> will review that part.
>
>>>
>>> Signed-off-by: Marcin Nowakowski <marcin.nowakowski@imgtec.com>
>>>
>>> ---
>>>  arch/mips/kernel/ftrace.c     |   4 +-
>>>  arch/x86/include/asm/ftrace.h |  10 +---
>>>  arch/x86/kernel/ftrace.c      |  14 ++++++
>>>  include/linux/ftrace.h        |   2 +-
>>>  kernel/trace/trace.h          |  11 +++-
>>>  kernel/trace/trace_syscalls.c | 113
>>> +++++++++++++++++++++++++-----------------
>>>  6 files changed, 94 insertions(+), 60 deletions(-)
>>>
>>> diff --git a/arch/mips/kernel/ftrace.c b/arch/mips/kernel/ftrace.c
>>> index 937c54b..e150cf6 100644
>>> --- a/arch/mips/kernel/ftrace.c
>>> +++ b/arch/mips/kernel/ftrace.c
>>> @@ -412,7 +412,7 @@ out:
>>>  #ifdef CONFIG_FTRACE_SYSCALLS
>>>
>>>  #ifdef CONFIG_32BIT
>>> -unsigned long __init arch_syscall_addr(int nr)
>>> +unsigned long __init arch_syscall_addr(int nr, int compat)
>>>  {
>>>         return (unsigned long)sys_call_table[nr - __NR_O32_Linux];
>>>  }
>>> @@ -420,7 +420,7 @@ unsigned long __init arch_syscall_addr(int nr)
>>>
>>>  #ifdef CONFIG_64BIT
>>>
>>> -unsigned long __init arch_syscall_addr(int nr)
>>> +unsigned long __init arch_syscall_addr(int nr, int compat)
>>
>>
>> bool compat?
>
>
> Yes, that should make the intention more clear.
>
>>>  {
>>>  #ifdef CONFIG_MIPS32_N32
>>>         if (nr >= __NR_N32_Linux && nr <= __NR_N32_Linux +
>>> __NR_N32_Linux_syscalls)
>>> diff --git a/arch/x86/include/asm/ftrace.h
>>> b/arch/x86/include/asm/ftrace.h
>>> index a4820d4..a24a21c 100644
>>> --- a/arch/x86/include/asm/ftrace.h
>>> +++ b/arch/x86/include/asm/ftrace.h
>>> @@ -47,15 +47,7 @@ int ftrace_int3_handler(struct pt_regs *regs);
>>>  #if defined(CONFIG_FTRACE_SYSCALLS) && defined(CONFIG_IA32_EMULATION)
>>>  #include <asm/compat.h>
>>>
>>> -/*
>>> - * Because ia32 syscalls do not map to x86_64 syscall numbers
>>> - * this screws up the trace output when tracing a ia32 task.
>>> - * Instead of reporting bogus syscalls, just do not trace them.
>>> - *
>>> - * If the user really wants these, then they should use the
>>> - * raw syscall tracepoints with filtering.
>>> - */
>>> -#define ARCH_TRACE_IGNORE_COMPAT_SYSCALLS 1
>>> +#define ARCH_COMPAT_SYSCALL_NUMBERS_OVERLAP 1
>>>  static inline bool arch_trace_is_compat_syscall(struct pt_regs *regs)
>>>  {
>>>         if (in_compat_syscall())
>>
>>
>> This isn't your fault obviously, but shouldn't that be in_ia32_syscall()?
>
>
> Thanks for pointing this out - I'll need to review this part of code a bit
> more.
>
> Marcin



-- 
Andy Lutomirski
AMA Capital Management, LLC

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web