Re: [PATCH v2 18/18] uaccess: drop maining CONFIG_SET_FS users

[Stafford Horne <shorne@gmail.com>]

On Wed, Feb 16, 2022 at 02:13:32PM +0100, Arnd Bergmann wrote:
> From: Arnd Bergmann <arnd@arndb.de>
> 
> There are no remaining callers of set_fs(), so CONFIG_SET_FS
> can be removed globally, along with the thread_info field and
> any references to it.
> 
> This turns access_ok() into a cheaper check against TASK_SIZE_MAX.
> 
> With CONFIG_SET_FS gone, so drop all remaining references to
> set_fs()/get_fs(), mm_segment_t and uaccess_kernel().
> 
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
...
>  arch/openrisc/Kconfig                     |  1 -
>  arch/openrisc/include/asm/thread_info.h   |  7 ---
>  arch/openrisc/include/asm/uaccess.h       | 23 --------
...
>  fs/exec.c                                 |  6 --
>  include/asm-generic/access_ok.h           | 10 +---
>  include/asm-generic/uaccess.h             | 25 +-------
>  include/linux/syscalls.h                  |  4 --
>  include/linux/uaccess.h                   | 33 -----------
>  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 --
>  mm/maccess.c                              | 11 ----
>  mm/memory.c                               |  8 ---
>  net/bpfilter/bpfilter_kern.c              |  2 +-
>  72 files changed, 10 insertions(+), 522 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
> 
> diff --git a/arch/Kconfig b/arch/Kconfig
> index fa5db36bda67..99349547afed 100644
> --- a/arch/Kconfig
> +++ b/arch/Kconfig
> @@ -24,9 +24,6 @@ config KEXEC_ELF
>  config HAVE_IMA_KEXEC
>  	bool
>  
> -config SET_FS
> -	bool
> -
>  config HOTPLUG_SMT
>  	bool
>  
...
> diff --git a/arch/openrisc/Kconfig b/arch/openrisc/Kconfig
> index f724b3f1aeed..0d68adf6e02b 100644
> --- a/arch/openrisc/Kconfig
> +++ b/arch/openrisc/Kconfig
> @@ -36,7 +36,6 @@ config OPENRISC
>  	select ARCH_WANT_FRAME_POINTERS
>  	select GENERIC_IRQ_MULTI_HANDLER
>  	select MMU_GATHER_NO_RANGE if MMU
> -	select SET_FS
>  	select TRACE_IRQFLAGS_SUPPORT
>  
>  config CPU_BIG_ENDIAN
> diff --git a/arch/openrisc/include/asm/thread_info.h b/arch/openrisc/include/asm/thread_info.h
> index 659834ab87fa..4af3049c34c2 100644
> --- a/arch/openrisc/include/asm/thread_info.h
> +++ b/arch/openrisc/include/asm/thread_info.h
> @@ -40,18 +40,12 @@
>   */
>  #ifndef __ASSEMBLY__
>  
> -typedef unsigned long mm_segment_t;
> -
>  struct thread_info {
>  	struct task_struct	*task;		/* main task structure */
>  	unsigned long		flags;		/* low level flags */
>  	__u32			cpu;		/* current CPU */
>  	__s32			preempt_count; /* 0 => preemptable, <0 => BUG */
>  
> -	mm_segment_t		addr_limit; /* thread address space:
> -					       0-0x7FFFFFFF for user-thead
> -					       0-0xFFFFFFFF for kernel-thread
> -					     */
>  	__u8			supervisor_stack[0];
>  
>  	/* saved context data */
> @@ -71,7 +65,6 @@ struct thread_info {
>  	.flags		= 0,				\
>  	.cpu		= 0,				\
>  	.preempt_count	= INIT_PREEMPT_COUNT,		\
> -	.addr_limit	= KERNEL_DS,			\
>  	.ksp            = 0,                            \
>  }
>  
> diff --git a/arch/openrisc/include/asm/uaccess.h b/arch/openrisc/include/asm/uaccess.h
> index 8f049ec99b3e..d6500a374e18 100644
> --- a/arch/openrisc/include/asm/uaccess.h
> +++ b/arch/openrisc/include/asm/uaccess.h
> @@ -22,29 +22,6 @@
>  #include <linux/string.h>
>  #include <asm/page.h>
>  #include <asm/extable.h>
> -
> -/*
> - * The fs value determines whether argument validity checking should be
> - * performed or not.  If get_fs() == USER_DS, checking is performed, with
> - * get_fs() == KERNEL_DS, checking is bypassed.
> - *
> - * For historical reasons, these macros are grossly misnamed.
> - */
> -
> -/* addr_limit is the maximum accessible address for the task. we misuse
> - * the KERNEL_DS and USER_DS values to both assign and compare the
> - * addr_limit values through the equally misnamed get/set_fs macros.
> - * (see above)
> - */
> -
> -#define KERNEL_DS	(~0UL)
> -
> -#define USER_DS		(TASK_SIZE)
> -#define get_fs()	(current_thread_info()->addr_limit)
> -#define set_fs(x)	(current_thread_info()->addr_limit = (x))
> -
> -#define uaccess_kernel()	(get_fs() == KERNEL_DS)
> -
>  #include <asm-generic/access_ok.h>
>  
>  /*
...
> diff --git a/fs/exec.c b/fs/exec.c
> index 79f2c9483302..bc68a0c089ac 100644
> --- a/fs/exec.c
> +++ b/fs/exec.c
> @@ -1303,12 +1303,6 @@ int begin_new_exec(struct linux_binprm * bprm)
>  	if (retval)
>  		goto out_unlock;
>  
> -	/*
> -	 * Ensure that the uaccess routines can actually operate on userspace
> -	 * pointers:
> -	 */
> -	force_uaccess_begin();
> -
>  	if (me->flags & PF_KTHREAD)
>  		free_kthread_struct(me);
>  	me->flags &= ~(PF_RANDOMIZE | PF_FORKNOEXEC | PF_KTHREAD |
> diff --git a/include/asm-generic/access_ok.h b/include/asm-generic/access_ok.h
> index 1aad8964d2ed..88a7cb5d9aad 100644
> --- a/include/asm-generic/access_ok.h
> +++ b/include/asm-generic/access_ok.h
> @@ -16,16 +16,8 @@
>  #define TASK_SIZE_MAX			TASK_SIZE
>  #endif
>  
> -#ifndef uaccess_kernel
> -#ifdef CONFIG_SET_FS
> -#define uaccess_kernel()		(get_fs().seg == KERNEL_DS.seg)
> -#else
> -#define uaccess_kernel()		(0)
> -#endif
> -#endif
> -
>  #ifndef user_addr_max
> -#define user_addr_max()			(uaccess_kernel() ? ~0UL : TASK_SIZE_MAX)
> +#define user_addr_max()			TASK_SIZE_MAX
>  #endif
>  
>  #ifndef __access_ok
> diff --git a/include/asm-generic/uaccess.h b/include/asm-generic/uaccess.h
> index ebc685dc8d74..a5be9e61a2a2 100644
> --- a/include/asm-generic/uaccess.h
> +++ b/include/asm-generic/uaccess.h
> @@ -8,6 +8,7 @@
>   * address space, e.g. all NOMMU machines.
>   */
>  #include <linux/string.h>
> +#include <asm-generic/access_ok.h>
>  
>  #ifdef CONFIG_UACCESS_MEMCPY
>  #include <asm/unaligned.h>
> @@ -94,30 +95,6 @@ raw_copy_to_user(void __user *to, const void *from, unsigned long n)
>  #define INLINE_COPY_TO_USER
>  #endif /* CONFIG_UACCESS_MEMCPY */
>  
> -#ifdef CONFIG_SET_FS
> -#define MAKE_MM_SEG(s)	((mm_segment_t) { (s) })
> -
> -#ifndef KERNEL_DS
> -#define KERNEL_DS	MAKE_MM_SEG(~0UL)
> -#endif
> -
> -#ifndef USER_DS
> -#define USER_DS		MAKE_MM_SEG(TASK_SIZE - 1)
> -#endif
> -
> -#ifndef get_fs
> -#define get_fs()	(current_thread_info()->addr_limit)
> -
> -static inline void set_fs(mm_segment_t fs)
> -{
> -	current_thread_info()->addr_limit = fs;
> -}
> -#endif
> -
> -#endif /* CONFIG_SET_FS */
> -
> -#include <asm-generic/access_ok.h>
> -
>  /*
>   * These are the main single-value transfer routines.  They automatically
>   * use the right size if we just have the right pointer type.
> diff --git a/include/linux/syscalls.h b/include/linux/syscalls.h
> index 819c0cb00b6d..a34b0f9a9972 100644
> --- a/include/linux/syscalls.h
> +++ b/include/linux/syscalls.h
> @@ -290,10 +290,6 @@ static inline void addr_limit_user_check(void)
>  		return;
>  #endif
>  
> -	if (CHECK_DATA_CORRUPTION(uaccess_kernel(),
> -				  "Invalid address limit on user-mode return"))
> -		force_sig(SIGKILL);
> -
>  #ifdef TIF_FSCHECK
>  	clear_thread_flag(TIF_FSCHECK);
>  #endif
> diff --git a/include/linux/uaccess.h b/include/linux/uaccess.h
> index 2c31667e62e0..2421a41f3a8e 100644
> --- a/include/linux/uaccess.h
> +++ b/include/linux/uaccess.h
> @@ -10,39 +10,6 @@
>  
>  #include <asm/uaccess.h>
>  
> -#ifdef CONFIG_SET_FS
> -/*
> - * Force the uaccess routines to be wired up for actual userspace access,
> - * overriding any possible set_fs(KERNEL_DS) still lingering around.  Undone
> - * using force_uaccess_end below.
> - */
> -static inline mm_segment_t force_uaccess_begin(void)
> -{
> -	mm_segment_t fs = get_fs();
> -
> -	set_fs(USER_DS);
> -	return fs;
> -}
> -
> -static inline void force_uaccess_end(mm_segment_t oldfs)
> -{
> -	set_fs(oldfs);
> -}
> -#else /* CONFIG_SET_FS */
> -typedef struct {
> -	/* empty dummy */
> -} mm_segment_t;
> -
> -static inline mm_segment_t force_uaccess_begin(void)
> -{
> -	return (mm_segment_t) { };
> -}
> -
> -static inline void force_uaccess_end(mm_segment_t oldfs)
> -{
> -}
> -#endif /* CONFIG_SET_FS */
> -
>  /*
>   * Architectures should provide two primitives (raw_copy_{to,from}_user())
>   * and get rid of their private instances of copy_{to,from}_user() and
> diff --git a/include/rdma/ib.h b/include/rdma/ib.h
> index 83139b9ce409..f7c185ff7a11 100644
> --- a/include/rdma/ib.h
> +++ b/include/rdma/ib.h
> @@ -75,7 +75,7 @@ struct sockaddr_ib {
>   */
>  static inline bool ib_safe_file_access(struct file *filp)
>  {
> -	return filp->f_cred == current_cred() && !uaccess_kernel();
> +	return filp->f_cred == current_cred();
>  }
>  
>  #endif /* _RDMA_IB_H */
> diff --git a/kernel/events/callchain.c b/kernel/events/callchain.c
> index 58cbe357fb2b..1273be84392c 100644
> --- a/kernel/events/callchain.c
> +++ b/kernel/events/callchain.c
> @@ -209,17 +209,13 @@ get_perf_callchain(struct pt_regs *regs, u32 init_nr, bool kernel, bool user,
>  		}
>  
>  		if (regs) {
> -			mm_segment_t fs;
> -
>  			if (crosstask)
>  				goto exit_put;
>  
>  			if (add_mark)
>  				perf_callchain_store_context(&ctx, PERF_CONTEXT_USER);
>  
> -			fs = force_uaccess_begin();
>  			perf_callchain_user(&ctx, regs);
> -			force_uaccess_end(fs);
>  		}
>  	}
>  
> diff --git a/kernel/events/core.c b/kernel/events/core.c
> index 57c7197838db..11ca7303d6df 100644
> --- a/kernel/events/core.c
> +++ b/kernel/events/core.c
> @@ -6746,7 +6746,6 @@ perf_output_sample_ustack(struct perf_output_handle *handle, u64 dump_size,
>  		unsigned long sp;
>  		unsigned int rem;
>  		u64 dyn_size;
> -		mm_segment_t fs;
>  
>  		/*
>  		 * We dump:
> @@ -6764,9 +6763,7 @@ perf_output_sample_ustack(struct perf_output_handle *handle, u64 dump_size,
>  
>  		/* Data. */
>  		sp = perf_user_stack_pointer(regs);
> -		fs = force_uaccess_begin();
>  		rem = __output_copy_user(handle, (void *) sp, dump_size);
> -		force_uaccess_end(fs);
>  		dyn_size = dump_size - rem;
>  
>  		perf_output_skip(handle, rem);
> diff --git a/kernel/exit.c b/kernel/exit.c
> index b00a25bb4ab9..0884a75bc2f8 100644
> --- a/kernel/exit.c
> +++ b/kernel/exit.c
> @@ -737,20 +737,6 @@ void __noreturn do_exit(long code)
>  
>  	WARN_ON(blk_needs_flush_plug(tsk));
>  
> -	/*
> -	 * If do_dead is called because this processes oopsed, it's possible
> -	 * that get_fs() was left as KERNEL_DS, so reset it to USER_DS before
> -	 * continuing. Amongst other possible reasons, this is to prevent
> -	 * mm_release()->clear_child_tid() from writing to a user-controlled
> -	 * kernel address.
> -	 *
> -	 * On uptodate architectures force_uaccess_begin is a noop.  On
> -	 * architectures that still have set_fs/get_fs in addition to handling
> -	 * oopses handles kernel threads that run as set_fs(KERNEL_DS) by
> -	 * default.
> -	 */
> -	force_uaccess_begin();
> -
>  	kcov_task_exit(tsk);
>  
>  	coredump_task_exit(tsk);
> diff --git a/kernel/kthread.c b/kernel/kthread.c
> index 38c6dd822da8..16c2275d4b50 100644
> --- a/kernel/kthread.c
> +++ b/kernel/kthread.c
> @@ -55,7 +55,6 @@ struct kthread {
>  	int result;
>  	int (*threadfn)(void *);
>  	void *data;
> -	mm_segment_t oldfs;
>  	struct completion parked;
>  	struct completion exited;
>  #ifdef CONFIG_BLK_CGROUP
> @@ -1441,8 +1440,6 @@ void kthread_use_mm(struct mm_struct *mm)
>  		mmdrop(active_mm);
>  	else
>  		smp_mb();
> -
> -	to_kthread(tsk)->oldfs = force_uaccess_begin();
>  }
>  EXPORT_SYMBOL_GPL(kthread_use_mm);
>  
> @@ -1457,8 +1454,6 @@ void kthread_unuse_mm(struct mm_struct *mm)
>  	WARN_ON_ONCE(!(tsk->flags & PF_KTHREAD));
>  	WARN_ON_ONCE(!tsk->mm);
>  
> -	force_uaccess_end(to_kthread(tsk)->oldfs);
> -
>  	task_lock(tsk);
>  	/*
>  	 * When a kthread stops operating on an address space, the loop
> diff --git a/kernel/stacktrace.c b/kernel/stacktrace.c
> index 9c625257023d..9ed5ce989415 100644
> --- a/kernel/stacktrace.c
> +++ b/kernel/stacktrace.c
> @@ -226,15 +226,12 @@ unsigned int stack_trace_save_user(unsigned long *store, unsigned int size)
>  		.store	= store,
>  		.size	= size,
>  	};
> -	mm_segment_t fs;
>  
>  	/* Trace user stack if not a kernel thread */
>  	if (current->flags & PF_KTHREAD)
>  		return 0;
>  
> -	fs = force_uaccess_begin();
>  	arch_stack_walk_user(consume_entry, &c, task_pt_regs(current));
> -	force_uaccess_end(fs);
>  
>  	return c.len;
>  }
> diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
> index 21aa30644219..8115fff17018 100644
> --- a/kernel/trace/bpf_trace.c
> +++ b/kernel/trace/bpf_trace.c
> @@ -332,8 +332,6 @@ BPF_CALL_3(bpf_probe_write_user, void __user *, unsafe_ptr, const void *, src,
>  	if (unlikely(in_interrupt() ||
>  		     current->flags & (PF_KTHREAD | PF_EXITING)))
>  		return -EPERM;
> -	if (unlikely(uaccess_kernel()))
> -		return -EPERM;
>  	if (unlikely(!nmi_uaccess_okay()))
>  		return -EPERM;
>  
> @@ -835,8 +833,6 @@ static int bpf_send_signal_common(u32 sig, enum pid_type type)
>  	 */
>  	if (unlikely(current->flags & (PF_KTHREAD | PF_EXITING)))
>  		return -EPERM;
> -	if (unlikely(uaccess_kernel()))
> -		return -EPERM;
>  	if (unlikely(!nmi_uaccess_okay()))
>  		return -EPERM;
>  
> diff --git a/mm/maccess.c b/mm/maccess.c
> index cbd1b3959af2..106820b33a2b 100644
> --- a/mm/maccess.c
> +++ b/mm/maccess.c
> @@ -113,14 +113,11 @@ long strncpy_from_kernel_nofault(char *dst, const void *unsafe_addr, long count)
>  long copy_from_user_nofault(void *dst, const void __user *src, size_t size)
>  {
>  	long ret = -EFAULT;
> -	mm_segment_t old_fs = force_uaccess_begin();
> -
>  	if (access_ok(src, size)) {
>  		pagefault_disable();
>  		ret = __copy_from_user_inatomic(dst, src, size);
>  		pagefault_enable();
>  	}
> -	force_uaccess_end(old_fs);
>  
>  	if (ret)
>  		return -EFAULT;
> @@ -140,14 +137,12 @@ EXPORT_SYMBOL_GPL(copy_from_user_nofault);
>  long copy_to_user_nofault(void __user *dst, const void *src, size_t size)
>  {
>  	long ret = -EFAULT;
> -	mm_segment_t old_fs = force_uaccess_begin();
>  
>  	if (access_ok(dst, size)) {
>  		pagefault_disable();
>  		ret = __copy_to_user_inatomic(dst, src, size);
>  		pagefault_enable();
>  	}
> -	force_uaccess_end(old_fs);
>  
>  	if (ret)
>  		return -EFAULT;
> @@ -176,17 +171,14 @@ EXPORT_SYMBOL_GPL(copy_to_user_nofault);
>  long strncpy_from_user_nofault(char *dst, const void __user *unsafe_addr,
>  			      long count)
>  {
> -	mm_segment_t old_fs;
>  	long ret;
>  
>  	if (unlikely(count <= 0))
>  		return 0;
>  
> -	old_fs = force_uaccess_begin();
>  	pagefault_disable();
>  	ret = strncpy_from_user(dst, unsafe_addr, count);
>  	pagefault_enable();
> -	force_uaccess_end(old_fs);
>  
>  	if (ret >= count) {
>  		ret = count;
> @@ -216,14 +208,11 @@ long strncpy_from_user_nofault(char *dst, const void __user *unsafe_addr,
>   */
>  long strnlen_user_nofault(const void __user *unsafe_addr, long count)
>  {
> -	mm_segment_t old_fs;
>  	int ret;
>  
> -	old_fs = force_uaccess_begin();
>  	pagefault_disable();
>  	ret = strnlen_user(unsafe_addr, count);
>  	pagefault_enable();
> -	force_uaccess_end(old_fs);
>  
>  	return ret;
>  }
> diff --git a/mm/memory.c b/mm/memory.c
> index c125c4969913..9a6ebf68a846 100644
> --- a/mm/memory.c
> +++ b/mm/memory.c
> @@ -5256,14 +5256,6 @@ void print_vma_addr(char *prefix, unsigned long ip)
>  #if defined(CONFIG_PROVE_LOCKING) || defined(CONFIG_DEBUG_ATOMIC_SLEEP)
>  void __might_fault(const char *file, int line)
>  {
> -	/*
> -	 * Some code (nfs/sunrpc) uses socket ops on kernel memory while
> -	 * holding the mmap_lock, this is safe because kernel memory doesn't
> -	 * get paged out, therefore we'll never actually fault, and the
> -	 * below annotations will generate false positives.
> -	 */
> -	if (uaccess_kernel())
> -		return;
>  	if (pagefault_disabled())
>  		return;
>  	__might_sleep(file, line);
> diff --git a/net/bpfilter/bpfilter_kern.c b/net/bpfilter/bpfilter_kern.c
> index 51a941b56ec3..422ec6e7ccff 100644

Acked-by: Stafford Horne <shorne@gmail.com> [openrisc, asm-generic]

Thanks!