mbox series

[v2,00/18] clean up asm/uaccess.h, kill set_fs for good

Message ID 20220216131332.1489939-1-arnd@kernel.org
Headers show
Series clean up asm/uaccess.h, kill set_fs for good | expand

Message

Arnd Bergmann Feb. 16, 2022, 1:13 p.m. UTC
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

Comments

Christophe Leroy Feb. 17, 2022, 7:20 a.m. UTC | #1
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
Arnd Bergmann Feb. 17, 2022, 7:49 a.m. UTC | #2
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
Arnd Bergmann Feb. 17, 2022, 8:13 a.m. UTC | #3
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
Al Viro Feb. 18, 2022, 1:50 a.m. UTC | #4
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...
Al Viro Feb. 18, 2022, 2:21 a.m. UTC | #5
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.
Arnd Bergmann Feb. 18, 2022, 9:20 a.m. UTC | #6
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
Christophe Leroy Feb. 18, 2022, 10:01 a.m. UTC | #7
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