| Message ID | 1526318067-4964-1-git-send-email-Dave.Martin@arm.com |
|---|---|
| Headers | show |
| Series | prctl: Modernise wiring for optional prctl() calls | expand |
On Mon, May 14, 2018 at 10:14 AM, Dave Martin <Dave.Martin@arm.com> wrote: > [Reviewer note: this is a cross-arch series. To reduce spam, I have > tried not to Cc people on patches they aren't likely to care about. > The complete series can be found in the LKML or linux-arch archives.] > > The core framework for the prctl() syscall is unloved and looking > rather crusty these days. It also relies on defining ancillary > boilerplate macros for each prctl() in order to control conditional > compilation of the different prctl calls. We have better ways to > do this now. This is a nice clean-up series, thanks! Some thoughts/comments below... > This series attempts to modernise the code by defining a couple of new > Kconfig variables HAVE_PRCTL_ARCH and HAVE_ARCH_PR_SET_GET_UNALIGN to > allow architectures to provide hooks for arch-dependent prctls. > > > For now this series has had minimal testing: some basic testing of > arch-specific prctls using PR_SVE_SET_VL on arm64; build-tested on > x86; otherwise untested. > > This is not polished yet... but I'm interested to know what people > think about the approach. I'm not entirely convinced the removal of the "task not current" interface is very useful as I've found myself needing to adjust other interfaces like this to _add_ the "task not current" logic, but ... I can't really argue very hard for keeping the task args since nothing is using them... meh. I dislike this being named "prctl_arch" when everything else like this is "arch_foo...", but I do see the collision with the x86-specific "arch_prctl" syscall. Perhaps it might be better to wire things up differently, since x86's arch_prctl is actually "sys_arch_prctl" so I don't think there would be a symbol collision, and maybe the 9 options could be added to the global prctl list, with the x86 syscall getting called at the end of the new "arch_prctl"? Then the syscall could get deprecated in favor of just using prctl directly? Your new x86 arch_prctl (neé prctl_arch) could be something like: int arch_prctl(int option, unsigned long arg2, unsigned long arg3, unsigned long arg4, unsigned long arg5) { ... existing stuff in your series ... case ARCH_SET_GS: case ARCH_SET_FS: case ARCH_GET_GS: case ARCH_GET_FS: case ARCH_MAP_VDSO_X32: case ARCH_MAP_VDSO_32: case ARCH_MAP_VDSO_64: if (arg3 || arg4 || arg5) return -EINVAL; return do_arch_prctl_64(current, option, arg2); case ARCH_GET_CPUID: case ARCH_SET_CPUID: if (arg3 || arg4 || arg5) return -EINVAL; return do_arch_prctl_common(current, option, arg2); default: return -EINVAL; } } Or maybe all the logic could get ripped out of arch/x86/kernel/process*.c and put in arch/x86/kernel/sys.c directly? Perhaps this is all overkill, but I find it rather annoying that one arch's weird syscall would block "expected" naming of a cross-arch interface... -Kees -- Kees Cook Pixel Security
On Mon, May 14, 2018 at 07:28:11PM +0100, Kees Cook wrote: > On Mon, May 14, 2018 at 10:14 AM, Dave Martin <Dave.Martin@arm.com> wrote: > > [Reviewer note: this is a cross-arch series. To reduce spam, I have > > tried not to Cc people on patches they aren't likely to care about. > > The complete series can be found in the LKML or linux-arch archives.] > > > > The core framework for the prctl() syscall is unloved and looking > > rather crusty these days. It also relies on defining ancillary > > boilerplate macros for each prctl() in order to control conditional > > compilation of the different prctl calls. We have better ways to > > do this now. > > This is a nice clean-up series, thanks! Some thoughts/comments below... > > > This series attempts to modernise the code by defining a couple of new > > Kconfig variables HAVE_PRCTL_ARCH and HAVE_ARCH_PR_SET_GET_UNALIGN to > > allow architectures to provide hooks for arch-dependent prctls. > > > > > > For now this series has had minimal testing: some basic testing of > > arch-specific prctls using PR_SVE_SET_VL on arm64; build-tested on > > x86; otherwise untested. > > > > This is not polished yet... but I'm interested to know what people > > think about the approach. > > I'm not entirely convinced the removal of the "task not current" > interface is very useful as I've found myself needing to adjust other > interfaces like this to _add_ the "task not current" logic, but ... I > can't really argue very hard for keeping the task args since nothing > is using them... meh. For operations that are shared with ptrace it does make sense to have a task argument, but in the end I found it equally natural to put that only in the backend rather than having it invoked directly with an explicit argument by the prctl demux. In the case of PR_SVE_SET_VL the interface has to be massaged in different ways depending on whether it's being invokved via prctl or ptrace anyway, so I guess there was no temptation to keep the task argument on the prctl side in this case. Ptrace differs from prctl in that in the former case we can assume the task is stopped and in the latter we can't. This results in some conditional local_bh_disable()..local_bh_enable() in sve_set_vector_length(), which is a bit gross. Maybe it's best not to encourage ptrace and prctl to be given the same backend functions without thinking about it. (See the different callers of sve_set_vector_length() under arch/arm64/.) Others' views may differ though, so I'm happy to be overruled on this if people object. > I dislike this being named "prctl_arch" when everything else like this > is "arch_foo...", but I do see the collision with the x86-specific > "arch_prctl" syscall. Perhaps it might be better to wire things up Agreed. Actually, it was originally called arch_prctl(), until I discovered that it broke um. x86 seems happy enough, since its existing arch_prctl() functions all have do_/sys_/etc. prefixes. > differently, since x86's arch_prctl is actually "sys_arch_prctl" so I > don't think there would be a symbol collision, and maybe the 9 options > could be added to the global prctl list, with the x86 syscall getting > called at the end of the new "arch_prctl"? Then the syscall could get > deprecated in favor of just using prctl directly? > > Your new x86 arch_prctl (neé prctl_arch) could be something like: > > int arch_prctl(int option, unsigned long arg2, unsigned long arg3, > unsigned long arg4, unsigned long arg5) > { > ... existing stuff in your series ... > case ARCH_SET_GS: > case ARCH_SET_FS: > case ARCH_GET_GS: > case ARCH_GET_FS: > case ARCH_MAP_VDSO_X32: > case ARCH_MAP_VDSO_32: > case ARCH_MAP_VDSO_64: > if (arg3 || arg4 || arg5) > return -EINVAL; > return do_arch_prctl_64(current, option, arg2); > case ARCH_GET_CPUID: > case ARCH_SET_CPUID: > if (arg3 || arg4 || arg5) > return -EINVAL; > return do_arch_prctl_common(current, option, arg2); > default: > return -EINVAL; > } > } So, you mean we merge x86's arch_prctl() syscall with the generic one, and have both syscalls from userland invoke the common backend? That does look kind of feasible, given the compatible ABI and the fact that there does not appear to be a namespace clash in the option values. > Or maybe all the logic could get ripped out of > arch/x86/kernel/process*.c and put in arch/x86/kernel/sys.c directly? > > Perhaps this is all overkill, but I find it rather annoying that one > arch's weird syscall would block "expected" naming of a cross-arch > interface... I didn't really like overloading the name "arch_prctl" at all, since this is established as the name of the existing x86 syscall. Even if we arrange things in the kernel so that there is no linkage namespace clash, there seemed to be high risk of confusion. If we can merge the two (as suggested above) then that might be acceptable, but I wasn't confident that there were no subtle gotchas with such an approach... Thoughts? Cheers ---Dave
[Reviewer note: this is a cross-arch series. To reduce spam, I have tried not to Cc people on patches they aren't likely to care about. The complete series can be found in the LKML or linux-arch archives.] The core framework for the prctl() syscall is unloved and looking rather crusty these days. It also relies on defining ancillary boilerplate macros for each prctl() in order to control conditional compilation of the different prctl calls. We have better ways to do this now. This series attempts to modernise the code by defining a couple of new Kconfig variables HAVE_PRCTL_ARCH and HAVE_ARCH_PR_SET_GET_UNALIGN to allow architectures to provide hooks for arch-dependent prctls. For now this series has had minimal testing: some basic testing of arch-specific prctls using PR_SVE_SET_VL on arm64; build-tested on x86; otherwise untested. This is not polished yet... but I'm interested to know what people think about the approach. Cheers ---Dave Dave Martin (11): prctl: Support movement of arch prctls out of common code arm64: Move arch-specific prctls out of core code MIPS: Remove unused task argument from prctl functions MIPS: Move arch-specific prctls out of core code x86: Move arch-specific prctls out of core code powerpc: Remove unused task argument from prctl functions powerpc: Move arch-specific prctls out of core code ia64: Remove unused task argument from prctl functions ia64: Move arch-specific prctls out of core code prctl: Remove redundant task argument from PR_{SET,GET}_UNALIGN backends prctl: Refactor PR_{SET,GET}_UNALIGN to reduce boilerplate arch/Kconfig | 6 ++ arch/alpha/Kconfig | 1 + arch/alpha/include/asm/thread_info.h | 23 -------- arch/alpha/kernel/Makefile | 2 +- arch/alpha/kernel/sys.c | 36 ++++++++++++ arch/arm64/Kconfig | 1 + arch/arm64/include/asm/processor.h | 4 -- arch/arm64/kernel/sys.c | 15 +++++ arch/ia64/Kconfig | 2 + arch/ia64/include/asm/processor.h | 24 -------- arch/ia64/kernel/sys_ia64.c | 37 ++++++++++++ arch/mips/Kconfig | 1 + arch/mips/include/asm/processor.h | 3 - arch/mips/kernel/process.c | 23 ++++---- arch/mips/kernel/syscall.c | 15 +++++ arch/parisc/Kconfig | 1 + arch/parisc/include/asm/processor.h | 14 ----- arch/parisc/kernel/sys_parisc.c | 15 +++++ arch/powerpc/Kconfig | 2 + arch/powerpc/include/asm/processor.h | 20 ++----- arch/powerpc/kernel/process.c | 38 ++++++------ arch/powerpc/kernel/syscalls.c | 17 ++++++ arch/sh/Kconfig | 1 + arch/sh/include/asm/processor.h | 7 --- arch/sh/mm/alignment.c | 12 ++-- arch/x86/Kconfig | 1 + arch/x86/include/asm/processor.h | 6 -- arch/x86/kernel/Makefile | 1 + arch/x86/kernel/sys.c | 26 +++++++++ include/linux/prctl.h | 27 +++++++++ include/uapi/linux/prctl.h | 6 +- kernel/sys.c | 110 ++++------------------------------- tools/include/uapi/linux/prctl.h | 6 +- 33 files changed, 263 insertions(+), 240 deletions(-) create mode 100644 arch/alpha/kernel/sys.c create mode 100644 arch/x86/kernel/sys.c create mode 100644 include/linux/prctl.h -- 2.1.4