| Message ID | 20181109031605.GF21511@altlinux.org |
|---|---|
| State | New |
| Headers | show |
| Series | Prepare for PTRACE_GET_SYSCALL_INFO | expand |
Hi Dmitry, On Fri, 2018-11-09 at 06:16 +0300, Dmitry V. Levin wrote: > syscall_get_arch() is required to be implemented on all architectures > that use tracehook_report_syscall_entry() in order to extend > the generic ptrace API with PTRACE_GET_SYSCALL_INFO request. > > Signed-off-by: Dmitry V. Levin <ldv@altlinux.org> > --- > arch/arc/include/asm/syscall.h | 6 ++++++ > include/uapi/linux/audit.h | 1 + > 2 files changed, 7 insertions(+) [snip] > diff --git a/include/uapi/linux/audit.h b/include/uapi/linux/audit.h > index 818ae690ab79..a7149ceb5b98 100644 > --- a/include/uapi/linux/audit.h > +++ b/include/uapi/linux/audit.h > @@ -375,6 +375,7 @@ enum { > > #define AUDIT_ARCH_AARCH64 (EM_AARCH64|__AUDIT_ARCH_64BIT|__AUDIT_ARCH_LE) > #define AUDIT_ARCH_ALPHA (EM_ALPHA|__AUDIT_ARCH_64BIT|__AUDIT_ARCH_LE) > +#define AUDIT_ARCH_ARC (EM_ARC) Similarly here we need to have: ---------------------------->8----------------------------- +#define AUDIT_ARCH_ARC (EM_ARC|EM_ARCV2) ---------------------------->8----------------------------- -Alexey
On Fri, Nov 9, 2018 at 6:22 AM Alexey Brodkin <alexey.brodkin@synopsys.com> wrote: > > Hi Dmitry, > > On Fri, 2018-11-09 at 06:16 +0300, Dmitry V. Levin wrote: > > syscall_get_arch() is required to be implemented on all architectures > > that use tracehook_report_syscall_entry() in order to extend > > the generic ptrace API with PTRACE_GET_SYSCALL_INFO request. > > > > Signed-off-by: Dmitry V. Levin <ldv@altlinux.org> > > --- > > arch/arc/include/asm/syscall.h | 6 ++++++ > > include/uapi/linux/audit.h | 1 + > > 2 files changed, 7 insertions(+) > > [snip] > > > diff --git a/include/uapi/linux/audit.h b/include/uapi/linux/audit.h > > index 818ae690ab79..a7149ceb5b98 100644 > > --- a/include/uapi/linux/audit.h > > +++ b/include/uapi/linux/audit.h > > @@ -375,6 +375,7 @@ enum { > > > > #define AUDIT_ARCH_AARCH64 (EM_AARCH64|__AUDIT_ARCH_64BIT|__AUDIT_ARCH_LE) > > #define AUDIT_ARCH_ALPHA (EM_ALPHA|__AUDIT_ARCH_64BIT|__AUDIT_ARCH_LE) > > +#define AUDIT_ARCH_ARC (EM_ARC) > > Similarly here we need to have: > ---------------------------->8----------------------------- > +#define AUDIT_ARCH_ARC (EM_ARC|EM_ARCV2) > ---------------------------->8----------------------------- > Huh? How does the bitwise or of two ELF machine codes make any sense?
Hi Andy, On Fri, 2018-11-09 at 07:17 -0800, Andy Lutomirski wrote: > On Fri, Nov 9, 2018 at 6:22 AM Alexey Brodkin > <alexey.brodkin@synopsys.com> wrote: > > Hi Dmitry, > > > > On Fri, 2018-11-09 at 06:16 +0300, Dmitry V. Levin wrote: > > > syscall_get_arch() is required to be implemented on all architectures > > > that use tracehook_report_syscall_entry() in order to extend > > > the generic ptrace API with PTRACE_GET_SYSCALL_INFO request. > > > > > > Signed-off-by: Dmitry V. Levin <ldv@altlinux.org> > > > --- > > > arch/arc/include/asm/syscall.h | 6 ++++++ > > > include/uapi/linux/audit.h | 1 + > > > 2 files changed, 7 insertions(+) > > > > [snip] > > > > > diff --git a/include/uapi/linux/audit.h b/include/uapi/linux/audit.h > > > index 818ae690ab79..a7149ceb5b98 100644 > > > --- a/include/uapi/linux/audit.h > > > +++ b/include/uapi/linux/audit.h > > > @@ -375,6 +375,7 @@ enum { > > > > > > #define AUDIT_ARCH_AARCH64 (EM_AARCH64|__AUDIT_ARCH_64BIT|__AUDIT_ARCH_LE) > > > #define AUDIT_ARCH_ALPHA (EM_ALPHA|__AUDIT_ARCH_64BIT|__AUDIT_ARCH_LE) > > > +#define AUDIT_ARCH_ARC (EM_ARC) > > > > Similarly here we need to have: > > ---------------------------->8----------------------------- > > +#define AUDIT_ARCH_ARC (EM_ARC|EM_ARCV2) > > ---------------------------->8----------------------------- > > > > Huh? How does the bitwise or of two ELF machine codes make any sense? Oops... I didn't read examples of AUDIT_ARCH_ALPHA above :( Indeed that was stupid. But what would be a proper fix then? Something like that? ---------------------------->8----------------------------- #define AUDIT_ARCH_ARC (EM_ARC) #define AUDIT_ARCH_ARCV2 (EM_ARCV2) static inline int syscall_get_arch(void) { #ifdef __ARC700__ return AUDIT_ARCH_ARC; #else return AUDIT_ARCH_ARCV2; #endif } ---------------------------->8----------------------------- -Alexey
> On Nov 9, 2018, at 7:27 AM, Alexey Brodkin <alexey.brodkin@synopsys.com> wrote: > > Hi Andy, > >> On Fri, 2018-11-09 at 07:17 -0800, Andy Lutomirski wrote: >> On Fri, Nov 9, 2018 at 6:22 AM Alexey Brodkin >> <alexey.brodkin@synopsys.com> wrote: >>> Hi Dmitry, >>> >>>> On Fri, 2018-11-09 at 06:16 +0300, Dmitry V. Levin wrote: >>>> syscall_get_arch() is required to be implemented on all architectures >>>> that use tracehook_report_syscall_entry() in order to extend >>>> the generic ptrace API with PTRACE_GET_SYSCALL_INFO request. >>>> >>>> Signed-off-by: Dmitry V. Levin <ldv@altlinux.org> >>>> --- >>>> arch/arc/include/asm/syscall.h | 6 ++++++ >>>> include/uapi/linux/audit.h | 1 + >>>> 2 files changed, 7 insertions(+) >>> >>> [snip] >>> >>>> diff --git a/include/uapi/linux/audit.h b/include/uapi/linux/audit.h >>>> index 818ae690ab79..a7149ceb5b98 100644 >>>> --- a/include/uapi/linux/audit.h >>>> +++ b/include/uapi/linux/audit.h >>>> @@ -375,6 +375,7 @@ enum { >>>> >>>> #define AUDIT_ARCH_AARCH64 (EM_AARCH64|__AUDIT_ARCH_64BIT|__AUDIT_ARCH_LE) >>>> #define AUDIT_ARCH_ALPHA (EM_ALPHA|__AUDIT_ARCH_64BIT|__AUDIT_ARCH_LE) >>>> +#define AUDIT_ARCH_ARC (EM_ARC) >>> >>> Similarly here we need to have: >>> ---------------------------->8----------------------------- >>> +#define AUDIT_ARCH_ARC (EM_ARC|EM_ARCV2) >>> ---------------------------->8----------------------------- >>> >> >> Huh? How does the bitwise or of two ELF machine codes make any sense? > > Oops... I didn't read examples of AUDIT_ARCH_ALPHA above :( > Indeed that was stupid. > > But what would be a proper fix then? > > Something like that? > ---------------------------->8----------------------------- > #define AUDIT_ARCH_ARC (EM_ARC) > #define AUDIT_ARCH_ARCV2 (EM_ARCV2) > > > static inline int syscall_get_arch(void) > { > #ifdef __ARC700__ > return AUDIT_ARCH_ARC; > #else > return AUDIT_ARCH_ARCV2; > #endif > } > ---------------------------->8----------------------------- > Maybe, but I know basically nothing about ARC. Is the syscall numbering or calling convention different on ARC vs ARCv2?
Hi Andy, On Fri, 2018-11-09 at 07:56 -0800, Andy Lutomirski wrote: > > On Nov 9, 2018, at 7:27 AM, Alexey Brodkin <alexey.brodkin@synopsys.com> wrote: > > > > Hi Andy, > > > > > On Fri, 2018-11-09 at 07:17 -0800, Andy Lutomirski wrote: > > > On Fri, Nov 9, 2018 at 6:22 AM Alexey Brodkin > > > <alexey.brodkin@synopsys.com> wrote: > > > > Hi Dmitry, > > > > > > > > > On Fri, 2018-11-09 at 06:16 +0300, Dmitry V. Levin wrote: > > > > > syscall_get_arch() is required to be implemented on all architectures > > > > > that use tracehook_report_syscall_entry() in order to extend > > > > > the generic ptrace API with PTRACE_GET_SYSCALL_INFO request. > > > > > > > > > > Signed-off-by: Dmitry V. Levin <ldv@altlinux.org> > > > > > --- > > > > > arch/arc/include/asm/syscall.h | 6 ++++++ > > > > > include/uapi/linux/audit.h | 1 + > > > > > 2 files changed, 7 insertions(+) > > > > > > > > [snip] > > > > > > > > > diff --git a/include/uapi/linux/audit.h b/include/uapi/linux/audit.h > > > > > index 818ae690ab79..a7149ceb5b98 100644 > > > > > --- a/include/uapi/linux/audit.h > > > > > +++ b/include/uapi/linux/audit.h > > > > > @@ -375,6 +375,7 @@ enum { > > > > > > > > > > #define AUDIT_ARCH_AARCH64 (EM_AARCH64|__AUDIT_ARCH_64BIT|__AUDIT_ARCH_LE) > > > > > #define AUDIT_ARCH_ALPHA (EM_ALPHA|__AUDIT_ARCH_64BIT|__AUDIT_ARCH_LE) > > > > > +#define AUDIT_ARCH_ARC (EM_ARC) > > > > > > > > Similarly here we need to have: > > > > ---------------------------->8----------------------------- > > > > +#define AUDIT_ARCH_ARC (EM_ARC|EM_ARCV2) > > > > ---------------------------->8----------------------------- > > > > > > > > > > Huh? How does the bitwise or of two ELF machine codes make any sense? > > > > Oops... I didn't read examples of AUDIT_ARCH_ALPHA above :( > > Indeed that was stupid. > > > > But what would be a proper fix then? > > > > Something like that? > > ---------------------------->8----------------------------- > > #define AUDIT_ARCH_ARC (EM_ARC) > > #define AUDIT_ARCH_ARCV2 (EM_ARCV2) > > > > > > static inline int syscall_get_arch(void) > > { > > #ifdef __ARC700__ > > return AUDIT_ARCH_ARC; > > #else > > return AUDIT_ARCH_ARCV2; > > #endif > > } > > ---------------------------->8----------------------------- > > > > Maybe, but I know basically nothing about ARC. Is the syscall numbering or calling convention different on ARC vs ARCv2? Syscall numbering should be the same as we use UAPI for both ARCompact (AKA ARCv1) and ARCv2. As for calling convention I think it indeed differs. Note ARCompact and ARCv2 ISAs are binary incompatible! Even though assembly look pretty much the same (sans instructions available only for either ARCompact or ARCv2) encodings are different so in that sense these are completely different architectures. Also I'm wondering what could be other cases for use of syscall_get_arch(). So I'd say it's better to report different values for ARC ISAs. And given we use the same values as in Binutils IMHO it would be good to not mix IDs here. -Alexey
On Fri, Nov 9, 2018 at 8:11 AM Alexey Brodkin <alexey.brodkin@synopsys.com> wrote: > > Hi Andy, > > On Fri, 2018-11-09 at 07:56 -0800, Andy Lutomirski wrote: > > > On Nov 9, 2018, at 7:27 AM, Alexey Brodkin <alexey.brodkin@synopsys.com> wrote: > > > > > > Hi Andy, > > > > > > > On Fri, 2018-11-09 at 07:17 -0800, Andy Lutomirski wrote: > > > > On Fri, Nov 9, 2018 at 6:22 AM Alexey Brodkin > > > > <alexey.brodkin@synopsys.com> wrote: > > > > > Hi Dmitry, > > > > > > > > > > > On Fri, 2018-11-09 at 06:16 +0300, Dmitry V. Levin wrote: > > > > > > syscall_get_arch() is required to be implemented on all architectures > > > > > > that use tracehook_report_syscall_entry() in order to extend > > > > > > the generic ptrace API with PTRACE_GET_SYSCALL_INFO request. > > > > > > > > > > > > Signed-off-by: Dmitry V. Levin <ldv@altlinux.org> > > > > > > --- > > > > > > arch/arc/include/asm/syscall.h | 6 ++++++ > > > > > > include/uapi/linux/audit.h | 1 + > > > > > > 2 files changed, 7 insertions(+) > > > > > > > > > > [snip] > > > > > > > > > > > diff --git a/include/uapi/linux/audit.h b/include/uapi/linux/audit.h > > > > > > index 818ae690ab79..a7149ceb5b98 100644 > > > > > > --- a/include/uapi/linux/audit.h > > > > > > +++ b/include/uapi/linux/audit.h > > > > > > @@ -375,6 +375,7 @@ enum { > > > > > > > > > > > > #define AUDIT_ARCH_AARCH64 (EM_AARCH64|__AUDIT_ARCH_64BIT|__AUDIT_ARCH_LE) > > > > > > #define AUDIT_ARCH_ALPHA (EM_ALPHA|__AUDIT_ARCH_64BIT|__AUDIT_ARCH_LE) > > > > > > +#define AUDIT_ARCH_ARC (EM_ARC) > > > > > > > > > > Similarly here we need to have: > > > > > ---------------------------->8----------------------------- > > > > > +#define AUDIT_ARCH_ARC (EM_ARC|EM_ARCV2) > > > > > ---------------------------->8----------------------------- > > > > > > > > > > > > > Huh? How does the bitwise or of two ELF machine codes make any sense? > > > > > > Oops... I didn't read examples of AUDIT_ARCH_ALPHA above :( > > > Indeed that was stupid. > > > > > > But what would be a proper fix then? > > > > > > Something like that? > > > ---------------------------->8----------------------------- > > > #define AUDIT_ARCH_ARC (EM_ARC) > > > #define AUDIT_ARCH_ARCV2 (EM_ARCV2) > > > > > > > > > static inline int syscall_get_arch(void) > > > { > > > #ifdef __ARC700__ > > > return AUDIT_ARCH_ARC; > > > #else > > > return AUDIT_ARCH_ARCV2; > > > #endif > > > } > > > ---------------------------->8----------------------------- > > > > > > > Maybe, but I know basically nothing about ARC. Is the syscall numbering or calling convention different on ARC vs ARCv2? > > Syscall numbering should be the same as we use UAPI for both ARCompact (AKA ARCv1) > and ARCv2. As for calling convention I think it indeed differs. > > Note ARCompact and ARCv2 ISAs are binary incompatible! > > Even though assembly look pretty much the same (sans instructions > available only for either ARCompact or ARCv2) encodings are different so > in that sense these are completely different architectures. > > Also I'm wondering what could be other cases for use of syscall_get_arch(). The intent of syscall_get_arch() is that the tuple: (arch, nr, arg1, ..., arg6) fully identifies a system call and its arguments. So it sounds like we do indeed need to arch values. > > So I'd say it's better to report different values for ARC ISAs. > And given we use the same values as in Binutils IMHO it would be good > to not mix IDs here. > > -Alexey
On 11/8/18 7:16 PM, Dmitry V. Levin wrote: > syscall_get_arch() is required to be implemented on all architectures > that use tracehook_report_syscall_entry() in order to extend > the generic ptrace API with PTRACE_GET_SYSCALL_INFO request. > > Signed-off-by: Dmitry V. Levin <ldv@altlinux.org> > --- > arch/arc/include/asm/syscall.h | 6 ++++++ > include/uapi/linux/audit.h | 1 + > 2 files changed, 7 insertions(+) > > diff --git a/arch/arc/include/asm/syscall.h b/arch/arc/include/asm/syscall.h > index 29de09804306..5662778a7411 100644 > --- a/arch/arc/include/asm/syscall.h > +++ b/arch/arc/include/asm/syscall.h > @@ -9,6 +9,7 @@ > #ifndef _ASM_ARC_SYSCALL_H > #define _ASM_ARC_SYSCALL_H 1 > > +#include <uapi/linux/audit.h> > #include <linux/err.h> > #include <linux/sched.h> > #include <asm/unistd.h> > @@ -68,4 +69,9 @@ syscall_get_arguments(struct task_struct *task, struct pt_regs *regs, > } > } > > +static inline int syscall_get_arch(void) > +{ > + return AUDIT_ARCH_ARC; > +} > + Does ptrace (or user of this API) need a unique value per arch. Otherwise instead of adding the boilerplate code to all arches, they could simply define AUDIT_ARCH and common code could return it. Also the EM_xxx are not there in include/uapi/linux/elf.h to begin with since libc elf.h already defines them. > #endif > diff --git a/include/uapi/linux/audit.h b/include/uapi/linux/audit.h > index 818ae690ab79..a7149ceb5b98 100644 > --- a/include/uapi/linux/audit.h > +++ b/include/uapi/linux/audit.h > @@ -375,6 +375,7 @@ enum { > > #define AUDIT_ARCH_AARCH64 (EM_AARCH64|__AUDIT_ARCH_64BIT|__AUDIT_ARCH_LE) > #define AUDIT_ARCH_ALPHA (EM_ALPHA|__AUDIT_ARCH_64BIT|__AUDIT_ARCH_LE) > +#define AUDIT_ARCH_ARC (EM_ARC) > #define AUDIT_ARCH_ARM (EM_ARM|__AUDIT_ARCH_LE) > #define AUDIT_ARCH_ARMEB (EM_ARM) > #define AUDIT_ARCH_CRIS (EM_CRIS|__AUDIT_ARCH_LE) So I don't have the context of this patch (or coverletter) but what exactly are we trying to do with this (adding LE to audit) - what happens when an arch is capable of either and is say built for BE ?
> On Nov 9, 2018, at 8:50 AM, Vineet Gupta <vineet.gupta1@synopsys.com> wrote: > >> On 11/8/18 7:16 PM, Dmitry V. Levin wrote: >> syscall_get_arch() is required to be implemented on all architectures >> that use tracehook_report_syscall_entry() in order to extend >> the generic ptrace API with PTRACE_GET_SYSCALL_INFO request. >> >> Signed-off-by: Dmitry V. Levin <ldv@altlinux.org> >> --- >> arch/arc/include/asm/syscall.h | 6 ++++++ >> include/uapi/linux/audit.h | 1 + >> 2 files changed, 7 insertions(+) >> >> diff --git a/arch/arc/include/asm/syscall.h b/arch/arc/include/asm/syscall.h >> index 29de09804306..5662778a7411 100644 >> --- a/arch/arc/include/asm/syscall.h >> +++ b/arch/arc/include/asm/syscall.h >> @@ -9,6 +9,7 @@ >> #ifndef _ASM_ARC_SYSCALL_H >> #define _ASM_ARC_SYSCALL_H 1 >> >> +#include <uapi/linux/audit.h> >> #include <linux/err.h> >> #include <linux/sched.h> >> #include <asm/unistd.h> >> @@ -68,4 +69,9 @@ syscall_get_arguments(struct task_struct *task, struct pt_regs *regs, >> } >> } >> >> +static inline int syscall_get_arch(void) >> +{ >> + return AUDIT_ARCH_ARC; >> +} >> + > > Does ptrace (or user of this API) need a unique value per arch. Otherwise instead > of adding the boilerplate code to all arches, they could simply define AUDIT_ARCH > and common code could return it. Also the EM_xxx are not there in > include/uapi/linux/elf.h to begin with since libc elf.h already defines them. A lot of architectures allow multiple audit_arches at runtime due to compat support and similar features, so it really does want to be a function. The goal of this patch set is to get it supported everywhere. >> #endif >> diff --git a/include/uapi/linux/audit.h b/include/uapi/linux/audit.h >> index 818ae690ab79..a7149ceb5b98 100644 >> --- a/include/uapi/linux/audit.h >> +++ b/include/uapi/linux/audit.h >> @@ -375,6 +375,7 @@ enum { >> >> #define AUDIT_ARCH_AARCH64 (EM_AARCH64|__AUDIT_ARCH_64BIT|__AUDIT_ARCH_LE) >> #define AUDIT_ARCH_ALPHA (EM_ALPHA|__AUDIT_ARCH_64BIT|__AUDIT_ARCH_LE) >> +#define AUDIT_ARCH_ARC (EM_ARC) >> #define AUDIT_ARCH_ARM (EM_ARM|__AUDIT_ARCH_LE) >> #define AUDIT_ARCH_ARMEB (EM_ARM) >> #define AUDIT_ARCH_CRIS (EM_CRIS|__AUDIT_ARCH_LE) > > So I don't have the context of this patch (or coverletter) but what exactly are we > trying to do with this (adding LE to audit) - what happens when an arch is > capable of either and is say built for BE ? The primary intent is that the triple (audit_arch, syscall_nr, arg1, ..., arg6) should describe what system call is being called and what its arguments are. I’m personally not sure what, if any, technical value there is in the LE bit. I do think it makes sense for BE and LE to have different values.
On 11/9/18 11:03 AM, Andy Lutomirski wrote: >> Does ptrace (or user of this API) need a unique value per arch. Otherwise instead >> of adding the boilerplate code to all arches, they could simply define AUDIT_ARCH >> and common code could return it. Also the EM_xxx are not there in >> include/uapi/linux/elf.h to begin with since libc elf.h already defines them. >> > A lot of architectures allow multiple audit_arches at runtime due to compat support and similar features, so it really does want to be a function. The goal of this patch set is to get it supported everywhere. > I was wondering if we could have a common syscall_get_arch() simply returning an per-arch AUDIT_ARCH. But seems like the function itself needs to be per arch anyways due to the nuances above.
diff --git a/arch/arc/include/asm/syscall.h b/arch/arc/include/asm/syscall.h index 29de09804306..5662778a7411 100644 --- a/arch/arc/include/asm/syscall.h +++ b/arch/arc/include/asm/syscall.h @@ -9,6 +9,7 @@ #ifndef _ASM_ARC_SYSCALL_H #define _ASM_ARC_SYSCALL_H 1 +#include <uapi/linux/audit.h> #include <linux/err.h> #include <linux/sched.h> #include <asm/unistd.h> @@ -68,4 +69,9 @@ syscall_get_arguments(struct task_struct *task, struct pt_regs *regs, } } +static inline int syscall_get_arch(void) +{ + return AUDIT_ARCH_ARC; +} + #endif diff --git a/include/uapi/linux/audit.h b/include/uapi/linux/audit.h index 818ae690ab79..a7149ceb5b98 100644 --- a/include/uapi/linux/audit.h +++ b/include/uapi/linux/audit.h @@ -375,6 +375,7 @@ enum { #define AUDIT_ARCH_AARCH64 (EM_AARCH64|__AUDIT_ARCH_64BIT|__AUDIT_ARCH_LE) #define AUDIT_ARCH_ALPHA (EM_ALPHA|__AUDIT_ARCH_64BIT|__AUDIT_ARCH_LE) +#define AUDIT_ARCH_ARC (EM_ARC) #define AUDIT_ARCH_ARM (EM_ARM|__AUDIT_ARCH_LE) #define AUDIT_ARCH_ARMEB (EM_ARM) #define AUDIT_ARCH_CRIS (EM_CRIS|__AUDIT_ARCH_LE)
syscall_get_arch() is required to be implemented on all architectures that use tracehook_report_syscall_entry() in order to extend the generic ptrace API with PTRACE_GET_SYSCALL_INFO request. Signed-off-by: Dmitry V. Levin <ldv@altlinux.org> --- arch/arc/include/asm/syscall.h | 6 ++++++ include/uapi/linux/audit.h | 1 + 2 files changed, 7 insertions(+)