[PATCH] microblaze: remove CONFIG_SET_FS

[PATCH] microblaze: remove CONFIG_SET_FS

[Arnd Bergmann]

From: Arnd Bergmann <arnd@arndb.de>

Remove the address space override API set_fs().  The microblaze user
address space is now limited to TASK_SIZE.

To support this we implement and wire in __get_kernel_nofault and
__set_kernel_nofault.

The function user_addr_max is removed as there is a default definition
provided when CONFIG_SET_FS is not used.

Link: https://lore.kernel.org/lkml/20220117132757.1881981-1-arnd@kernel.org/
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 arch/microblaze/Kconfig                   |  1 -
 arch/microblaze/include/asm/thread_info.h |  6 ---
 arch/microblaze/include/asm/uaccess.h     | 56 ++++++++++-------------
 arch/microblaze/kernel/asm-offsets.c      |  1 -
 4 files changed, 25 insertions(+), 39 deletions(-)

diff --git a/arch/microblaze/Kconfig b/arch/microblaze/Kconfig
index 59798e43cdb0..1fb1cec087b7 100644
--- a/arch/microblaze/Kconfig
+++ b/arch/microblaze/Kconfig
@@ -42,7 +42,6 @@ config MICROBLAZE
 	select CPU_NO_EFFICIENT_FFS
 	select MMU_GATHER_NO_RANGE
 	select SPARSE_IRQ
-	select SET_FS
 	select ZONE_DMA
 	select TRACE_IRQFLAGS_SUPPORT
 
diff --git a/arch/microblaze/include/asm/thread_info.h b/arch/microblaze/include/asm/thread_info.h
index 44f5ca331862..a0ddd2a36fb9 100644
--- a/arch/microblaze/include/asm/thread_info.h
+++ b/arch/microblaze/include/asm/thread_info.h
@@ -56,17 +56,12 @@ struct cpu_context {
 	__u32	fsr;
 };
 
-typedef struct {
-	unsigned long seg;
-} mm_segment_t;
-
 struct thread_info {
 	struct task_struct	*task; /* main task structure */
 	unsigned long		flags; /* low level flags */
 	unsigned long		status; /* thread-synchronous flags */
 	__u32			cpu; /* current CPU */
 	__s32			preempt_count; /* 0 => preemptable,< 0 => BUG*/
-	mm_segment_t		addr_limit; /* thread address space */
 
 	struct cpu_context	cpu_context;
 };
@@ -80,7 +75,6 @@ struct thread_info {
 	.flags		= 0,			\
 	.cpu		= 0,			\
 	.preempt_count	= INIT_PREEMPT_COUNT,	\
-	.addr_limit	= KERNEL_DS,		\
 }
 
 /* how to get the thread information struct from C */
diff --git a/arch/microblaze/include/asm/uaccess.h b/arch/microblaze/include/asm/uaccess.h
index d2a8ef9f8978..346fe4618b27 100644
--- a/arch/microblaze/include/asm/uaccess.h
+++ b/arch/microblaze/include/asm/uaccess.h
@@ -16,45 +16,20 @@
 #include <asm/extable.h>
 #include <linux/string.h>
 
-/*
- * On Microblaze the fs value is actually the top of the corresponding
- * address space.
- *
- * 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.
- *
- * For non-MMU arch like Microblaze, KERNEL_DS and USER_DS is equal.
- */
-# define MAKE_MM_SEG(s)       ((mm_segment_t) { (s) })
-
-#  define KERNEL_DS	MAKE_MM_SEG(0xFFFFFFFF)
-#  define USER_DS	MAKE_MM_SEG(TASK_SIZE - 1)
-
-# define get_fs()	(current_thread_info()->addr_limit)
-# define set_fs(val)	(current_thread_info()->addr_limit = (val))
-# define user_addr_max() get_fs().seg
-
-# define uaccess_kernel()	(get_fs().seg == KERNEL_DS.seg)
-
 static inline int access_ok(const void __user *addr, unsigned long size)
 {
 	if (!size)
 		goto ok;
 
-	if ((get_fs().seg < ((unsigned long)addr)) ||
-			(get_fs().seg < ((unsigned long)addr + size - 1))) {
-		pr_devel("ACCESS fail at 0x%08x (size 0x%x), seg 0x%08x\n",
-			(__force u32)addr, (u32)size,
-			(u32)get_fs().seg);
+	if ((((unsigned long)addr) > TASK_SIZE) ||
+	    (((unsigned long)addr + size - 1) > TASK_SIZE)) {
+		pr_devel("ACCESS fail at 0x%08x (size 0x%x)",
+			(__force u32)addr, (u32)size);
 		return 0;
 	}
 ok:
-	pr_devel("ACCESS OK at 0x%08x (size 0x%x), seg 0x%08x\n",
-			(__force u32)addr, (u32)size,
-			(u32)get_fs().seg);
+	pr_devel("ACCESS OK at 0x%08x (size 0x%x)\n",
+			(__force u32)addr, (u32)size);
 	return 1;
 }
 
@@ -280,6 +255,25 @@ extern long __user_bad(void);
 	__gu_err;							\
 })
 
