| Message ID | 20220216131332.1489939-1-arnd@kernel.org |
|---|---|
| Headers | show |
| Series | clean up asm/uaccess.h, kill set_fs for good | expand |
Le 16/02/2022 à 14:13, Arnd Bergmann a écrit : > From: Arnd Bergmann <arnd@arndb.de> > > Christoph Hellwig and a few others spent a huge effort on removing > set_fs() from most of the important architectures, but about half the > other architectures were never completed even though most of them don't > actually use set_fs() at all. > > I did a patch for microblaze at some point, which turned out to be fairly > generic, and now ported it to most other architectures, using new generic > implementations of access_ok() and __{get,put}_kernel_nocheck(). > > Three architectures (sparc64, ia64, and sh) needed some extra work, > which I also completed. > > The final series contains extra cleanup changes that touch all > architectures. Please review and test these, so we can merge them > for v5.18. As a further cleanup, have you thought about making a generic version of clear_user() ? On almost all architectures, clear_user() does an access_ok() then calls __clear_user() or similar. Maybe also the same with put_user() and get_user() ? After all it is just access_ok() followed by __put_user() or __get_user() ? It seems more tricky though, as some architectures seems to have less trivial stuff there. I also see all architectures have a prototype for strncpy_from_user() and strnlen_user(). Could be a common prototype instead when we have GENERIC_STRNCPY_FROM_USER / GENERIC_STRNLEN_USER And we have also user_access_begin()/user_read_access_begin()/user_write_access_begin() which call access_ok() then do the real work. Could be made generic with call to some arch specific __user_access_begin() and friends after the access_ok() and eventually the might_fault(). Christophe
On Thu, Feb 17, 2022 at 8:20 AM Christophe Leroy <christophe.leroy@csgroup.eu> wrote: > Le 16/02/2022 à 14:13, Arnd Bergmann a écrit : > > > > Christoph Hellwig and a few others spent a huge effort on removing > > set_fs() from most of the important architectures, but about half the > > other architectures were never completed even though most of them don't > > actually use set_fs() at all. > > > > I did a patch for microblaze at some point, which turned out to be fairly > > generic, and now ported it to most other architectures, using new generic > > implementations of access_ok() and __{get,put}_kernel_nocheck(). > > > > Three architectures (sparc64, ia64, and sh) needed some extra work, > > which I also completed. > > > > The final series contains extra cleanup changes that touch all > > architectures. Please review and test these, so we can merge them > > for v5.18. > > As a further cleanup, have you thought about making a generic version of > clear_user() ? On almost all architectures, clear_user() does an > access_ok() then calls __clear_user() or similar. This already exists in include/asm-generic/uaccess.h, but that file is currently not as easy to use as it should be. I've previously looked into what it would take to get more architectures to use common code in that file, but I currently have no plans to work on that. > Maybe also the same with put_user() and get_user() ? After all it is > just access_ok() followed by __put_user() or __get_user() ? It seems > more tricky though, as some architectures seems to have less trivial > stuff there. Same here: architectures can already provide a __put_user_fn() and __get_user_fn(), to get the generic versions of the interface, but few architectures use that. You can actually get all the interfaces by just providing raw_copy_from_user() and raw_copy_to_user(), but the get_user/put_user versions you get from that are fairly inefficient. > I also see all architectures have a prototype for strncpy_from_user() > and strnlen_user(). Could be a common prototype instead when we have > GENERIC_STRNCPY_FROM_USER / GENERIC_STRNLEN_USER > > And we have also > user_access_begin()/user_read_access_begin()/user_write_access_begin() > which call access_ok() then do the real work. Could be made generic with > call to some arch specific __user_access_begin() and friends after the > access_ok() and eventually the might_fault(). In my opinion, the biggest win would be to move the type-agnostic part of get_user/put_user into completely generic code, this is what architectures get wrong the most, see patch 02/18 in this series for instance. What I'd like to see is that architectures only provide fixed-length versions of unsafe_get_user()/unsafe_put_user(), with the type-agnostic versions (get_user(), __get_user(), unsafe_get_user() and their put versions) all defined once in include/linux/uaccess.h based on those. I tried implementing this in the past, but unfortunately the resulting object code from my generalized implementation was worse than what we have today, so I did not continue that work. Arnd
On Wed, Feb 16, 2022 at 2:13 PM Arnd Bergmann <arnd@kernel.org> wrote: > > From: Arnd Bergmann <arnd@arndb.de> > > Christoph Hellwig and a few others spent a huge effort on removing > set_fs() from most of the important architectures, but about half the > other architectures were never completed even though most of them don't > actually use set_fs() at all. > > I did a patch for microblaze at some point, which turned out to be fairly > generic, and now ported it to most other architectures, using new generic > implementations of access_ok() and __{get,put}_kernel_nocheck(). > > Three architectures (sparc64, ia64, and sh) needed some extra work, > which I also completed. > > The final series contains extra cleanup changes that touch all > architectures. Please review and test these, so we can merge them > for v5.18. > > The series is available at > https://git.kernel.org/pub/scm/linux/kernel/git/arnd/playground.git/log/?h=set_fs-2 > for testing. I've added the updated contents to my asm-generic tree now to put them into linux-next. Arnd
On Thu, Feb 17, 2022 at 07:20:11AM +0000, Christophe Leroy wrote: > And we have also > user_access_begin()/user_read_access_begin()/user_write_access_begin() > which call access_ok() then do the real work. Could be made generic with > call to some arch specific __user_access_begin() and friends after the > access_ok() and eventually the might_fault(). Not a good idea, considering the fact that we do not want to invite uses of "faster" variants...
On Thu, Feb 17, 2022 at 08:49:59AM +0100, Arnd Bergmann wrote: > Same here: architectures can already provide a __put_user_fn() > and __get_user_fn(), to get the generic versions of the interface, > but few architectures use that. You can actually get all the interfaces > by just providing raw_copy_from_user() and raw_copy_to_user(), > but the get_user/put_user versions you get from that are fairly > inefficient. FWIW, __{get,put}_user_{8,16,32,64} would probably make it easier to unify. That's where the really variable part tends to be, anyway. IMO __get_user_fn() had been a mistake. One thing I somewhat dislike about the series is the boilerplate in asm/uaccess.h instances - #include <asm-generic/access-ok.h> in a lot of them might make sense as a transitory state, but getting stuck with those indefinitely... BTW, do we need user_addr_max() anymore? The definition in asm-generic/access-ok.h is the only one, so ifndef around it is pointless.
n Fri, Feb 18, 2022 at 3:21 AM Al Viro <viro@zeniv.linux.org.uk> wrote: > > On Thu, Feb 17, 2022 at 08:49:59AM +0100, Arnd Bergmann wrote: > > > Same here: architectures can already provide a __put_user_fn() > > and __get_user_fn(), to get the generic versions of the interface, > > but few architectures use that. You can actually get all the interfaces > > by just providing raw_copy_from_user() and raw_copy_to_user(), > > but the get_user/put_user versions you get from that are fairly > > inefficient. > > FWIW, __{get,put}_user_{8,16,32,64} would probably make it easier to > unify. That's where the really variable part tends to be, anyway. > IMO __get_user_fn() had been a mistake. I've prototyped this now, to see what this might look like, see https://git.kernel.org/pub/scm/linux/kernel/git/arnd/playground.git/commit/?h=generic-get_user-prototype This adds generic inline version of {__get,get,__put,put}_user() and converts x86 to (optionally) use it. This builds with gcc-5 through gcc-11 on 32-bit and 64-bit x86, using asm-goto with outputs where possible, and requiring a minimum set of macro definitions from the architecture. Compiling with clang produces no warnings but does cause a linker issue at the moment, so there is probably at least one bug in it. Aside from compile-testing, I have not tried to verify if this is correct or efficient, but let me know if you think this is headed in the right direction. > One thing I somewhat dislike about the series is the boilerplate in > asm/uaccess.h instances - #include <asm-generic/access-ok.h> in > a lot of them might make sense as a transitory state, but getting > stuck with those indefinitely... Christoph also complained about it, the problem for now is that asm-generic/access_ok.h must first see the macro definitions for architectures that override any of the contents, but access_ok() itself is used at least in some of the asm/uaccess.h files as well, so it must be included in the middle of it, until more of the uaccess.h implementation is moved to linux/uaccess.h in an architecture independent way. Would you prefer having an asm/access_ok.h that falls back to the asm-generic version but can have an architecture specific override when needed (ia64, arm64, x86, um)? > BTW, do we need user_addr_max() anymore? The definition in > asm-generic/access-ok.h is the only one, so ifndef around it is pointless. Right, the v2 changes got rid of the last override, so it could get hardcoded to TASK_SIZE_MAX, or we can convert the five references to just use that instead and remove it altogether: arch/arm64/kernel/traps.c: if (address >= user_addr_max()) { \ arch/parisc/kernel/signal.c: if (start >= user_addr_max() - sigframe_size) arch/parisc/kernel/signal.c: if (A(&usp[0]) >= user_addr_max() - 5 * sizeof(int)) lib/strncpy_from_user.c: max_addr = user_addr_max(); lib/strnlen_user.c: max_addr = user_addr_max(); user_addr_max() first showed up in architecture-independent code in c5389831cda3 ("sparc: Fix user_addr_max() definition."), and from that I think the original intent is no longer useful. Arnd
Le 18/02/2022 à 02:50, Al Viro a écrit : > On Thu, Feb 17, 2022 at 07:20:11AM +0000, Christophe Leroy wrote: > >> And we have also >> user_access_begin()/user_read_access_begin()/user_write_access_begin() >> which call access_ok() then do the real work. Could be made generic with >> call to some arch specific __user_access_begin() and friends after the >> access_ok() and eventually the might_fault(). > > Not a good idea, considering the fact that we do not want to invite > uses of "faster" variants... I'm not sure I understand your concern. Today in powerpc we have: static __must_check inline bool user_read_access_begin(const void __user *ptr, size_t len) { if (unlikely(!access_ok(ptr, len))) return false; might_fault(); allow_read_from_user(ptr, len); return true; } We could instead have a generic static __must_check inline bool user_read_access_begin(const void __user *ptr, size_t len) { if (unlikely(!access_ok(ptr, len))) return false; might_fault(); return arch_user_read_access_begin(ptr, len); } And then a powerpc specific static __must_check __always_inline bool arch_user_read_access_begin(const void __user *ptr, size_t len) { allow_read_from_user(ptr, len); return true; } #define arch_user_read_access_begin arch_user_read_access_begin And a generic fallback for arch_user_read_access_begin() that does nothing at all. Do you mean that in that case people might be tempted to use arch_user_read_access_begin() instead of using user_read_access_begin() ? If that's the case isn't it something we could verify via checkpatch.pl ? Today it seems to be problematic that functions in asm/uaccess.h use access_ok(). Such an approach would allow to get rid of access_ok() use in architecture's uaccess.h Christophe
From: Arnd Bergmann <arnd@arndb.de> Christoph Hellwig and a few others spent a huge effort on removing set_fs() from most of the important architectures, but about half the other architectures were never completed even though most of them don't actually use set_fs() at all. I did a patch for microblaze at some point, which turned out to be fairly generic, and now ported it to most other architectures, using new generic implementations of access_ok() and __{get,put}_kernel_nocheck(). Three architectures (sparc64, ia64, and sh) needed some extra work, which I also completed. The final series contains extra cleanup changes that touch all architectures. Please review and test these, so we can merge them for v5.18. The series is available at https://git.kernel.org/pub/scm/linux/kernel/git/arnd/playground.git/log/?h=set_fs-2 for testing. Changes in v2: - add fixes for more nios2 and microblaze bugs found in the process - do final cleanup in a single patch - fix sparc64 regression - introduce CONFIG_ALTERNATE_USER_ADDRESS_SPACE - fix access_ok() in lib/test_lockup.c Arnd Bergmann (18): uaccess: fix integer overflow on access_ok() uaccess: fix nios2 and microblaze get_user_8() nds32: fix access_ok() checks in get/put_user sparc64: add __{get,put}_kernel_nocheck() x86: remove __range_not_ok() x86: use more conventional access_ok() definition nios2: drop access_ok() check from __put_user() uaccess: add generic __{get,put}_kernel_nofault mips: use simpler access_ok() m68k: fix access_ok for coldfire arm64: simplify access_ok() uaccess: fix type mismatch warnings from access_ok() uaccess: generalize access_ok() lib/test_lockup: fix kernel pointer check for separate address spaces sparc64: remove CONFIG_SET_FS support sh: remove CONFIG_SET_FS support ia64: remove CONFIG_SET_FS support uaccess: drop maining CONFIG_SET_FS users arch/Kconfig | 10 +- arch/alpha/Kconfig | 1 - arch/alpha/include/asm/processor.h | 4 - arch/alpha/include/asm/thread_info.h | 2 - arch/alpha/include/asm/uaccess.h | 53 +--------- arch/arc/Kconfig | 1 - arch/arc/include/asm/segment.h | 20 ---- arch/arc/include/asm/thread_info.h | 3 - arch/arc/include/asm/uaccess.h | 30 ------ arch/arc/kernel/process.c | 2 +- arch/arm/include/asm/uaccess.h | 22 +--- arch/arm/kernel/swp_emulate.c | 2 +- arch/arm/kernel/traps.c | 2 +- arch/arm/lib/uaccess_with_memcpy.c | 10 -- arch/arm64/include/asm/uaccess.h | 29 +----- arch/csky/Kconfig | 1 - arch/csky/include/asm/processor.h | 2 - arch/csky/include/asm/segment.h | 10 -- arch/csky/include/asm/thread_info.h | 2 - arch/csky/include/asm/uaccess.h | 12 --- arch/csky/kernel/asm-offsets.c | 1 - arch/csky/kernel/signal.c | 2 +- arch/h8300/Kconfig | 1 - arch/h8300/include/asm/processor.h | 1 - arch/h8300/include/asm/segment.h | 40 -------- arch/h8300/include/asm/thread_info.h | 3 - arch/h8300/kernel/entry.S | 1 - arch/h8300/kernel/head_ram.S | 1 - arch/h8300/mm/init.c | 6 -- arch/h8300/mm/memory.c | 1 - arch/hexagon/Kconfig | 1 - arch/hexagon/include/asm/thread_info.h | 6 -- arch/hexagon/include/asm/uaccess.h | 25 ----- arch/hexagon/kernel/process.c | 1 - arch/ia64/Kconfig | 1 - arch/ia64/include/asm/processor.h | 4 - arch/ia64/include/asm/thread_info.h | 2 - arch/ia64/include/asm/uaccess.h | 26 ++--- arch/ia64/kernel/unaligned.c | 60 +++++++---- arch/m68k/Kconfig.cpu | 1 + arch/m68k/include/asm/uaccess.h | 14 +-- arch/microblaze/Kconfig | 1 - arch/microblaze/include/asm/thread_info.h | 6 -- arch/microblaze/include/asm/uaccess.h | 61 ++--------- arch/microblaze/kernel/asm-offsets.c | 1 - arch/microblaze/kernel/process.c | 1 - arch/mips/include/asm/uaccess.h | 49 +-------- arch/mips/sibyte/common/sb_tbprof.c | 6 +- arch/nds32/Kconfig | 1 - arch/nds32/include/asm/thread_info.h | 4 - arch/nds32/include/asm/uaccess.h | 40 ++++---- arch/nds32/kernel/process.c | 5 +- arch/nds32/mm/alignment.c | 3 - arch/nios2/Kconfig | 1 - arch/nios2/include/asm/thread_info.h | 9 -- arch/nios2/include/asm/uaccess.h | 105 +++++++++---------- arch/nios2/kernel/signal.c | 20 ++-- arch/openrisc/Kconfig | 1 - arch/openrisc/include/asm/thread_info.h | 7 -- arch/openrisc/include/asm/uaccess.h | 42 +------- arch/parisc/Kconfig | 1 + arch/parisc/include/asm/futex.h | 6 -- arch/parisc/include/asm/uaccess.h | 13 +-- arch/parisc/lib/memcpy.c | 2 +- arch/powerpc/include/asm/uaccess.h | 13 +-- arch/powerpc/lib/sstep.c | 4 +- arch/riscv/include/asm/uaccess.h | 33 +----- arch/riscv/kernel/perf_callchain.c | 2 +- arch/s390/Kconfig | 1 + arch/s390/include/asm/uaccess.h | 16 +-- arch/sh/Kconfig | 1 - arch/sh/include/asm/processor.h | 1 - arch/sh/include/asm/segment.h | 33 ------ arch/sh/include/asm/thread_info.h | 2 - arch/sh/include/asm/uaccess.h | 24 +---- arch/sh/kernel/io_trapped.c | 9 +- arch/sh/kernel/process_32.c | 2 - arch/sh/kernel/traps_32.c | 30 ++++-- arch/sparc/Kconfig | 3 +- arch/sparc/include/asm/processor_32.h | 6 -- arch/sparc/include/asm/processor_64.h | 4 - arch/sparc/include/asm/switch_to_64.h | 4 +- arch/sparc/include/asm/thread_info_64.h | 4 +- arch/sparc/include/asm/uaccess.h | 3 - arch/sparc/include/asm/uaccess_32.h | 31 +----- arch/sparc/include/asm/uaccess_64.h | 113 +++++++++++++------- arch/sparc/kernel/process_32.c | 2 - arch/sparc/kernel/process_64.c | 12 --- arch/sparc/kernel/signal_32.c | 2 +- arch/sparc/kernel/traps_64.c | 2 - arch/sparc/lib/NGmemcpy.S | 3 +- arch/sparc/mm/init_64.c | 7 +- arch/um/include/asm/uaccess.h | 7 +- arch/x86/events/core.c | 2 +- arch/x86/include/asm/uaccess.h | 35 +------ arch/x86/kernel/dumpstack.c | 2 +- arch/x86/kernel/stacktrace.c | 2 +- arch/x86/lib/usercopy.c | 2 +- arch/xtensa/Kconfig | 1 - arch/xtensa/include/asm/asm-uaccess.h | 71 ------------- arch/xtensa/include/asm/processor.h | 7 -- arch/xtensa/include/asm/thread_info.h | 3 - arch/xtensa/include/asm/uaccess.h | 26 +---- arch/xtensa/kernel/asm-offsets.c | 3 - drivers/hid/uhid.c | 2 +- drivers/scsi/sg.c | 5 - fs/exec.c | 6 -- include/asm-generic/access_ok.h | 51 ++++++++++ include/asm-generic/uaccess.h | 46 +-------- include/linux/syscalls.h | 4 - include/linux/uaccess.h | 59 ++++------- include/rdma/ib.h | 2 +- kernel/events/callchain.c | 4 - kernel/events/core.c | 3 - kernel/exit.c | 14 --- kernel/kthread.c | 5 - kernel/stacktrace.c | 3 - kernel/trace/bpf_trace.c | 4 - lib/test_lockup.c | 11 +- mm/maccess.c | 119 ---------------------- mm/memory.c | 8 -- net/bpfilter/bpfilter_kern.c | 2 +- 122 files changed, 387 insertions(+), 1290 deletions(-) delete mode 100644 arch/arc/include/asm/segment.h delete mode 100644 arch/csky/include/asm/segment.h delete mode 100644 arch/h8300/include/asm/segment.h delete mode 100644 arch/sh/include/asm/segment.h create mode 100644 include/asm-generic/access_ok.h