| Message ID | 1477081997-4770-17-git-send-email-ynorov@caviumnetworks.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Oct 21, 2016 at 11:33:15PM +0300, Yury Norov wrote: > New aarch32 ptrace syscall handler is introduced to avoid run-time > detection of the task type. What's wrong with the run-time detection? If it's just to avoid a negligible overhead, I would rather keep the code simpler by avoiding duplicating the generic compat_sys_ptrace().
On Mon, Dec 05, 2016 at 04:34:23PM +0000, Catalin Marinas wrote: > On Fri, Oct 21, 2016 at 11:33:15PM +0300, Yury Norov wrote: > > New aarch32 ptrace syscall handler is introduced to avoid run-time > > detection of the task type. > > What's wrong with the run-time detection? If it's just to avoid a > negligible overhead, I would rather keep the code simpler by avoiding > duplicating the generic compat_sys_ptrace(). Nothing wrong. This is how Arnd asked me to do. You already asked this question: http://lkml.iu.edu/hypermail/linux/kernel/1604.3/00930.html If it's still looking weird to you, I can switch back to runtime ptrace. But I'd like to see Arnd's opinion. Yury.
On Tue, Dec 06, 2016 at 11:55:08AM +0530, Yury Norov wrote: > On Mon, Dec 05, 2016 at 04:34:23PM +0000, Catalin Marinas wrote: > > On Fri, Oct 21, 2016 at 11:33:15PM +0300, Yury Norov wrote: > > > New aarch32 ptrace syscall handler is introduced to avoid run-time > > > detection of the task type. > > > > What's wrong with the run-time detection? If it's just to avoid a > > negligible overhead, I would rather keep the code simpler by avoiding > > duplicating the generic compat_sys_ptrace(). > > Nothing wrong. This is how Arnd asked me to do. You already asked this > question: http://lkml.iu.edu/hypermail/linux/kernel/1604.3/00930.html > > If it's still looking weird to you, I can switch back to runtime > ptrace. But I'd like to see Arnd's opinion. This is the Arnd's email: https://patchwork.kernel.org/patch/7980521/ Yury.
On Tue, Dec 06, 2016 at 11:55:08AM +0530, Yury Norov wrote: > On Mon, Dec 05, 2016 at 04:34:23PM +0000, Catalin Marinas wrote: > > On Fri, Oct 21, 2016 at 11:33:15PM +0300, Yury Norov wrote: > > > New aarch32 ptrace syscall handler is introduced to avoid run-time > > > detection of the task type. > > > > What's wrong with the run-time detection? If it's just to avoid a > > negligible overhead, I would rather keep the code simpler by avoiding > > duplicating the generic compat_sys_ptrace(). > > Nothing wrong. This is how Arnd asked me to do. You already asked this > question: http://lkml.iu.edu/hypermail/linux/kernel/1604.3/00930.html Hmm, I completely forgot about this ;). There is still an advantage to doing run-time checking if we avoid touching core code (less acks to gather and less code duplication). Let's see what Arnd says but the initial patch looked simpler.
On Wednesday, December 7, 2016 4:59:13 PM CET Catalin Marinas wrote: > On Tue, Dec 06, 2016 at 11:55:08AM +0530, Yury Norov wrote: > > On Mon, Dec 05, 2016 at 04:34:23PM +0000, Catalin Marinas wrote: > > > On Fri, Oct 21, 2016 at 11:33:15PM +0300, Yury Norov wrote: > > > > New aarch32 ptrace syscall handler is introduced to avoid run-time > > > > detection of the task type. > > > > > > What's wrong with the run-time detection? If it's just to avoid a > > > negligible overhead, I would rather keep the code simpler by avoiding > > > duplicating the generic compat_sys_ptrace(). > > > > Nothing wrong. This is how Arnd asked me to do. You already asked this > > question: http://lkml.iu.edu/hypermail/linux/kernel/1604.3/00930.html > > Hmm, I completely forgot about this ;). There is still an advantage to > doing run-time checking if we avoid touching core code (less acks to > gather and less code duplication). > > Let's see what Arnd says but the initial patch looked simpler. I don't currently have either version of the patch in my inbox (the archive is on a different machine), but in general I'd still think it's best to avoid the runtime check for aarch64-ilp32 altogether. I'd have to look at the overall kernel source to see if it's worth avoiding one or two instances though, or if there are an overwhelming number of other checks that we can't avoid at all. Regarding ptrace, I notice that arch/tile doesn't even use the compat entry point for its ilp32 user space on 64-bit kernels, it just calls the regular 64-bit one. Would that help here? Arnd
On Wed, Dec 07, 2016 at 09:40:13PM +0100, Arnd Bergmann wrote: > On Wednesday, December 7, 2016 4:59:13 PM CET Catalin Marinas wrote: > > On Tue, Dec 06, 2016 at 11:55:08AM +0530, Yury Norov wrote: > > > On Mon, Dec 05, 2016 at 04:34:23PM +0000, Catalin Marinas wrote: > > > > On Fri, Oct 21, 2016 at 11:33:15PM +0300, Yury Norov wrote: > > > > > New aarch32 ptrace syscall handler is introduced to avoid run-time > > > > > detection of the task type. > > > > > > > > What's wrong with the run-time detection? If it's just to avoid a > > > > negligible overhead, I would rather keep the code simpler by avoiding > > > > duplicating the generic compat_sys_ptrace(). > > > > > > Nothing wrong. This is how Arnd asked me to do. You already asked this > > > question: http://lkml.iu.edu/hypermail/linux/kernel/1604.3/00930.html > > > > Hmm, I completely forgot about this ;). There is still an advantage to > > doing run-time checking if we avoid touching core code (less acks to > > gather and less code duplication). > > > > Let's see what Arnd says but the initial patch looked simpler. > > I don't currently have either version of the patch in my inbox > (the archive is on a different machine), but in general I'd still > think it's best to avoid the runtime check for aarch64-ilp32 > altogether. I'd have to look at the overall kernel source to > see if it's worth avoiding one or two instances though, or > if there are an overwhelming number of other checks that we > can't avoid at all. Just in case you haven't found them already, current version: https://marc.info/?l=linux-arm-kernel&m=147708276818318&w=2 Original version: https://patchwork.kernel.org/patch/7980521/ The old one looks more readable and given that ptrace is not really a fast path, I'm not two worried about run-time checks > Regarding ptrace, I notice that arch/tile doesn't even use > the compat entry point for its ilp32 user space on 64-bit > kernels, it just calls the regular 64-bit one. Would that > help here? I don't know whether it would work, we have incompatible siginfo_t on AArch64/ILP32.
On Wed, Dec 07, 2016 at 09:40:13PM +0100, Arnd Bergmann wrote: > On Wednesday, December 7, 2016 4:59:13 PM CET Catalin Marinas wrote: > > On Tue, Dec 06, 2016 at 11:55:08AM +0530, Yury Norov wrote: > > > On Mon, Dec 05, 2016 at 04:34:23PM +0000, Catalin Marinas wrote: > > > > On Fri, Oct 21, 2016 at 11:33:15PM +0300, Yury Norov wrote: > > > > > New aarch32 ptrace syscall handler is introduced to avoid run-time > > > > > detection of the task type. > > > > > > > > What's wrong with the run-time detection? If it's just to avoid a > > > > negligible overhead, I would rather keep the code simpler by avoiding > > > > duplicating the generic compat_sys_ptrace(). > > > > > > Nothing wrong. This is how Arnd asked me to do. You already asked this > > > question: http://lkml.iu.edu/hypermail/linux/kernel/1604.3/00930.html > > > > Hmm, I completely forgot about this ;). There is still an advantage to > > doing run-time checking if we avoid touching core code (less acks to > > gather and less code duplication). > > > > Let's see what Arnd says but the initial patch looked simpler. > > I don't currently have either version of the patch in my inbox > (the archive is on a different machine), but in general I'd still > think it's best to avoid the runtime check for aarch64-ilp32 > altogether. I'd have to look at the overall kernel source to > see if it's worth avoiding one or two instances though, or > if there are an overwhelming number of other checks that we > can't avoid at all. > > Regarding ptrace, I notice that arch/tile doesn't even use > the compat entry point for its ilp32 user space on 64-bit > kernels, it just calls the regular 64-bit one. Would that > help here? ILP32 tasks has unique context that is not like aarch64 or aarch32, so we have to have unique ptrace handler. I prepared the patch for ptrace with runtime ABI detection, as Catalin said, see there: https://github.com/norov/linux/commit/1f66dc22a4450b192e83458f2c3cc0e79f53e670 If it's OK, I'd like to update submission. Yury
On Fri, Jan 06, 2017 at 02:10:03AM +0530, Yury Norov wrote: > On Wed, Dec 07, 2016 at 09:40:13PM +0100, Arnd Bergmann wrote: > > On Wednesday, December 7, 2016 4:59:13 PM CET Catalin Marinas wrote: > > > On Tue, Dec 06, 2016 at 11:55:08AM +0530, Yury Norov wrote: > > > > On Mon, Dec 05, 2016 at 04:34:23PM +0000, Catalin Marinas wrote: > > > > > On Fri, Oct 21, 2016 at 11:33:15PM +0300, Yury Norov wrote: > > > > > > New aarch32 ptrace syscall handler is introduced to avoid run-time > > > > > > detection of the task type. > > > > > > > > > > What's wrong with the run-time detection? If it's just to avoid a > > > > > negligible overhead, I would rather keep the code simpler by avoiding > > > > > duplicating the generic compat_sys_ptrace(). > > > > > > > > Nothing wrong. This is how Arnd asked me to do. You already asked this > > > > question: http://lkml.iu.edu/hypermail/linux/kernel/1604.3/00930.html > > > > > > Hmm, I completely forgot about this ;). There is still an advantage to > > > doing run-time checking if we avoid touching core code (less acks to > > > gather and less code duplication). > > > > > > Let's see what Arnd says but the initial patch looked simpler. > > > > I don't currently have either version of the patch in my inbox > > (the archive is on a different machine), but in general I'd still > > think it's best to avoid the runtime check for aarch64-ilp32 > > altogether. I'd have to look at the overall kernel source to > > see if it's worth avoiding one or two instances though, or > > if there are an overwhelming number of other checks that we > > can't avoid at all. > > > > Regarding ptrace, I notice that arch/tile doesn't even use > > the compat entry point for its ilp32 user space on 64-bit > > kernels, it just calls the regular 64-bit one. Would that > > help here? > > ILP32 tasks has unique context that is not like aarch64 or aarch32, > so we have to have unique ptrace handler. I prepared the patch for > ptrace with runtime ABI detection, as Catalin said, see there: > https://github.com/norov/linux/commit/1f66dc22a4450b192e83458f2c3cc0e79f53e670 > > If it's OK, I'd like to update submission. This looks better to me (and even better if you no longer need to touch the generic ptrace code).
diff --git a/arch/arm64/include/asm/unistd32.h b/arch/arm64/include/asm/unistd32.h index b7e8ef1..6da7cbd 100644 --- a/arch/arm64/include/asm/unistd32.h +++ b/arch/arm64/include/asm/unistd32.h @@ -74,7 +74,7 @@ __SYSCALL(__NR_getuid, sys_getuid16) /* 25 was sys_stime */ __SYSCALL(25, sys_ni_syscall) #define __NR_ptrace 26 -__SYSCALL(__NR_ptrace, compat_sys_ptrace) +__SYSCALL(__NR_ptrace, compat_sys_aarch32_ptrace) /* 27 was sys_alarm */ __SYSCALL(27, sys_ni_syscall) /* 28 was sys_fstat */ diff --git a/arch/arm64/kernel/ptrace.c b/arch/arm64/kernel/ptrace.c index 1d075ed..ac542c9 100644 --- a/arch/arm64/kernel/ptrace.c +++ b/arch/arm64/kernel/ptrace.c @@ -29,6 +29,7 @@ #include <linux/user.h> #include <linux/seccomp.h> #include <linux/security.h> +#include <linux/syscalls.h> #include <linux/init.h> #include <linux/signal.h> #include <linux/uaccess.h> @@ -40,6 +41,7 @@ #include <asm/debug-monitors.h> #include <asm/pgtable.h> +#include <asm/signal32_common.h> #include <asm/syscall.h> #include <asm/traps.h> #include <asm/system_misc.h> @@ -1215,7 +1217,7 @@ static int compat_ptrace_sethbpregs(struct task_struct *tsk, compat_long_t num, } #endif /* CONFIG_HAVE_HW_BREAKPOINT */ -long compat_arch_ptrace(struct task_struct *child, compat_long_t request, +static long compat_a32_ptrace(struct task_struct *child, compat_long_t request, compat_ulong_t caddr, compat_ulong_t cdata) { unsigned long addr = caddr; @@ -1292,8 +1294,95 @@ long compat_arch_ptrace(struct task_struct *child, compat_long_t request, return ret; } + +COMPAT_SYSCALL_DEFINE4(aarch32_ptrace, compat_long_t, request, compat_long_t, pid, + compat_long_t, addr, compat_long_t, data) +{ + struct task_struct *child; + long ret; + + if (request == PTRACE_TRACEME) { + ret = ptrace_traceme(); + goto out; + } + + child = ptrace_get_task_struct(pid); + if (IS_ERR(child)) { + ret = PTR_ERR(child); + goto out; + } + + if (request == PTRACE_ATTACH || request == PTRACE_SEIZE) { + ret = ptrace_attach(child, request, addr, data); + goto out_put_task_struct; + } + + ret = ptrace_check_attach(child, request == PTRACE_KILL || + request == PTRACE_INTERRUPT); + if (!ret) { + ret = compat_a32_ptrace(child, request, addr, data); + if (ret || request != PTRACE_DETACH) + ptrace_unfreeze_traced(child); + } + + out_put_task_struct: + put_task_struct(child); + out: + return ret; +} + #endif /* CONFIG_AARCH32_EL0 */ +#ifdef CONFIG_ARM64_ILP32 + +long compat_arch_ptrace(struct task_struct *child, compat_long_t request, + compat_ulong_t caddr, compat_ulong_t cdata) +{ + sigset_t new_set; + + switch (request) { + case PTRACE_GETSIGMASK: + if (caddr != sizeof(compat_sigset_t)) + return -EINVAL; + + return put_sigset_t((compat_sigset_t __user *) (u64) cdata, + &child->blocked); + + case PTRACE_SETSIGMASK: + if (caddr != sizeof(compat_sigset_t)) + return -EINVAL; + + if (get_sigset_t(&new_set, (compat_sigset_t __user *) (u64) cdata)) + return -EFAULT; + + sigdelsetmask(&new_set, sigmask(SIGKILL)|sigmask(SIGSTOP)); + + /* + * Every thread does recalc_sigpending() after resume, so + * retarget_shared_pending() and recalc_sigpending() are not + * called here. + */ + spin_lock_irq(&child->sighand->siglock); + child->blocked = new_set; + spin_unlock_irq(&child->sighand->siglock); + + return 0; + + default: + return compat_ptrace_request(child, request, caddr, cdata); + } +} + +#elif defined(CONFIG_COMPAT) + +long compat_arch_ptrace(struct task_struct *child, compat_long_t request, + compat_ulong_t caddr, compat_ulong_t cdata) +{ + return 0; +} + +#endif + const struct user_regset_view *task_user_regset_view(struct task_struct *task) { #ifdef CONFIG_AARCH32_EL0 diff --git a/arch/arm64/kernel/sys32.c b/arch/arm64/kernel/sys32.c index a40b134..3752443 100644 --- a/arch/arm64/kernel/sys32.c +++ b/arch/arm64/kernel/sys32.c @@ -38,6 +38,7 @@ asmlinkage long compat_sys_fadvise64_64_wrapper(void); asmlinkage long compat_sys_sync_file_range2_wrapper(void); asmlinkage long compat_sys_fallocate_wrapper(void); asmlinkage long compat_sys_mmap2_wrapper(void); +asmlinkage long compat_sys_aarch32_ptrace(void); #undef __SYSCALL #define __SYSCALL(nr, sym) [nr] = sym, diff --git a/include/linux/ptrace.h b/include/linux/ptrace.h index 504c98a..75887a0 100644 --- a/include/linux/ptrace.h +++ b/include/linux/ptrace.h @@ -97,6 +97,12 @@ int generic_ptrace_peekdata(struct task_struct *tsk, unsigned long addr, unsigned long data); int generic_ptrace_pokedata(struct task_struct *tsk, unsigned long addr, unsigned long data); +int ptrace_traceme(void); +struct task_struct *ptrace_get_task_struct(pid_t pid); +int ptrace_attach(struct task_struct *task, long request, + unsigned long addr, unsigned long flags); +int ptrace_check_attach(struct task_struct *child, bool ignore_state); +void ptrace_unfreeze_traced(struct task_struct *task); /** * ptrace_parent - return the task that is tracing the given task diff --git a/kernel/ptrace.c b/kernel/ptrace.c index 2a99027..5638880 100644 --- a/kernel/ptrace.c +++ b/kernel/ptrace.c @@ -138,7 +138,7 @@ static bool ptrace_freeze_traced(struct task_struct *task) return ret; } -static void ptrace_unfreeze_traced(struct task_struct *task) +void ptrace_unfreeze_traced(struct task_struct *task) { if (task->state != __TASK_TRACED) return; @@ -170,7 +170,7 @@ static void ptrace_unfreeze_traced(struct task_struct *task) * RETURNS: * 0 on success, -ESRCH if %child is not ready. */ -static int ptrace_check_attach(struct task_struct *child, bool ignore_state) +int ptrace_check_attach(struct task_struct *child, bool ignore_state) { int ret = -ESRCH; @@ -294,7 +294,7 @@ bool ptrace_may_access(struct task_struct *task, unsigned int mode) return !err; } -static int ptrace_attach(struct task_struct *task, long request, +int ptrace_attach(struct task_struct *task, long request, unsigned long addr, unsigned long flags) { @@ -408,7 +408,7 @@ static int ptrace_attach(struct task_struct *task, long request, * Performs checks and sets PT_PTRACED. * Should be used by all ptrace implementations for PTRACE_TRACEME. */ -static int ptrace_traceme(void) +int ptrace_traceme(void) { int ret = -EPERM; @@ -1057,7 +1057,7 @@ int ptrace_request(struct task_struct *child, long request, return ret; } -static struct task_struct *ptrace_get_task_struct(pid_t pid) +struct task_struct *ptrace_get_task_struct(pid_t pid) { struct task_struct *child;