+#define __get_kernel_nofault(dst, src, type, label)	\
+{							\
+	type __user *p = (type __force __user *)(src);	\
+	type data;					\
+	if (__get_user(data, p))			\
+		goto label;				\
+	*(type *)dst = data;				\
+}
+
+#define __put_kernel_nofault(dst, src, type, label)	\
+{							\
+	type __user *p = (type __force __user *)(dst);	\
+	type data = *(type *)src;			\
+	if (__put_user(data, p))			\
+		goto label;				\
+}
+
+#define HAVE_GET_KERNEL_NOFAULT
+
 static inline unsigned long
 raw_copy_from_user(void *to, const void __user *from, unsigned long n)
 {
diff --git a/arch/microblaze/kernel/asm-offsets.c b/arch/microblaze/kernel/asm-offsets.c
index b77dd188dec4..47ee409508b1 100644
--- a/arch/microblaze/kernel/asm-offsets.c
+++ b/arch/microblaze/kernel/asm-offsets.c
@@ -86,7 +86,6 @@ int main(int argc, char *argv[])
 	/* struct thread_info */
 	DEFINE(TI_TASK, offsetof(struct thread_info, task));
 	DEFINE(TI_FLAGS, offsetof(struct thread_info, flags));
-	DEFINE(TI_ADDR_LIMIT, offsetof(struct thread_info, addr_limit));
 	DEFINE(TI_CPU_CONTEXT, offsetof(struct thread_info, cpu_context));
 	DEFINE(TI_PREEMPT_COUNT, offsetof(struct thread_info, preempt_count));
 	BLANK();
-- 
2.29.2

RE: [PATCH] microblaze: remove CONFIG_SET_FS

[David Laight]

From: Arnd
> Sent: 09 February 2022 14:49
> 
> Remove the address space override API set_fs().  The microblaze user
> address space is now limited to TASK_SIZE.
> 
> To support this we implement and wire in __get_kernel_nofault and
> __set_kernel_nofault.
> 
> The function user_addr_max is removed as there is a default definition
> provided when CONFIG_SET_FS is not used.
...
>  static inline int access_ok(const void __user *addr, unsigned long size)
>  {
>  	if (!size)
>  		goto ok;
> 
> -	if ((get_fs().seg < ((unsigned long)addr)) ||
> -			(get_fs().seg < ((unsigned long)addr + size - 1))) {
> -		pr_devel("ACCESS fail at 0x%08x (size 0x%x), seg 0x%08x\n",
> -			(__force u32)addr, (u32)size,
> -			(u32)get_fs().seg);
> +	if ((((unsigned long)addr) > TASK_SIZE) ||
> +	    (((unsigned long)addr + size - 1) > TASK_SIZE)) {
> +		pr_devel("ACCESS fail at 0x%08x (size 0x%x)",
> +			(__force u32)addr, (u32)size);
>  		return 0;

Isn't that the wrong check?
If 'size' is big 'addr + size' can wrap.

It needs to be (addr >= TASK_SIZE || size > TASK_SIZE - addr)

Which is pretty much a generic version.
Although typical 64bit architectures can use the faster:
	((addr | size) >> 62)

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Re: [PATCH] microblaze: remove CONFIG_SET_FS

[Arnd Bergmann]

On Thu, Feb 10, 2022 at 10:36 AM David Laight <David.Laight@aculab.com> wrote:
> From: Arnd Sent: 09 February 2022 14:49
> >
> > Remove the address space override API set_fs().  The microblaze user
> > address space is now limited to TASK_SIZE.
> >
> > To support this we implement and wire in __get_kernel_nofault and
> > __set_kernel_nofault.
> >
> > The function user_addr_max is removed as there is a default definition
> > provided when CONFIG_SET_FS is not used.
> ...
> >  static inline int access_ok(const void __user *addr, unsigned long size)
> >  {
> >       if (!size)
> >               goto ok;
> >
> > -     if ((get_fs().seg < ((unsigned long)addr)) ||
> > -                     (get_fs().seg < ((unsigned long)addr + size - 1))) {
> > -             pr_devel("ACCESS fail at 0x%08x (size 0x%x), seg 0x%08x\n",
> > -                     (__force u32)addr, (u32)size,
> > -                     (u32)get_fs().seg);
> > +     if ((((unsigned long)addr) > TASK_SIZE) ||
> > +         (((unsigned long)addr + size - 1) > TASK_SIZE)) {
> > +             pr_devel("ACCESS fail at 0x%08x (size 0x%x)",
> > +                     (__force u32)addr, (u32)size);
> >               return 0;
>
> Isn't that the wrong check?
> If 'size' is big 'addr + size' can wrap.

Ah right, that seems dangerous, and we should probably fix that first, with
a backport to stable kernels before my patch. I see the same bug on csky
and hexagon:

static inline int __access_ok(unsigned long addr, unsigned long size)
{
        unsigned long limit = current_thread_info()->addr_limit.seg;
        return ((addr < limit) && ((addr + size) < limit));
}

#define __access_ok(addr, size) \
        ((get_fs().seg == KERNEL_DS.seg) || \
        (((unsigned long)addr < get_fs().seg) && \
          (unsigned long)size < (get_fs().seg - (unsigned long)addr)))

ia64 and sparc skip the size check entirely but rely on an unmapped page
at the beginning of the kernel address range, which avoids this problem
but may also be dangerous.

m68k-coldfire-mmu has a comment about needing a check, but tests
for neither address nor size.

> It needs to be (addr >= TASK_SIZE || size > TASK_SIZE - addr)
>
> Which is pretty much a generic version.
> Although typical 64bit architectures can use the faster:
>         ((addr | size) >> 62)

I think this is the best version, and it's already widely used:

static inline int __range_ok(unsigned long addr, unsigned long size)
{
        return size <= TASK_SIZE && addr <= (TASK_SIZE - size);
}

since 'size' is usually constant, so this turns into a single comparison
against a compile-time constant.

         Arnd

RE: [PATCH] microblaze: remove CONFIG_SET_FS

[David Laight]

From: Arnd Bergmann
> Sent: 10 February 2022 13:30
...
> #define __access_ok(addr, size) \
>         ((get_fs().seg == KERNEL_DS.seg) || \
>         (((unsigned long)addr < get_fs().seg) && \
>           (unsigned long)size < (get_fs().seg - (unsigned long)addr)))

That one is strange.
Seems to be optimised for kernel accesses!

> ia64 and sparc skip the size check entirely but rely on an unmapped page
> at the beginning of the kernel address range, which avoids this problem
> but may also be dangerous.

An unmapped page requires that the kernel do sequential accesses
(or, at least, not big offset) - which is normally fine.
Especially for 64bit where there is plenty of address space.
I guess it could be problematic for 32bit if you can/want to
use 'big pages' for the kernel addresses.
Losing a single (typically) 4k page isn't a problem.

Certainly not mapping the page at TASK_SIZE is a good safety check.
Actually, setting TASK_SIZE to 0xc0000000 - PAGE_SIZE and never
mapping the last user page has the same effect.
Except I bet the ldso has to go there :-(
Not to mention the instruction sets where loading the constant
would then be two instructions.

...
> > Although typical 64bit architectures can use the faster:
> >         ((addr | size) >> 62)
> 
> I think this is the best version, and it's already widely used:

I just did a quick check, both clang and gcc optimise out
constant values for 'size'.

> static inline int __range_ok(unsigned long addr, unsigned long size)
> {
>         return size <= TASK_SIZE && addr <= (TASK_SIZE - size);
> }
> 
> since 'size' is usually constant, so this turns into a single comparison
> against a compile-time constant.

Hmmm... maybe there should be a comment that it is the same as
the more obvious:
	(addr <= TASK_SIZE && addr <= TASK_SIZE - size)
but is better for constant size.
(Provided TASK_SIZE is a constant.)

I'm sure Linus was 'unhappy' about checking against 2^63 for
32bit processes on a 64bit kernel.

Hmmm compat code that has 32bit addr/size needn't even call
access_ok() - it can never access kernel memory at all.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Re: [PATCH] microblaze: remove CONFIG_SET_FS

[Arnd Bergmann]

On Thu, Feb 10, 2022 at 3:21 PM David Laight <David.Laight@aculab.com> wrote:
> From: Arnd Bergmann Sent: 10 February 2022 13:30
>
> > static inline int __range_ok(unsigned long addr, unsigned long size)
> > {
> >         return size <= TASK_SIZE && addr <= (TASK_SIZE - size);
> > }
> >
> > since 'size' is usually constant, so this turns into a single comparison
> > against a compile-time constant.
>
> Hmmm... maybe there should be a comment that it is the same as
> the more obvious:
>         (addr <= TASK_SIZE && addr <= TASK_SIZE - size)
> but is better for constant size.
> (Provided TASK_SIZE is a constant.)
>
> I'm sure Linus was 'unhappy' about checking against 2^63 for
> 32bit processes on a 64bit kernel.
>
> Hmmm compat code that has 32bit addr/size needn't even call
> access_ok() - it can never access kernel memory at all.

I suppose the generic function should compare against
TASK_SIZE_MAX or user_addr_max() then, to make it a
constant while TASK_SIZE potentially depends on compat
mode.

        Arnd

Re: [PATCH] microblaze: remove CONFIG_SET_FS

[Stafford Horne]

On Thu, Feb 10, 2022 at 02:21:07PM +0000, David Laight wrote:
> From: Arnd Bergmann
> > static inline int __range_ok(unsigned long addr, unsigned long size)
> > {
> >         return size <= TASK_SIZE && addr <= (TASK_SIZE - size);
> > }
> > 
> > since 'size' is usually constant, so this turns into a single comparison
> > against a compile-time constant.
> 
> Hmmm... maybe there should be a comment that it is the same as
> the more obvious:
> 	(addr <= TASK_SIZE && addr <= TASK_SIZE - size)
> but is better for constant size.
> (Provided TASK_SIZE is a constant.)

Ah, this makes sense that.  I might as well revert the OpenRISC patch for this.
Though, we shall move to the generic version in the end.

With ideal compare:

    -rwxrwxr-x. 1 shorne shorne 6011932 Feb  9 17:57 vmlinux

With comparing (size <= TASK_SIZE && addr <= TASK_SIZE - size):

    -rwxrwxr-x. 1 shorne shorne 6003556 Feb 11 07:18 vmlinux

-Stafford

Re: [PATCH 03/10] exit: Move oops specific logic from do_exit into make_task_dead

[Arnd Bergmann]

On Mon, Jan 17, 2022 at 9:05 AM Christoph Hellwig <hch@infradead.org> wrote:
>
> On Fri, Jan 07, 2022 at 12:59:33PM -0600, Eric W. Biederman wrote:
> > Assuming it won't be too much longer before the rest of the arches have
> > set_fs/get_fs removed it looks like it makes sense to leave the
> > force_uaccess_begin where it is, and just let force_uaccess_begin be
> > removed when set_fs/get_fs are removed from the tree.
> >
> > Christoph does it look like the set_fs/get_fs removal work is going
> > to stall indefinitely on some architectures?  If so I think we want to
> > find a way to get kernel threads to run with set_fs(USER_DS) on the
> > stalled architectures.  Otherwise I think we have a real hazard of
> > introducing bugs that will only show up on the stalled architectures.
>
> I really need help from the arch maintainers to finish the set_fs
> removal.  There have been very few arch maintainers helping with that
> work (arm, arm64, parisc, m68k) in addition to the ones I did because
> I have the test setups and knowledge.  I'll send out another ping,
> for necrotic architectures like ia64 and sh I have very little hope.

I did a conversion of microblaze for fun at some point, and I think I never
sent that out. I haven't tested it, but if this looks correct to you and
Michal, it could serve as a model for other trivial conversions.

I also looked into converting ia64 and sh at the same time, but I can't
find those patches now, so I think they were never complete.

       Arnd

[PATCH] microblaze: remove CONFIG_SET_FS

[Arnd Bergmann]

From: Arnd Bergmann <arnd@arndb.de>

I picked microblaze as one of the architectures that still
use set_fs() and converted it not to.

Link: https://lore.kernel.org/lkml/CAK8P3a22ntk5fTuk6xjh1pyS-eVbGo7zDQSVkn2VG1xgp01D9g@mail.gmail.com/
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
This is an old patch I found after Christoph asked about
conversions for the remaining architectures. I have no idea
about the state of this patch, but there is a reasonable
chance that it works.
---
 arch/microblaze/Kconfig                   |  1 -
 arch/microblaze/include/asm/thread_info.h |  6 ---
 arch/microblaze/include/asm/uaccess.h     | 56 ++++++++++-------------
 arch/microblaze/kernel/asm-offsets.c      |  1 -
 4 files changed, 25 insertions(+), 39 deletions(-)

diff --git a/arch/microblaze/Kconfig b/arch/microblaze/Kconfig
index 59798e43cdb0..1fb1cec087b7 100644
--- a/arch/microblaze/Kconfig
+++ b/arch/microblaze/Kconfig
@@ -42,7 +42,6 @@ config MICROBLAZE
 	select CPU_NO_EFFICIENT_FFS
 	select MMU_GATHER_NO_RANGE
 	select SPARSE_IRQ
-	select SET_FS
 	select ZONE_DMA
 	select TRACE_IRQFLAGS_SUPPORT
 
diff --git a/arch/microblaze/include/asm/thread_info.h b/arch/microblaze/include/asm/thread_info.h
index 44f5ca331862..a0ddd2a36fb9 100644
--- a/arch/microblaze/include/asm/thread_info.h
+++ b/arch/microblaze/include/asm/thread_info.h
@@ -56,17 +56,12 @@ struct cpu_context {
 	__u32	fsr;
 };
 
-typedef struct {
-	unsigned long seg;
-} mm_segment_t;
-
 struct thread_info {
 	struct task_struct	*task; /* main task structure */
 	unsigned long		flags; /* low level flags */
 	unsigned long		status; /* thread-synchronous flags */
 	__u32			cpu; /* current CPU */
 	__s32			preempt_count; /* 0 => preemptable,< 0 => BUG*/
-	mm_segment_t		addr_limit; /* thread address space */
 
 	struct cpu_context	cpu_context;
 };
@@ -80,7 +75,6 @@ struct thread_info {
 	.flags		= 0,			\
 	.cpu		= 0,			\
 	.preempt_count	= INIT_PREEMPT_COUNT,	\
-	.addr_limit	= KERNEL_DS,		\
 }
 
 /* how to get the thread information struct from C */
diff --git a/arch/microblaze/include/asm/uaccess.h b/arch/microblaze/include/asm/uaccess.h
index d2a8ef9f8978..346fe4618b27 100644
--- a/arch/microblaze/include/asm/uaccess.h
+++ b/arch/microblaze/include/asm/uaccess.h
@@ -16,45 +16,20 @@
 #include <asm/extable.h>
 #include <linux/string.h>
 
-/*
- * On Microblaze the fs value is actually the top of the corresponding
- * address space.
- *
- * 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.
- *
- * For non-MMU arch like Microblaze, KERNEL_DS and USER_DS is equal.
- */
-# define MAKE_MM_SEG(s)       ((mm_segment_t) { (s) })
-
-#  define KERNEL_DS	MAKE_MM_SEG(0xFFFFFFFF)
-#  define USER_DS	MAKE_MM_SEG(TASK_SIZE - 1)
-
-# define get_fs()	(current_thread_info()->addr_limit)
-# define set_fs(val)	(current_thread_info()->addr_limit = (val))
-# define user_addr_max() get_fs().seg
-
-# define uaccess_kernel()	(get_fs().seg == KERNEL_DS.seg)
-
 static inline int access_ok(const void __user *addr, unsigned long size)
 {
 	if (!size)
 		goto ok;
 
-	if ((get_fs().seg < ((unsigned long)addr)) ||
-			(get_fs().seg < ((unsigned long)addr + size - 1))) {
-		pr_devel("ACCESS fail at 0x%08x (size 0x%x), seg 0x%08x\n",
-			(__force u32)addr, (u32)size,
-			(u32)get_fs().seg);
+	if ((((unsigned long)addr) > TASK_SIZE) ||
+	    (((unsigned long)addr + size - 1) > TASK_SIZE)) {
+		pr_devel("ACCESS fail at 0x%08x (size 0x%x)",
+			(__force u32)addr, (u32)size);
 		return 0;
 	}
 ok:
-	pr_devel("ACCESS OK at 0x%08x (size 0x%x), seg 0x%08x\n",
-			(__force u32)addr, (u32)size,
-			(u32)get_fs().seg);
+	pr_devel("ACCESS OK at 0x%08x (size 0x%x)\n",
+			(__force u32)addr, (u32)size);
 	return 1;
 }
 
@@ -280,6 +255,25 @@ extern long __user_bad(void);
 	__gu_err;							\
 })
 
+#define __get_kernel_nofault(dst, src, type, label)	\
+{							\
+	type __user *p = (type __force __user *)(src);	\
+	type data;					\
+	if (__get_user(data, p))			\
+		goto label;				\
+	*(type *)dst = data;				\
+}
+
+#define __put_kernel_nofault(dst, src, type, label)	\
+{							\
+	type __user *p = (type __force __user *)(dst);	\
+	type data = *(type *)src;			\
+	if (__put_user(data, p))			\
+		goto label;				\
+}
+
+#define HAVE_GET_KERNEL_NOFAULT
+
 static inline unsigned long
 raw_copy_from_user(void *to, const void __user *from, unsigned long n)
 {
diff --git a/arch/microblaze/kernel/asm-offsets.c b/arch/microblaze/kernel/asm-offsets.c
index b77dd188dec4..47ee409508b1 100644
--- a/arch/microblaze/kernel/asm-offsets.c
+++ b/arch/microblaze/kernel/asm-offsets.c
@@ -86,7 +86,6 @@ int main(int argc, char *argv[])
 	/* struct thread_info */
 	DEFINE(TI_TASK, offsetof(struct thread_info, task));
 	DEFINE(TI_FLAGS, offsetof(struct thread_info, flags));
-	DEFINE(TI_ADDR_LIMIT, offsetof(struct thread_info, addr_limit));
 	DEFINE(TI_CPU_CONTEXT, offsetof(struct thread_info, cpu_context));
 	DEFINE(TI_PREEMPT_COUNT, offsetof(struct thread_info, preempt_count));
 	BLANK();
-- 
2.29.2

Re: [PATCH] microblaze: remove CONFIG_SET_FS

[Michal Simek]

Hi Arnd,

po 17. 1. 2022 v 14:28 odesílatel Arnd Bergmann <arnd@kernel.org> napsal:
>
> From: Arnd Bergmann <arnd@arndb.de>
>
> I picked microblaze as one of the architectures that still
> use set_fs() and converted it not to.

Can you please update the commit message because what is above is not
the right one?

I can't see any issue with the patch when I run it on real HW.
Tested-by: Michal Simek <michal.simek@xilinx.com>

Christoph: Is there any recommended test suite which I should run?

Thanks,
Michal

-- 
Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91
w: www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel - Xilinx Microblaze
Maintainer of Linux kernel - Xilinx Zynq ARM and ZynqMP ARM64 SoCs
U-Boot custodian - Xilinx Microblaze/Zynq/ZynqMP/Versal SoCs

Re: [PATCH] microblaze: remove CONFIG_SET_FS

[Christoph Hellwig]

On Wed, Feb 09, 2022 at 02:50:32PM +0100, Michal Simek wrote:
> I can't see any issue with the patch when I run it on real HW.
> Tested-by: Michal Simek <michal.simek@xilinx.com>
> 
> Christoph: Is there any recommended test suite which I should run?

No.  For architectures that already didn't use set_fs internally
there is nothing specific to test.  Running some perf or backtrace
tests might be useful to check if the non-faulting kernel helpers
work properly.

Re: [PATCH] microblaze: remove CONFIG_SET_FS

[Michal Simek]

On 2/9/22 14:52, Christoph Hellwig wrote:
> On Wed, Feb 09, 2022 at 02:50:32PM +0100, Michal Simek wrote:
>> I can't see any issue with the patch when I run it on real HW.
>> Tested-by: Michal Simek <michal.simek@xilinx.com>
>>
>> Christoph: Is there any recommended test suite which I should run?
> 
> No.  For architectures that already didn't use set_fs internally
> there is nothing specific to test.  Running some perf or backtrace
> tests might be useful to check if the non-faulting kernel helpers
> work properly.

Thanks for confirmation. Once Arnd sent v2 with updated commit message I will 
queue it to next release.

Thanks,
Michal

-- 
Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91
w: www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel - Xilinx Microblaze
Maintainer of Linux kernel - Xilinx Zynq ARM and ZynqMP ARM64 SoCs
U-Boot custodian - Xilinx Microblaze/Zynq/ZynqMP/Versal SoCs

Re: [PATCH] microblaze: remove CONFIG_SET_FS

[Arnd Bergmann]

On Wed, Feb 9, 2022 at 2:50 PM Michal Simek <monstr@monstr.eu> wrote:
>
> Hi Arnd,
>
> po 17. 1. 2022 v 14:28 odesílatel Arnd Bergmann <arnd@kernel.org> napsal:
> >
> > From: Arnd Bergmann <arnd@arndb.de>
> >
> > I picked microblaze as one of the architectures that still
> > use set_fs() and converted it not to.
>
> Can you please update the commit message because what is above is not
> the right one?

Ah, sorry about that. I think you can copy from the openrisc patch,
see https://lore.kernel.org/lkml/20220208064905.199632-1-shorne@gmail.com/

         Arnd

Re: [PATCH] microblaze: remove CONFIG_SET_FS

[Michal Simek]

On 2/9/22 15:40, Arnd Bergmann wrote:
> On Wed, Feb 9, 2022 at 2:50 PM Michal Simek <monstr@monstr.eu> wrote:
>>
>> Hi Arnd,
>>
>> po 17. 1. 2022 v 14:28 odesílatel Arnd Bergmann <arnd@kernel.org> napsal:
>>>
>>> From: Arnd Bergmann <arnd@arndb.de>
>>>
>>> I picked microblaze as one of the architectures that still
>>> use set_fs() and converted it not to.
>>
>> Can you please update the commit message because what is above is not
>> the right one?
> 
> Ah, sorry about that. I think you can copy from the openrisc patch,
> see https://lore.kernel.org/lkml/20220208064905.199632-1-shorne@gmail.com/

Please do it. You are the author of this patch and we should follow the process.
Link to riscv commit would be also useful.
Definitely thanks for this work and getting this to my attention.

Thanks,
Michal

Re: [PATCH] microblaze: remove CONFIG_SET_FS

[Arnd Bergmann]

On Wed, Feb 9, 2022 at 3:44 PM Michal Simek <michal.simek@xilinx.com> wrote:
> On 2/9/22 15:40, Arnd Bergmann wrote:
> > On Wed, Feb 9, 2022 at 2:50 PM Michal Simek <monstr@monstr.eu> wrote:
> >>
> >> Hi Arnd,
> >>
> >> po 17. 1. 2022 v 14:28 odesílatel Arnd Bergmann <arnd@kernel.org> napsal:
> >>>
> >>> From: Arnd Bergmann <arnd@arndb.de>
> >>>
> >>> I picked microblaze as one of the architectures that still
> >>> use set_fs() and converted it not to.
> >>
> >> Can you please update the commit message because what is above is not
> >> the right one?
> >
> > Ah, sorry about that. I think you can copy from the openrisc patch,
> > see https://lore.kernel.org/lkml/20220208064905.199632-1-shorne@gmail.com/
>
> Please do it. You are the author of this patch and we should follow the process.

Done.

Looking at it again, I wonder if it would help to use the __get_kernel_nofault()
and __get_kernel_nofault() helpers as the default in
include/asm-generic/uaccess.h.

I see it's identical to the openrisc version and would probably be the same
for some of the other architectures that have no other use for
set_fs(). That may
help to do a bulk remove of set_fs for alpha, arc, csky, h8300, hexagon, nds32,
nios2, um and extensa, leaving only ia64, sparc and sh.

        Arnd

Re: [PATCH] microblaze: remove CONFIG_SET_FS

[Stafford Horne]

On Wed, Feb 09, 2022 at 03:54:54PM +0100, Arnd Bergmann wrote:
> On Wed, Feb 9, 2022 at 3:44 PM Michal Simek <michal.simek@xilinx.com> wrote:
> > On 2/9/22 15:40, Arnd Bergmann wrote:
> > > On Wed, Feb 9, 2022 at 2:50 PM Michal Simek <monstr@monstr.eu> wrote:
> > >>
> > >> Hi Arnd,
> > >>
> > >> po 17. 1. 2022 v 14:28 odesílatel Arnd Bergmann <arnd@kernel.org> napsal:
> > >>>
> > >>> From: Arnd Bergmann <arnd@arndb.de>
> > >>>
> > >>> I picked microblaze as one of the architectures that still
> > >>> use set_fs() and converted it not to.
> > >>
> > >> Can you please update the commit message because what is above is not
> > >> the right one?
> > >
> > > Ah, sorry about that. I think you can copy from the openrisc patch,
> > > see https://lore.kernel.org/lkml/20220208064905.199632-1-shorne@gmail.com/
> >
> > Please do it. You are the author of this patch and we should follow the process.
> 
> Done.
> 
> Looking at it again, I wonder if it would help to use the __get_kernel_nofault()
> and __get_kernel_nofault() helpers as the default in
> include/asm-generic/uaccess.h.

That would make sense.  Perhaps also the __range_ok() function from OpenRISC
could move there as I think other architectures would also want to use that.

> I see it's identical to the openrisc version and would probably be the same
> for some of the other architectures that have no other use for
> set_fs(). That may
> help to do a bulk remove of set_fs for alpha, arc, csky, h8300, hexagon, nds32,
> nios2, um and extensa, leaving only ia64, sparc and sh.

If you could add it into include/asm-generic/uaccess.h I can test changing my
patch to use it.

-Stafford

Re: [PATCH] microblaze: remove CONFIG_SET_FS

[Stafford Horne]

On Thu, Feb 10, 2022 at 08:31:05AM +0900, Stafford Horne wrote:
> On Wed, Feb 09, 2022 at 03:54:54PM +0100, Arnd Bergmann wrote:
> > On Wed, Feb 9, 2022 at 3:44 PM Michal Simek <michal.simek@xilinx.com> wrote:
> > > On 2/9/22 15:40, Arnd Bergmann wrote:
> > > > On Wed, Feb 9, 2022 at 2:50 PM Michal Simek <monstr@monstr.eu> wrote:
> > > >>
> > > >> Hi Arnd,
> > > >>
> > > >> po 17. 1. 2022 v 14:28 odesílatel Arnd Bergmann <arnd@kernel.org> napsal:
> > > >>>
> > > >>> From: Arnd Bergmann <arnd@arndb.de>
> > > >>>
> > > >>> I picked microblaze as one of the architectures that still
> > > >>> use set_fs() and converted it not to.
> > > >>
> > > >> Can you please update the commit message because what is above is not
> > > >> the right one?
> > > >
> > > > Ah, sorry about that. I think you can copy from the openrisc patch,
> > > > see https://lore.kernel.org/lkml/20220208064905.199632-1-shorne@gmail.com/
> > >
> > > Please do it. You are the author of this patch and we should follow the process.
> > 
> > Done.
> > 
> > Looking at it again, I wonder if it would help to use the __get_kernel_nofault()
> > and __get_kernel_nofault() helpers as the default in
> > include/asm-generic/uaccess.h.
> 
> That would make sense.  Perhaps also the __range_ok() function from OpenRISC
> could move there as I think other architectures would also want to use that.
> 
> > I see it's identical to the openrisc version and would probably be the same
> > for some of the other architectures that have no other use for
> > set_fs(). That may
> > help to do a bulk remove of set_fs for alpha, arc, csky, h8300, hexagon, nds32,
> > nios2, um and extensa, leaving only ia64, sparc and sh.
> 
> If you could add it into include/asm-generic/uaccess.h I can test changing my
> patch to use it.

Note, I would be happy to do the work to move these into include/asm-generic/uaccess.h.
But as I see it the existing include/asm-generic/uaccess.h is for NOMMU.  How
should we go about having an MMU and NOMMU version?  Should we move uaccess.h to
uaccess-nommu.h?  Or add more ifdefs to uaccess.h?

-Stafford

Re: [PATCH] microblaze: remove CONFIG_SET_FS

[Arnd Bergmann]

 On Fri, Feb 11, 2022 at 1:17 AM Stafford Horne <shorne@gmail.com> wrote:
> On Thu, Feb 10, 2022 at 08:31:05AM +0900, Stafford Horne wrote:

> > > Looking at it again, I wonder if it would help to use the __get_kernel_nofault()
> > > and __get_kernel_nofault() helpers as the default in
> > > include/asm-generic/uaccess.h.
> >
> > That would make sense.  Perhaps also the __range_ok() function from OpenRISC
> > could move there as I think other architectures would also want to use that.

I have now uploaded a cleanup series to
https://git.kernel.org/pub/scm/linux/kernel/git/arnd/playground.git/log/?h=set_fs

This uses the same access_ok() function across almost all
architectures, with the
exception of those that need something else, and I then I went further
and killed
off set_fs for everything other than ia64.

> > > I see it's identical to the openrisc version and would probably be the same
> > > for some of the other architectures that have no other use for
> > > set_fs(). That may
> > > help to do a bulk remove of set_fs for alpha, arc, csky, h8300, hexagon, nds32,
> > > nios2, um and extensa, leaving only ia64, sparc and sh.
> >
> > If you could add it into include/asm-generic/uaccess.h I can test changing my
> > patch to use it.
>
> Note, I would be happy to do the work to move these into include/asm-generic/uaccess.h.
> But as I see it the existing include/asm-generic/uaccess.h is for NOMMU.  How
> should we go about having an MMU and NOMMU version?  Should we move uaccess.h to
> uaccess-nommu.h?  Or add more ifdefs to uaccess.h?

There are two parts of asm-generic/uaccess.h:

- the CONFIG_UACCESS_MEMCPYsection is fundamentally limited to nommu
  targets and cannot be shared. Similarly, targets with an MMU must use a custom
  implementation to get the correct fixups.

- the put_user/get_user implementation is fairly dumb, you can use these to
  avoid having your own ones, but you still need copy_{to,from}_user, and
  a custom implementation tends to produce better code.

So chances are that you won't want to use either one. In my new branch,
I added the common helpers to linux/uaccess.h and asm-generic/access-ok.h,
respectively, both of which are used everywhere now.

        Arnd

Re: [PATCH] microblaze: remove CONFIG_SET_FS

[Linus Torvalds]

On Fri, Feb 11, 2022 at 9:00 AM Arnd Bergmann <arnd@kernel.org> wrote:
>
> I have now uploaded a cleanup series to
> https://git.kernel.org/pub/scm/linux/kernel/git/arnd/playground.git/log/?h=set_fs
>
> This uses the same access_ok() function across almost all
> architectures, with the exception of those that need something else,
> and I then I went further and killed off set_fs for everything other
> than ia64.

Thanks, looks good to me.

Can you say why you didn't convert ia64? I don't see any set_fs() use
there, except for the unaligned handler, which looks trivial to
remove. It looks like the only reason for it is kernel-mode unaligned
exceptions, which we should just turn fatal, I suspect (they already
get logged).

And ia64 people could make the unaligned handling do the kernel mode
case in emulate_load/store_int() - it doesn't look *that* painful.

But maybe you noticed something else?

It would be really good to just be able to say that set_fs() no longer
exists at all.

                Linus

Re: [PATCH] microblaze: remove CONFIG_SET_FS

[Arnd Bergmann]

On Fri, Feb 11, 2022 at 6:46 PM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
> On Fri, Feb 11, 2022 at 9:00 AM Arnd Bergmann <arnd@kernel.org> wrote:
> >
> > I have now uploaded a cleanup series to
> > https://git.kernel.org/pub/scm/linux/kernel/git/arnd/playground.git/log/?h=set_fs
> >
> > This uses the same access_ok() function across almost all
> > architectures, with the exception of those that need something else,
> > and I then I went further and killed off set_fs for everything other
> > than ia64.
>
> Thanks, looks good to me.
>
> Can you say why you didn't convert ia64? I don't see any set_fs() use
> there, except for the unaligned handler, which looks trivial to
> remove. It looks like the only reason for it is kernel-mode unaligned
> exceptions, which we should just turn fatal, I suspect (they already
> get logged).
>
> And ia64 people could make the unaligned handling do the kernel mode
> case in emulate_load/store_int() - it doesn't look *that* painful.
>
> But maybe you noticed something else?
>
> It would be really good to just be able to say that set_fs() no longer
> exists at all.

I had previously gotten stuck at ia64, but gave it another go now
and uploaded an updated branch with ia64 taken care of and another
patch to clean up bits afterwards.

I only gave it light testing so far, mainly building the defconfig for every
architecture. I'll post the series once the build bots are happy with the
branch overall.

         Arnd

Re: [PATCH] microblaze: remove CONFIG_SET_FS

[Eric W. Biederman]

Arnd Bergmann <arnd@kernel.org> writes:

> On Fri, Feb 11, 2022 at 6:46 PM Linus Torvalds
> <torvalds@linux-foundation.org> wrote:
>> On Fri, Feb 11, 2022 at 9:00 AM Arnd Bergmann <arnd@kernel.org> wrote:
>> >
>> > I have now uploaded a cleanup series to
>> > https://git.kernel.org/pub/scm/linux/kernel/git/arnd/playground.git/log/?h=set_fs
>> >
>> > This uses the same access_ok() function across almost all
>> > architectures, with the exception of those that need something else,
>> > and I then I went further and killed off set_fs for everything other
>> > than ia64.
>>
>> Thanks, looks good to me.
>>
>> Can you say why you didn't convert ia64? I don't see any set_fs() use
>> there, except for the unaligned handler, which looks trivial to
>> remove. It looks like the only reason for it is kernel-mode unaligned
>> exceptions, which we should just turn fatal, I suspect (they already
>> get logged).
>>
>> And ia64 people could make the unaligned handling do the kernel mode
>> case in emulate_load/store_int() - it doesn't look *that* painful.
>>
>> But maybe you noticed something else?
>>
>> It would be really good to just be able to say that set_fs() no longer
>> exists at all.
>
> I had previously gotten stuck at ia64, but gave it another go now
> and uploaded an updated branch with ia64 taken care of and another
> patch to clean up bits afterwards.
>
> I only gave it light testing so far, mainly building the defconfig for every
> architecture. I'll post the series once the build bots are happy with the
> branch overall.
>

Thank you so much for doing this work.

Eric

Re: [PATCH] microblaze: remove CONFIG_SET_FS

[Stafford Horne]

On Fri, Feb 11, 2022 at 03:10:10PM -0600, Eric W. Biederman wrote:
> Arnd Bergmann <arnd@kernel.org> writes:
> >
> > I had previously gotten stuck at ia64, but gave it another go now
> > and uploaded an updated branch with ia64 taken care of and another
> > patch to clean up bits afterwards.
> >
> > I only gave it light testing so far, mainly building the defconfig for every
> > architecture. I'll post the series once the build bots are happy with the
> > branch overall.
> >
> 
> Thank you so much for doing this work.
> 

I'll echo this.  Thank you, the changes look good.  I test built and booted the
OpenRISC architecture and it works.

I can drop the openrisc only patch for this from the openrisc queue now.

-Stafford

Re: [PATCH] microblaze: remove CONFIG_SET_FS

[Christoph Hellwig]

On Fri, Feb 11, 2022 at 09:46:03AM -0800, Linus Torvalds wrote:
> Can you say why you didn't convert ia64? I don't see any set_fs() use
> there, except for the unaligned handler, which looks trivial to
> remove. It looks like the only reason for it is kernel-mode unaligned
> exceptions, which we should just turn fatal, I suspect (they already
> get logged).
> 
> And ia64 people could make the unaligned handling do the kernel mode
> case in emulate_load/store_int() - it doesn't look *that* painful.

Are there any ia64 people left? :)

Re: [PATCH] microblaze: remove CONFIG_SET_FS

[Christoph Hellwig]

I like the series a lot.

Superficial comments:

for nds32 is there any good reason why __get_user / __set_user check
the address limit directly?  Maybe we should unify this and make it work
like the other architectures.

With "uaccess: add generic __{get,put}_kernel_nofault" we should be able
to remove HAVE_GET_KERNEL_NOFAULT entirely and just check if the helpers
are already defined in linux/uaccess.h.

The new generic __access_ok, and the 3 fixed up version early on
have a whole lot of superflous braces.

Re: [PATCH] microblaze: remove CONFIG_SET_FS

[Arnd Bergmann]

On Mon, Feb 14, 2022 at 8:50 AM Christoph Hellwig <hch@infradead.org> wrote:
>
> I like the series a lot.
>
> Superficial comments:
>
> for nds32 is there any good reason why __get_user / __set_user check
> the address limit directly?  Maybe we should unify this and make it work
> like the other architectures.

I've done that now, and am glad I did, because I uncovered that
put_user() was actually missing the check that got added to __get_user()
by accident.

> With "uaccess: add generic __{get,put}_kernel_nofault" we should be able
> to remove HAVE_GET_KERNEL_NOFAULT entirely and just check if the helpers
> are already defined in linux/uaccess.h.

Good idea, changed now as well.

> The new generic __access_ok, and the 3 fixed up version early on
> have a whole lot of superflous braces.

I'd prefer to leave those in, the logic is complex enough that I'd
rather make sure this is completely obvious to readers.

I got a few built bot reports about missing __user annotations that were
uncovered by the added type checking in access_ok, fixed those
as well.

I'm doing another test build after the last changes, will send it out in a bit
if there are no further regressions.

        Arnd