[PATCH] microblaze: remove CONFIG_SET_FS

Arnd Bergmann posted 1 patch 4 years, 5 months ago
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(-)
[PATCH] microblaze: remove CONFIG_SET_FS
Posted by Arnd Bergmann 4 years, 5 months ago
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
Posted by Michal Simek 4 years, 4 months ago
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
Posted by Christoph Hellwig 4 years, 4 months ago
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
Posted by Michal Simek 4 years, 4 months ago

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
Posted by Arnd Bergmann 4 years, 4 months ago
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
Posted by Michal Simek 4 years, 4 months ago

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
Posted by Arnd Bergmann 4 years, 4 months ago
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
Posted by Stafford Horne 4 years, 4 months ago
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