[PATCH RFC v2] m68k: remove get_fs()/set_fs()

[PATCH RFC v2] m68k: remove get_fs()/set_fs()

[Michael Schmitz]

Followup to my patch "Fixes to Linus' 'remove set_fs patch'", with
kernel mode copy macros used in __get/put_kernel_nofault().
(As soon as I use the same __get/put_user_asm() macros with
parameters, like in Christoph's patch, everything falls to pieces.)

The fact that I had to touch include/linux/uaccess.h at all means
I got something horribly wrong in my use of MAKE_MM_SEG() to
set the segments. This is just to show what does happen to boot
and run OK, and perhaps help work out the differences to Christoph's
version that sadly crashes for me.

Link: https://lore.kernel.org/r/1625527229-3224-1-git-send-email-schmitzmic@gmail.com
Link: https://lore.kernel.org/r/20210707142531.GA26080@lst.de
CC: Christoph Hellwig <hch@lst.de>
CC: Linus Torvalds <torvalds@linux-foundation.org>
Signed-off-by: Michael Schmitz <schmitzmic@gmail.com>
---
 arch/m68k/Kconfig                   |   1 -
 arch/m68k/include/asm/segment.h     |  21 +-----
 arch/m68k/include/asm/thread_info.h |   2 +-
 arch/m68k/include/asm/tlbflush.h    |   7 +-
 arch/m68k/include/asm/uaccess.h     | 146 +++++++++++++++++++++++++++++++++++-
 arch/m68k/kernel/process.c          |   2 +-
 arch/m68k/kernel/traps.c            |  16 ++--
 arch/m68k/mm/cache.c                |  23 +++---
 arch/m68k/mm/init.c                 |   2 +-
 arch/m68k/mm/motorola.c             |   2 +-
 arch/m68k/sun3/config.c             |   2 +-
 arch/m68k/sun3/mmu_emu.c            |   7 +-
 include/linux/uaccess.h             |   3 -
 13 files changed, 181 insertions(+), 53 deletions(-)

diff --git a/arch/m68k/Kconfig b/arch/m68k/Kconfig
index deaea88..a9dd518 100644
--- a/arch/m68k/Kconfig
+++ b/arch/m68k/Kconfig
@@ -33,7 +33,6 @@ config M68K
 	select NO_DMA if !MMU && !COLDFIRE
 	select OLD_SIGACTION
 	select OLD_SIGSUSPEND3
-	select SET_FS
 	select UACCESS_MEMCPY if !MMU
 	select VIRT_TO_BUS
 
diff --git a/arch/m68k/include/asm/segment.h b/arch/m68k/include/asm/segment.h
index 2b5e68a..a9fabf7d 100644
--- a/arch/m68k/include/asm/segment.h
+++ b/arch/m68k/include/asm/segment.h
@@ -26,19 +26,9 @@ typedef struct {
 
 #ifdef CONFIG_CPU_HAS_ADDRESS_SPACES
 /*
- * Get/set the SFC/DFC registers for MOVES instructions
+ * Set the SFC/DFC registers for MOVES instructions
  */
-#define USER_DS		MAKE_MM_SEG(__USER_DS)
-#define KERNEL_DS	MAKE_MM_SEG(__KERNEL_DS)
-
-static inline mm_segment_t get_fs(void)
-{
-	mm_segment_t _v;
-	__asm__ ("movec %/dfc,%0":"=r" (_v.seg):);
-	return _v;
-}
-
-static inline void set_fs(mm_segment_t val)
+static inline void set_segment(mm_segment_t val)
 {
 	__asm__ __volatile__ ("movec %0,%/sfc\n\t"
 			      "movec %0,%/dfc\n\t"
@@ -46,14 +36,9 @@ static inline void set_fs(mm_segment_t val)
 }
 
 #else
-#define USER_DS		MAKE_MM_SEG(TASK_SIZE)
-#define KERNEL_DS	MAKE_MM_SEG(0xFFFFFFFF)
-#define get_fs()	(current_thread_info()->addr_limit)
-#define set_fs(x)	(current_thread_info()->addr_limit = (x))
+#define set_segment(x)	((void)(x))
 #endif
 
-#define uaccess_kernel()	(get_fs().seg == KERNEL_DS.seg)
-
 #endif /* __ASSEMBLY__ */
 
 #endif /* _M68K_SEGMENT_H */
diff --git a/arch/m68k/include/asm/thread_info.h b/arch/m68k/include/asm/thread_info.h
index d813fed..6a93808 100644
--- a/arch/m68k/include/asm/thread_info.h
+++ b/arch/m68k/include/asm/thread_info.h
@@ -37,7 +37,7 @@ struct thread_info {
 #define INIT_THREAD_INFO(tsk)			\
 {						\
 	.task		= &tsk,			\
-	.addr_limit	= KERNEL_DS,		\
+	.addr_limit	= MAKE_MM_SEG(SUPER_DATA),		\
 	.preempt_count	= INIT_PREEMPT_COUNT,	\
 }
 
diff --git a/arch/m68k/include/asm/tlbflush.h b/arch/m68k/include/asm/tlbflush.h
index 5337bc2..c6a85d4 100644
--- a/arch/m68k/include/asm/tlbflush.h
+++ b/arch/m68k/include/asm/tlbflush.h
@@ -13,13 +13,14 @@ static inline void flush_tlb_kernel_page(void *addr)
 	if (CPU_IS_COLDFIRE) {
 		mmu_write(MMUOR, MMUOR_CNL);
 	} else if (CPU_IS_040_OR_060) {
-		mm_segment_t old_fs = get_fs();
-		set_fs(KERNEL_DS);
+		preempt_disable();
+		set_segment(MAKE_MM_SEG(SUPER_DATA));
 		__asm__ __volatile__(".chip 68040\n\t"
 				     "pflush (%0)\n\t"
 				     ".chip 68k"
 				     : : "a" (addr));
-		set_fs(old_fs);
+		set_segment(MAKE_MM_SEG(USER_DATA));
+		preempt_enable();
 	} else if (CPU_IS_020_OR_030)
 		__asm__ __volatile__("pflush #4,#4,(%0)" : : "a" (addr));
 }
diff --git a/arch/m68k/include/asm/uaccess.h b/arch/m68k/include/asm/uaccess.h
index f98208c..e7273e7 100644
--- a/arch/m68k/include/asm/uaccess.h
+++ b/arch/m68k/include/asm/uaccess.h
@@ -182,6 +182,149 @@ asm volatile ("\n"					\
 })
 #define get_user(x, ptr) __get_user(x, ptr)
 
+#define HAVE_GET_KERNEL_NOFAULT
+
+#define __put_kernel_asm(res, x, ptr, bwl, reg, err)	\
+asm volatile ("\n"					\
+	"1:	move."#bwl"	%2,%1\n"		\
+	"2:\n"						\
+	"	.section .fixup,\"ax\"\n"		\
+	"	.even\n"				\
+	"10:	moveq.l	%3,%0\n"			\
+	"	jra 2b\n"				\
+	"	.previous\n"				\
+	"\n"						\
+	"	.section __ex_table,\"a\"\n"		\
+	"	.align	4\n"				\
+	"	.long	1b,10b\n"			\
+	"	.long	2b,10b\n"			\
+	"	.previous"				\
+	: "+d" (res), "=m" (*(ptr))			\
+	: #reg (x), "i" (err))
+
+#define __get_kernel_asm(res, x, ptr, type, bwl, reg, err) ({		\
+	type __gu_val;							\
+	asm volatile ("\n"						\
+		"1:	move."#bwl"	%2,%1\n"			\
+		"2:\n"							\
+		"	.section .fixup,\"ax\"\n"			\
+		"	.even\n"					\
+		"10:	move.l	%3,%0\n"				\
+		"	sub.l	%1,%1\n"				\
+		"	jra	2b\n"					\
+		"	.previous\n"					\
+		"\n"							\
+		"	.section __ex_table,\"a\"\n"			\
+		"	.align	4\n"					\
+		"	.long	1b,10b\n"				\
+		"	.previous"					\
+		: "+d" (res), "=&" #reg (__gu_val)			\
+		: "m" (*(ptr)), "i" (err));				\
+	(x) = (__force typeof(*(ptr)))(__force unsigned long)__gu_val;	\
+})
+
+
+#define __put_kernel_nofault(dst, src, type, err_label)			\
+do {									\
+	type __pu_val;					\
+	int __pu_err = 0;						\
+									\
+	__pu_val = *(__force type *)(src);				\
+	switch (sizeof(type)) {						\
+	case 1:								\
+		__put_kernel_asm(__pu_err, __pu_val, (type *)(dst), b, d, -EFAULT);	\
+		break;							\
+	case 2:								\
+		__put_kernel_asm(__pu_err, __pu_val, (type *)(dst), w, r, -EFAULT);	\
+		break;							\
+	case 4:								\
+		__put_kernel_asm(__pu_err, __pu_val, (type *)(dst), l, r, -EFAULT);	\
+		break;							\
+	case 8:								\
+ 	    {								\
+ 		const void __user *__pu_ptr = (type *)(dst);			\
+		asm volatile ("\n"					\
+			"1:	movel	%2,(%1)+\n"		\
+			"2:	movel	%R2,(%1)\n"		\
+			"3:\n"						\
+			"	.section .fixup,\"ax\"\n"		\
+			"	.even\n"				\
+			"10:	movel %3,%0\n"				\
+			"	jra 3b\n"				\
+			"	.previous\n"				\
+			"\n"						\
+			"	.section __ex_table,\"a\"\n"		\
+			"	.align 4\n"				\
+			"	.long 1b,10b\n"				\
+			"	.long 2b,10b\n"				\
+			"	.long 3b,10b\n"				\
+			"	.previous"				\
+			: "+d" (__pu_err), "+a" (__pu_ptr)		\
+			: "r" (__pu_val), "i" (-EFAULT)			\
+			: "memory");					\
+		break;							\
+	    }								\
+	default:							\
+		BUILD_BUG();						\
+		break;							\
+	}								\
+	if (unlikely(__pu_err))						\
+		goto err_label;						\
+} while (0)
+
+#define __get_kernel_nofault(dst, src, type, err_label)			\
+do {									\
+	int __gu_err;							\
+									\
+	switch (sizeof(type)) {						\
+	case 1:								\
+		__get_kernel_asm(__gu_err, *(type *)(dst), (__force type *)(src), u8, b, d, -EFAULT);	\
+		break;							\
+	case 2:								\
+		__get_kernel_asm(__gu_err, *(type *)(dst), (__force type *)(src), u16, w, r, -EFAULT);	\
+		break;							\
+	case 4:								\
+		__get_kernel_asm(__gu_err, *(type *)(dst), (__force type *)(src), u32, l, r, -EFAULT);	\
+		break;							\
+	case 8: {							\
+		const void __user *__gu_ptr = (__force type *)(src);			\
+		union {							\
+			u64 l;						\
+			__typeof__((__force type *)(src)) t;				\
+		} __gu_val;						\
+		asm volatile ("\n"					\
+			"1:	move.l	(%2)+,%1\n"		\
+			"2:	move.l	(%2),%R1\n"		\
+			"3:\n"						\
+			"	.section .fixup,\"ax\"\n"		\
+			"	.even\n"				\
+			"10:	move.l	%3,%0\n"			\
+			"	sub.l	%1,%1\n"			\
+			"	sub.l	%R1,%R1\n"			\
+			"	jra	3b\n"				\
+			"	.previous\n"				\
+			"\n"						\
+			"	.section __ex_table,\"a\"\n"		\
+			"	.align	4\n"				\
+			"	.long	1b,10b\n"			\
+			"	.long	2b,10b\n"			\
+			"	.previous"				\
+			: "+d" (__gu_err), "=&r" (__gu_val.l),		\
+			  "+a" (__gu_ptr)				\
+			: "i" (-EFAULT)					\
+			: "memory");					\
+		(dst) = __gu_val.t;				\
+		break;							\
+	}								\
+	default:							\
+		BUILD_BUG();						\
+		break;							\
+	}								\
+	if (unlikely(__gu_err))						\
+		goto err_label;						\
+} while (0)
+
+
 unsigned long __generic_copy_from_user(void *to, const void __user *from, unsigned long n);
 unsigned long __generic_copy_to_user(void __user *to, const void *from, unsigned long n);
 
@@ -380,9 +523,6 @@ raw_copy_to_user(void __user *to, const void *from, unsigned long n)
 #define INLINE_COPY_FROM_USER
 #define INLINE_COPY_TO_USER
 
-#define user_addr_max() \
-	(uaccess_kernel() ? ~0UL : TASK_SIZE)
-
 extern long strncpy_from_user(char *dst, const char __user *src, long count);
 extern __must_check long strnlen_user(const char __user *str, long n);
 
diff --git a/arch/m68k/kernel/process.c b/arch/m68k/kernel/process.c
index da83cc8..f1c3e68 100644
--- a/arch/m68k/kernel/process.c
+++ b/arch/m68k/kernel/process.c
@@ -155,7 +155,7 @@ int copy_thread(unsigned long clone_flags, unsigned long usp, unsigned long arg,
 	 * Must save the current SFC/DFC value, NOT the value when
 	 * the parent was last descheduled - RGH  10-08-96
 	 */
-	p->thread.fs = get_fs().seg;
+	p->thread.fs = USER_DATA;
 
 	if (unlikely(p->flags & (PF_KTHREAD | PF_IO_WORKER))) {
 		/* kernel thread */
diff --git a/arch/m68k/kernel/traps.c b/arch/m68k/kernel/traps.c
index 9e12614..f74205a 100644
--- a/arch/m68k/kernel/traps.c
+++ b/arch/m68k/kernel/traps.c
@@ -181,9 +181,9 @@ static inline void access_error060 (struct frame *fp)
 static inline unsigned long probe040(int iswrite, unsigned long addr, int wbs)
 {
 	unsigned long mmusr;
-	mm_segment_t old_fs = get_fs();
 
-	set_fs(MAKE_MM_SEG(wbs));
+	preempt_disable();
+	set_segment(MAKE_MM_SEG(wbs));
 
 	if (iswrite)
 		asm volatile (".chip 68040; ptestw (%0); .chip 68k" : : "a" (addr));
@@ -192,7 +192,8 @@ static inline unsigned long probe040(int iswrite, unsigned long addr, int wbs)
 
 	asm volatile (".chip 68040; movec %%mmusr,%0; .chip 68k" : "=r" (mmusr));
 
-	set_fs(old_fs);
+	set_segment(MAKE_MM_SEG(USER_DATA));
+	preempt_enable();
 
 	return mmusr;
 }
@@ -201,10 +202,9 @@ static inline int do_040writeback1(unsigned short wbs, unsigned long wba,
 				   unsigned long wbd)
 {
 	int res = 0;
-	mm_segment_t old_fs = get_fs();
 
-	/* set_fs can not be moved, otherwise put_user() may oops */
-	set_fs(MAKE_MM_SEG(wbs));
+	preempt_disable();
+	set_segment(MAKE_MM_SEG(wbs));
 
 	switch (wbs & WBSIZ_040) {
 	case BA_SIZE_BYTE:
@@ -218,8 +218,8 @@ static inline int do_040writeback1(unsigned short wbs, unsigned long wba,
 		break;
 	}
 
-	/* set_fs can not be moved, otherwise put_user() may oops */
-	set_fs(old_fs);
+	set_segment(MAKE_MM_SEG(USER_DATA));
+	preempt_enable();
 
 
 	pr_debug("do_040writeback1, res=%d\n", res);
diff --git a/arch/m68k/mm/cache.c b/arch/m68k/mm/cache.c
index b486c08..cb513f6 100644
--- a/arch/m68k/mm/cache.c
+++ b/arch/m68k/mm/cache.c
@@ -12,7 +12,7 @@
 #include <asm/traps.h>
 
 
-static unsigned long virt_to_phys_slow(unsigned long vaddr)
+static unsigned long virt_to_phys_slow(unsigned long vaddr, mm_segment_t seg)
 {
 	if (CPU_IS_060) {
 		unsigned long paddr;
@@ -55,7 +55,7 @@ static unsigned long virt_to_phys_slow(unsigned long vaddr)
 		asm volatile ("ptestr %3,%2@,#7,%0\n\t"
 			      "pmove %%psr,%1"
 			      : "=a&" (descaddr), "=m" (mmusr)
-			      : "a" (vaddr), "d" (get_fs().seg));
+			      : "a" (vaddr), "d" (seg.seg));
 		if (mmusr & (MMU_I|MMU_B|MMU_L))
 			return 0;
 		descaddr = phys_to_virt((unsigned long)descaddr);
@@ -73,7 +73,7 @@ static unsigned long virt_to_phys_slow(unsigned long vaddr)
 
 /* Push n pages at kernel virtual address and clear the icache */
 /* RZ: use cpush %bc instead of cpush %dc, cinv %ic */
-void flush_icache_user_range(unsigned long address, unsigned long endaddr)
+static void do_flush_icache_range(unsigned long address, unsigned long endaddr, mm_segment_t seg)
 {
 	if (CPU_IS_COLDFIRE) {
 		unsigned long start, end;
@@ -92,7 +92,7 @@ void flush_icache_user_range(unsigned long address, unsigned long endaddr)
 				      ".chip 68040\n\t"
 				      "cpushp %%bc,(%0)\n\t"
 				      ".chip 68k"
-				      : : "a" (virt_to_phys_slow(address)));
+				      : : "a" (virt_to_phys_slow(address, seg)));
 			address += PAGE_SIZE;
 		} while (address < endaddr);
 	} else {
@@ -105,13 +105,18 @@ void flush_icache_user_range(unsigned long address, unsigned long endaddr)
 	}
 }
 
-void flush_icache_range(unsigned long address, unsigned long endaddr)
+void flush_icache_user_range(unsigned long address, unsigned long endaddr)
 {
-	mm_segment_t old_fs = get_fs();
+	do_flush_icache_range(address, endaddr, MAKE_MM_SEG(USER_DATA));
+}
 
-	set_fs(KERNEL_DS);
-	flush_icache_user_range(address, endaddr);
-	set_fs(old_fs);
+void flush_icache_range(unsigned long address, unsigned long endaddr)
+{
+	preempt_disable();
+	set_segment(MAKE_MM_SEG(SUPER_DATA));
+	do_flush_icache_range(address, endaddr, MAKE_MM_SEG(SUPER_DATA));
+	set_segment(MAKE_MM_SEG(USER_DATA));
+	preempt_enable();
 }
 EXPORT_SYMBOL(flush_icache_range);
 
diff --git a/arch/m68k/mm/init.c b/arch/m68k/mm/init.c
index 1759ab8..7a8d2b6 100644
--- a/arch/m68k/mm/init.c
+++ b/arch/m68k/mm/init.c
@@ -96,7 +96,7 @@ void __init paging_init(void)
 	/*
 	 * Set up SFC/DFC registers (user data space).
 	 */
-	set_fs (USER_DS);
+	set_segment(USER_DATA);
 
 	max_zone_pfn[ZONE_DMA] = end_mem >> PAGE_SHIFT;
 	free_area_init(max_zone_pfn);
diff --git a/arch/m68k/mm/motorola.c b/arch/m68k/mm/motorola.c
index 3a653f0..307c3ad 100644
--- a/arch/m68k/mm/motorola.c
+++ b/arch/m68k/mm/motorola.c
@@ -467,7 +467,7 @@ void __init paging_init(void)
 	/*
 	 * Set up SFC/DFC registers
 	 */
-	set_fs(KERNEL_DS);
+	set_segment(MAKE_MM_SEG(SUPER_DATA));
 
 #ifdef DEBUG
 	printk ("before free_area_init\n");
diff --git a/arch/m68k/sun3/config.c b/arch/m68k/sun3/config.c
index f7dd472..0e72766 100644
--- a/arch/m68k/sun3/config.c
+++ b/arch/m68k/sun3/config.c
@@ -89,7 +89,7 @@ void __init sun3_init(void)
 	sun3_reserved_pmeg[249] = 1;
 	sun3_reserved_pmeg[252] = 1;
 	sun3_reserved_pmeg[253] = 1;
-	set_fs(KERNEL_DS);
+	set_segment(SUPER_DATA);
 }
 
 /* Without this, Bad Things happen when something calls arch_reset. */
diff --git a/arch/m68k/sun3/mmu_emu.c b/arch/m68k/sun3/mmu_emu.c
index 7aa879b..f5b57aa 100644
--- a/arch/m68k/sun3/mmu_emu.c
+++ b/arch/m68k/sun3/mmu_emu.c
@@ -191,14 +191,15 @@ void __init mmu_emu_init(unsigned long bootmem_end)
 	for(seg = 0; seg < PAGE_OFFSET; seg += SUN3_PMEG_SIZE)
 		sun3_put_segmap(seg, SUN3_INVALID_PMEG);
 
-	set_fs(MAKE_MM_SEG(3));
+	preempt_disable();
+	set_segment(MAKE_MM_SEG(3));
 	for(seg = 0; seg < 0x10000000; seg += SUN3_PMEG_SIZE) {
 		i = sun3_get_segmap(seg);
 		for(j = 1; j < CONTEXTS_NUM; j++)
 			(*(romvec->pv_setctxt))(j, (void *)seg, i);
 	}
-	set_fs(KERNEL_DS);
-
+	set_segment(SUPER_DATA);
+	preempt_enable();
 }
 
 /* erase the mappings for a dead context.  Uses the pg_dir for hints
diff --git a/include/linux/uaccess.h b/include/linux/uaccess.h
index c05e903..ec43aff 100644
--- a/include/linux/uaccess.h
+++ b/include/linux/uaccess.h
@@ -29,9 +29,6 @@ static inline void force_uaccess_end(mm_segment_t oldfs)
 	set_fs(oldfs);
 }
 #else /* CONFIG_SET_FS */
-typedef struct {
-	/* empty dummy */
-} mm_segment_t;
 
 #ifndef TASK_SIZE_MAX
 #define TASK_SIZE_MAX			TASK_SIZE
-- 
2.7.4

Re: [PATCH RFC v2] m68k: remove get_fs()/set_fs()

[Linus Torvalds]

On Wed, Jul 7, 2021 at 6:48 PM Michael Schmitz <schmitzmic@gmail.com> wrote:
>
> Followup to my patch "Fixes to Linus' 'remove set_fs patch'", with
> kernel mode copy macros used in __get/put_kernel_nofault().
> (As soon as I use the same __get/put_user_asm() macros with
> parameters, like in Christoph's patch, everything falls to pieces.)

Ok, strange. If this works for you, then the _concept_ is fine, and
there's something odd going on with the "macros with 'moves' vs 'move'
as a parameter" thing.

> The fact that I had to touch include/linux/uaccess.h at all means
> I got something horribly wrong in my use of MAKE_MM_SEG() to
> set the segments.

Oh, that's straightforward enough: the m68k segment code uses the same
'mm_segment_t' type that the geberic kernel just wants to make empty
for the non-CONFIG_SET_FS case.

So the m68k segment type should just use a different name.

In fact, the m68k manuals call them "alternate function code
registers", so maybe it shouldn't be about "mm_segment_t" at all, but
the type should really be named as such.

The "segment" naming is just legacy x86 nomenclature, so I suspect it
would be a good thing to really walk away from that and make it very
explicit about what this is on m68k.

So maybe it could be

    typedef struct {
        unsigned char val:3;
    } function_code_reg_t;

or something like that.

I suspect some real m68k person should make up the name, not me. The
alternate function code registers are all from after my time ;)

              Linus

Re: [PATCH RFC v2] m68k: remove get_fs()/set_fs()

[Linus Torvalds]

On Wed, Jul 7, 2021 at 7:51 PM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> Ok, strange. If this works for you, then the _concept_ is fine, and
> there's something odd going on with the "macros with 'moves' vs 'move'
> as a parameter" thing.

Oh, I think I see the problem.

Or rather, I see it in Christoph's version of the patch, I don't think
I've seen Michael's version, but that one was apparently very similar,
so maybe it has the same bug.

Christoph does:

   #define __put_user_asm(inst, res, x, ptr, bwl, reg)    \
   asm volatile ("\n"                                     \
          "1:     #inst."#bwl"    %2,%1\n"
   ...

and then uses it with code like

                __put_user_asm(MOVES, __pu_err, __pu_val, ptr, b, d);

and

                __get_user_asm("move", __gk_err, __gk_dst, __gk_src, u8, b, d);

and the issue is that there's a '#' too many.

The '#' turns the argument into a string, but it was already
_supposed_ to be a string. But no, the problem is that it turns the
macro name MOVES into the _string_ "MOVES".

Which happens to compile just fine, because "moves" is a real
instruction. But it's actually _meant_ to be a macro that expands to
either the string "moves" or "move".

So what happens is that at least in Christoph's version, I think the
code _always_ uses "MOVES", even in configurations where the macro
MOVES should have just become "move".

So it all builds fine, looks fine to the assembler, but it uses the
wrong instruction.

Macro expansion with the '#' character and other macros used as
arguments is something people need to be very careful with.  It's why
we have a whole header file for just the "stringify" operation, see
<linux/stringify.h>

But in this case, it shouldn't have used '#' at all, since the
argument was already a string, and never needed to be turned into a
string by the pre-processor.

             Linus

Re: [PATCH RFC v2] m68k: remove get_fs()/set_fs()

[Michael Schmitz]

Hi Linus,

Am 08.07.2021 um 15:01 schrieb Linus Torvalds:
> On Wed, Jul 7, 2021 at 7:51 PM Linus Torvalds
> <torvalds@linux-foundation.org> wrote:
>>
>> Ok, strange. If this works for you, then the _concept_ is fine, and
>> there's something odd going on with the "macros with 'moves' vs 'move'
>> as a parameter" thing.
>
> Oh, I think I see the problem.
>
> Or rather, I see it in Christoph's version of the patch, I don't think
> I've seen Michael's version, but that one was apparently very similar,
> so maybe it has the same bug.
>
> Christoph does:
>
>    #define __put_user_asm(inst, res, x, ptr, bwl, reg)    \
>    asm volatile ("\n"                                     \
>           "1:     #inst."#bwl"    %2,%1\n"
>    ...
>
> and then uses it with code like
>
>                 __put_user_asm(MOVES, __pu_err, __pu_val, ptr, b, d);
>
> and
>
>                 __get_user_asm("move", __gk_err, __gk_dst, __gk_src, u8, b, d);
>
> and the issue is that there's a '#' too many.
>
> The '#' turns the argument into a string, but it was already
> _supposed_ to be a string. But no, the problem is that it turns the
> macro name MOVES into the _string_ "MOVES".
>
> Which happens to compile just fine, because "moves" is a real
> instruction. But it's actually _meant_ to be a macro that expands to
> either the string "moves" or "move".

Yes, that's where I got to (trying to implement what you suggested a few
days ago, after the version I just sent) and hit a wall. What I meant by
'getting overly smart with the preprocessor'.

>
> So what happens is that at least in Christoph's version, I think the
> code _always_ uses "MOVES", even in configurations where the macro
> MOVES should have just become "move".
>
> So it all builds fine, looks fine to the assembler, but it uses the
> wrong instruction.

Yep - that was easy enough to test - never mind what you define MOVES
as, that does not really matter. It always turns out 'moves'.

> Macro expansion with the '#' character and other macros used as
> arguments is something people need to be very careful with.  It's why
> we have a whole header file for just the "stringify" operation, see
> <linux/stringify.h>
>
> But in this case, it shouldn't have used '#' at all, since the
> argument was already a string, and never needed to be turned into a
> string by the pre-processor.

Testing that option next... and works!. Though I still think Christoph's
version is much cleaner...

Cheers,

	Michael

>
>              Linus
>

Re: [PATCH RFC v2] m68k: remove get_fs()/set_fs()

[Christoph Hellwig]

On Wed, Jul 07, 2021 at 07:51:45PM -0700, Linus Torvalds wrote:
> So maybe it could be
> 
>     typedef struct {
>         unsigned char val:3;
>     } function_code_reg_t;
> 
> or something like that.
> 
> I suspect some real m68k person should make up the name, not me. The
> alternate function code registers are all from after my time ;)

I think the typedef is a little pointless that is why I dropped it
entirely (mm_segment_t should go away entirely anyway).  We could
reduce the storage space for it, but that always requireѕ changing
inline assembly, so I'd rather let a real m68k person do it if they
care.

Re: [PATCH RFC v2] m68k: remove get_fs()/set_fs()

[Christoph Hellwig]

On Wed, Jul 07, 2021 at 08:01:33PM -0700, Linus Torvalds wrote:
> The '#' turns the argument into a string, but it was already
> _supposed_ to be a string. But no, the problem is that it turns the
> macro name MOVES into the _string_ "MOVES".
> 
> Which happens to compile just fine, because "moves" is a real
> instruction. But it's actually _meant_ to be a macro that expands to
> either the string "moves" or "move".
> 
> So what happens is that at least in Christoph's version, I think the
> code _always_ uses "MOVES", even in configurations where the macro
> MOVES should have just become "move".
> 
> So it all builds fine, looks fine to the assembler, but it uses the
> wrong instruction.
> 
> Macro expansion with the '#' character and other macros used as
> arguments is something people need to be very careful with.  It's why
> we have a whole header file for just the "stringify" operation, see
> <linux/stringify.h>
> 
> But in this case, it shouldn't have used '#' at all, since the
> argument was already a string, and never needed to be turned into a
> string by the pre-processor.

Yes, that problem exists, but just removing the # causes "inst" to go
into the cpp output.  Let me brush up my cpp-foo.

Re: [PATCH RFC v2] m68k: remove get_fs()/set_fs()

[Michael Schmitz]

Hi Christoph,

Am 08.07.2021 um 16:31 schrieb Christoph Hellwig:
> On Wed, Jul 07, 2021 at 08:01:33PM -0700, Linus Torvalds wrote:
>> The '#' turns the argument into a string, but it was already
>> _supposed_ to be a string. But no, the problem is that it turns the
>> macro name MOVES into the _string_ "MOVES".
>>
>> Which happens to compile just fine, because "moves" is a real
>> instruction. But it's actually _meant_ to be a macro that expands to
>> either the string "moves" or "move".
>>
>> So what happens is that at least in Christoph's version, I think the
>> code _always_ uses "MOVES", even in configurations where the macro
>> MOVES should have just become "move".
>>
>> So it all builds fine, looks fine to the assembler, but it uses the
>> wrong instruction.
>>
>> Macro expansion with the '#' character and other macros used as
>> arguments is something people need to be very careful with.  It's why
>> we have a whole header file for just the "stringify" operation, see
>> <linux/stringify.h>
>>
>> But in this case, it shouldn't have used '#' at all, since the
>> argument was already a string, and never needed to be turned into a
>> string by the pre-processor.
>
> Yes, that problem exists, but just removing the # causes "inst" to go
> into the cpp output.  Let me brush up my cpp-foo.

Putting quotes around 'inst' makes it compile for me.

Cheers,

	Michael

Re: [PATCH RFC v2] m68k: remove get_fs()/set_fs()

[Christoph Hellwig]

On Thu, Jul 08, 2021 at 04:33:08PM +1200, Michael Schmitz wrote:
>> Yes, that problem exists, but just removing the # causes "inst" to go
>> into the cpp output.  Let me brush up my cpp-foo.
>
> Putting quotes around 'inst' makes it compile for me.

Do you just want to take the series over from here?  I think you are
about 15 times more qualified than me anyway :)

Re: [PATCH RFC v2] m68k: remove get_fs()/set_fs()

[Michael Schmitz]

Hi Christoph,

Am 08.07.2021 um 16:58 schrieb Christoph Hellwig:
> On Thu, Jul 08, 2021 at 04:33:08PM +1200, Michael Schmitz wrote:
>>> Yes, that problem exists, but just removing the # causes "inst" to go
>>> into the cpp output.  Let me brush up my cpp-foo.
>>
>> Putting quotes around 'inst' makes it compile for me.
>
> Do you just want to take the series over from here?  I think you are
> about 15 times more qualified than me anyway :)

Not in a million years :-) A brief look at my patch review history on 
linux-m68k will disabuse you of that notion. I routinely hit v6 and 
higher ... I'm a simple hobbyist, and a little out of my depth here.

Cheers,

	Michael

Re: [PATCH RFC v2] m68k: remove get_fs()/set_fs()

[Andreas Schwab]

On Jul 07 2021, Linus Torvalds wrote:

> Christoph does:
>
>    #define __put_user_asm(inst, res, x, ptr, bwl, reg)    \
>    asm volatile ("\n"                                     \
>           "1:     #inst."#bwl"    %2,%1\n"
>    ...
>
> and then uses it with code like
>
>                 __put_user_asm(MOVES, __pu_err, __pu_val, ptr, b, d);
>
> and
>
>                 __get_user_asm("move", __gk_err, __gk_dst, __gk_src, u8, b, d);
>
> and the issue is that there's a '#' too many.
>
> The '#' turns the argument into a string,

No, it's inside a string, and the assembler just sees a comment starting
with `#inst.'.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."

Re: [PATCH RFC v2] m68k: remove get_fs()/set_fs()

[Christoph Hellwig]

On Thu, Jul 08, 2021 at 05:48:46PM +1200, Michael Schmitz wrote:
> Hi Christoph,
>
> Am 08.07.2021 um 16:58 schrieb Christoph Hellwig:
>> On Thu, Jul 08, 2021 at 04:33:08PM +1200, Michael Schmitz wrote:
>>>> Yes, that problem exists, but just removing the # causes "inst" to go
>>>> into the cpp output.  Let me brush up my cpp-foo.
>>>
>>> Putting quotes around 'inst' makes it compile for me.
>>
>> Do you just want to take the series over from here?  I think you are
>> about 15 times more qualified than me anyway :)
>
> Not in a million years :-) A brief look at my patch review history on 
> linux-m68k will disabuse you of that notion. I routinely hit v6 and higher 
> ... I'm a simple hobbyist, and a little out of my depth here.

I've force pushed a new version to the branch, can you give it a spin?

Re: [PATCH RFC v2] m68k: remove get_fs()/set_fs()

[Linus Torvalds]

On Thu, Jul 8, 2021 at 5:57 AM Christoph Hellwig <hch@lst.de> wrote:
>
> I've force pushed a new version to the branch, can you give it a spin?

Please stop playing broken games with __constant_copy_to_user().

Now you didn't just break the return value, you broke the actual copy
too. When it is supposed to do a 4-byte copy, the code now does *two*
4-byte copies, because that's the way __constant_copy_to_user_asm()
works - it always does at least two accesses, and then the third one
is conditional.

So that "6, l, l, )" in

        case 4:
                __constant_copy_to_user_asm(res, to, from, tmp, 6, l, l,);
                break;

literally means "try to do 2x 'l' sized moves (but not a third one),
and return 6 if it fails". All of which is very wrong indeed.

So commit d36105c942e0 ("m68k: simplify the __constant_copy_to_user
implementation") is very very broken.

But the rest looks good to me. Of course, I entirely missed the fact
that Andreas pointed out - "instr" was inside a string - so who knows
what I missed this time.

               Linus

Re: [PATCH RFC v2] m68k: remove get_fs()/set_fs()

[Michael Schmitz]

Hi Christoph,

On 9/07/21 12:57 am, Christoph Hellwig wrote:
> On Thu, Jul 08, 2021 at 05:48:46PM +1200, Michael Schmitz wrote:
>> Hi Christoph,
>>
>> Am 08.07.2021 um 16:58 schrieb Christoph Hellwig:
>>> On Thu, Jul 08, 2021 at 04:33:08PM +1200, Michael Schmitz wrote:
>>>>> Yes, that problem exists, but just removing the # causes "inst" to go
>>>>> into the cpp output.  Let me brush up my cpp-foo.
>>>> Putting quotes around 'inst' makes it compile for me.
>>> Do you just want to take the series over from here?  I think you are
>>> about 15 times more qualified than me anyway :)
>> Not in a million years :-) A brief look at my patch review history on
>> linux-m68k will disabuse you of that notion. I routinely hit v6 and higher
>> ... I'm a simple hobbyist, and a little out of my depth here.
> I've force pushed a new version to the branch, can you give it a spin?

Still the same fault (can't start init). But as Linus pointed out, your 
__constant_copy_to_user() is still broken.

I'll try to back out that part sometime later today.

Cheers,

     Michael

Re: [PATCH RFC v2] m68k: remove get_fs()/set_fs()

[Michael Schmitz]

Hi Linus,

On 9/07/21 6:20 am, Linus Torvalds wrote:
> On Thu, Jul 8, 2021 at 5:57 AM Christoph Hellwig <hch@lst.de> wrote:
>> I've force pushed a new version to the branch, can you give it a spin?
> Please stop playing broken games with __constant_copy_to_user().
>
> Now you didn't just break the return value, you broke the actual copy
> too. When it is supposed to do a 4-byte copy, the code now does *two*
> 4-byte copies, because that's the way __constant_copy_to_user_asm()
> works - it always does at least two accesses, and then the third one
> is conditional.
>
> So that "6, l, l, )" in
>
>          case 4:
>                  __constant_copy_to_user_asm(res, to, from, tmp, 6, l, l,);
>                  break;
>
> literally means "try to do 2x 'l' sized moves (but not a third one),
> and return 6 if it fails". All of which is very wrong indeed.

In order to get the correct number of bytes copied, the patch would have 
to look like:

        switch (n) {
         case 1:
-               __put_user_asm(res, *(u8 *)from, (u8 __user *)to, b, d, 1);
+               __constant_copy_to_user_asm(res, to, from, tmp, 1, b, );
                 break;
         case 2:
-               __put_user_asm(res, *(u16 *)from, (u16 __user *)to, w, 
r, 2);
+               __constant_copy_to_user_asm(res, to, from, tmp, 2, w, );
                 break;
         case 3:
                 __constant_copy_to_user_asm(res, to, from, tmp, 3, w, b,);
                 break;
         case 4:
-               __put_user_asm(res, *(u32 *)from, (u32 __user *)to, l, 
r, 4);
+               __constant_copy_to_user_asm(res, to, from, tmp, 4, l, );
                 break;

and __constant_copy_to_user_asm() changed to deal with an empty s2 
parameter. Probably too much work.


>
> So commit d36105c942e0 ("m68k: simplify the __constant_copy_to_user
> implementation") is very very broken.

Doesn't appear to matter - though I'll back out those changes to be safe.

Note that in this version, faults in __put_user_asm used from 
__constant_copy_to_user will return -EFAULT, not the remaining number of 
bytes to be copied, which might get confusing for the caller.

Cheers,

     Michael

>
> But the rest looks good to me. Of course, I entirely missed the fact
> that Andreas pointed out - "instr" was inside a string - so who knows
> what I missed this time.
>
>                 Linus

Re: [PATCH RFC v2] m68k: remove get_fs()/set_fs()

[Michael Schmitz]

Hi Christoph,

On 9/07/21 7:28 am, Michael Schmitz wrote:
>
> On 9/07/21 12:57 am, Christoph Hellwig wrote:
>> I've force pushed a new version to the branch, can you give it a spin?
>
> Still the same fault (can't start init). But as Linus pointed out, 
> your __constant_copy_to_user() is still broken.

Forget that - got my git am munged up (applied yesterday's patch a 
second time, eek).

That patch works fine on a casual test. What you did to 
__constant_copy_to_user() does not appear to matter - but I haven't put 
the system under any kind of stress yet. I'm a little reluctant to do 
that (recovering from a trashed boot disk is a little dicey), I'll 
probably only try that with your changes to __constant_copy_to_user() 
from commit d36105c942e0 backed out.

That still leaves the issue of __constant_copy_to_user() possibly 
returning -EFAULT instead of the number of residual bytes to copy, but 
we can probably live with that for now (__constant_copy_to_user_asm() 
doesn't actually calculate the residual but just sets it to the number 
of bytes originally to be copied, which is the best we can do with the 
scheme used there).

Cheers,

     Michael

Re: [PATCH RFC v2] m68k: remove get_fs()/set_fs()

[Christoph Hellwig]

On Fri, Jul 09, 2021 at 12:31:45PM +1200, Michael Schmitz wrote:
> That patch works fine on a casual test. What you did to 
> __constant_copy_to_user() does not appear to matter - but I haven't put the 
> system under any kind of stress yet. I'm a little reluctant to do that 
> (recovering from a trashed boot disk is a little dicey), I'll probably only 
> try that with your changes to __constant_copy_to_user() from commit 
> d36105c942e0 backed out.

As Linus pointed out, small copy_to_user basically doesn't happen as
we have switched all the suspect call sites to just use put_user.

Geert: do you care about __constant_copy_to_user at all, or can we just
kill it (as well as the copy_from_user side)?

Re: [PATCH RFC v2] m68k: remove get_fs()/set_fs()

[Michael Schmitz]

Hi Christoph,

Am 09.07.2021 um 16:22 schrieb Christoph Hellwig:
> On Fri, Jul 09, 2021 at 12:31:45PM +1200, Michael Schmitz wrote:
>> That patch works fine on a casual test. What you did to
>> __constant_copy_to_user() does not appear to matter - but I haven't put the
>> system under any kind of stress yet. I'm a little reluctant to do that
>> (recovering from a trashed boot disk is a little dicey), I'll probably only
>> try that with your changes to __constant_copy_to_user() from commit
>> d36105c942e0 backed out.
>
> As Linus pointed out, small copy_to_user basically doesn't happen as
> we have switched all the suspect call sites to just use put_user.

Thanks, I missed that. That does explain it then.

I see you've put a warning message in set_fc() - let's see whether that 
triggers on a test run on real hardware.

Cheers,

	Michael

> Geert: do you care about __constant_copy_to_user at all, or can we just
> kill it (as well as the copy_from_user side)?
>

Re: [PATCH RFC v2] m68k: remove get_fs()/set_fs()

[Geert Uytterhoeven]

Hi Christoph,

On Fri, Jul 9, 2021 at 6:22 AM Christoph Hellwig <hch@lst.de> wrote:
> On Fri, Jul 09, 2021 at 12:31:45PM +1200, Michael Schmitz wrote:
> > That patch works fine on a casual test. What you did to
> > __constant_copy_to_user() does not appear to matter - but I haven't put the
> > system under any kind of stress yet. I'm a little reluctant to do that
> > (recovering from a trashed boot disk is a little dicey), I'll probably only
> > try that with your changes to __constant_copy_to_user() from commit
> > d36105c942e0 backed out.
>
> As Linus pointed out, small copy_to_user basically doesn't happen as
> we have switched all the suspect call sites to just use put_user.
>
> Geert: do you care about __constant_copy_to_user at all, or can we just
> kill it (as well as the copy_from_user side)?

If it blocks you, feel free to remove it.
BTW, do you have an idea of how many calls use small sizes?

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH RFC v2] m68k: remove get_fs()/set_fs()

[Michael Schmitz]

Hi Geert,

Am 09.07.2021 um 19:29 schrieb Geert Uytterhoeven:
> Hi Christoph,
>
> On Fri, Jul 9, 2021 at 6:22 AM Christoph Hellwig <hch@lst.de> wrote:
>> On Fri, Jul 09, 2021 at 12:31:45PM +1200, Michael Schmitz wrote:
>>> That patch works fine on a casual test. What you did to
>>> __constant_copy_to_user() does not appear to matter - but I haven't put the
>>> system under any kind of stress yet. I'm a little reluctant to do that
>>> (recovering from a trashed boot disk is a little dicey), I'll probably only
>>> try that with your changes to __constant_copy_to_user() from commit
>>> d36105c942e0 backed out.
>>
>> As Linus pointed out, small copy_to_user basically doesn't happen as
>> we have switched all the suspect call sites to just use put_user.
>>
>> Geert: do you care about __constant_copy_to_user at all, or can we just
>> kill it (as well as the copy_from_user side)?
>
> If it blocks you, feel free to remove it.
> BTW, do you have an idea of how many calls use small sizes?

Just booting to a login prompt: Quite a few with 8 bytes, some with 4 
bytes (those from the keyboard driver), the rest is 16, 24, 36 and 92 
bytes (a lot of those). Used ratelimiting so probably missed a few.

I'll repeat that again logging only the small sizes, but I think we can 
live with using the generic version for those few 4 and 8 byte cases.

Cheers,

	Michael

>
> Gr{oetje,eeting}s,
>
>                         Geert
>

Re: [PATCH RFC v2] m68k: remove get_fs()/set_fs()

[Christoph Hellwig]

On Fri, Jul 09, 2021 at 08:34:07PM +1200, Michael Schmitz wrote:
> Just booting to a login prompt: Quite a few with 8 bytes, some with 4 bytes 
> (those from the keyboard driver), the rest is 16, 24, 36 and 92 bytes (a 

The 4/8 byte ones can usually be switched to just use get/put_user easily.

Re: [PATCH RFC v2] m68k: remove get_fs()/set_fs()

[Michael Schmitz]

Am 09.07.2021 um 20:53 schrieb Christoph Hellwig:
> On Fri, Jul 09, 2021 at 08:34:07PM +1200, Michael Schmitz wrote:
>> Just booting to a login prompt: Quite a few with 8 bytes, some with 4 bytes
>> (those from the keyboard driver), the rest is 16, 24, 36 and 92 bytes (a

Over 400x 8 bytes, 20x 4 bytes on a count with the larger one suppressed.

>
> The 4/8 byte ones can usually be switched to just use get/put_user easily.

Need to log the call sites then ... that should be fun. Something to do 
tomorrow :-)

Cheers,

	Michael

Re: [PATCH RFC v2] m68k: remove get_fs()/set_fs()

[Geert Uytterhoeven]

Hi Michael,

On Fri, Jul 9, 2021 at 11:00 AM Michael Schmitz <schmitzmic@gmail.com> wrote:
> Am 09.07.2021 um 20:53 schrieb Christoph Hellwig:
> > On Fri, Jul 09, 2021 at 08:34:07PM +1200, Michael Schmitz wrote:
> >> Just booting to a login prompt: Quite a few with 8 bytes, some with 4 bytes
> >> (those from the keyboard driver), the rest is 16, 24, 36 and 92 bytes (a
>
> Over 400x 8 bytes, 20x 4 bytes on a count with the larger one suppressed.
>
> >
> > The 4/8 byte ones can usually be switched to just use get/put_user easily.
>
> Need to log the call sites then ... that should be fun. Something to do
> tomorrow :-)

WARN_ON_ONCE() is your friend.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH RFC v2] m68k: remove get_fs()/set_fs()

[Geert Uytterhoeven]

Hi Christoph,

On Fri, Jul 9, 2021 at 10:53 AM Christoph Hellwig <hch@lst.de> wrote:
> On Fri, Jul 09, 2021 at 08:34:07PM +1200, Michael Schmitz wrote:
> > Just booting to a login prompt: Quite a few with 8 bytes, some with 4 bytes
> > (those from the keyboard driver), the rest is 16, 24, 36 and 92 bytes (a
>
> The 4/8 byte ones can usually be switched to just use get/put_user easily.

Yeah, that seems like the best path forward.
I'm positively surprised that getting rid of the constant size
handling shrinks the kernel by ca. 10 KiB.  Probably it made sense
a long time ago (perhaps before the fixup sections were added,
if anyone still remembers?)...

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH RFC v2] m68k: remove get_fs()/set_fs()

[Michael Schmitz]

Hi Geert,

Am 09.07.2021 um 23:20 schrieb Geert Uytterhoeven:
> Hi Michael,
>
> On Fri, Jul 9, 2021 at 11:00 AM Michael Schmitz <schmitzmic@gmail.com> wrote:
>> Am 09.07.2021 um 20:53 schrieb Christoph Hellwig:
>>> On Fri, Jul 09, 2021 at 08:34:07PM +1200, Michael Schmitz wrote:
>>>> Just booting to a login prompt: Quite a few with 8 bytes, some with 4 bytes
>>>> (those from the keyboard driver), the rest is 16, 24, 36 and 92 bytes (a
>>
>> Over 400x 8 bytes, 20x 4 bytes on a count with the larger one suppressed.
>>
>>>
>>> The 4/8 byte ones can usually be switched to just use get/put_user easily.
>>
>> Need to log the call sites then ... that should be fun. Something to do
>> tomorrow :-)
>
> WARN_ON_ONCE() is your friend.

Sure - here's the result:

WARNING: CPU: 0 PID: 1 at ./arch/m68k/include/asm/uaccess.h:395 
raw_copy_to_user.constprop.27+0x26/0x42
WARNING: CPU: 0 PID: 819 at ./arch/m68k/include/asm/uaccess.h:395 
do_pipe2+0x4a/0xbc
WARNING: CPU: 0 PID: 838 at ./arch/m68k/include/asm/uaccess.h:395 
__put_old_timespec32+0x36/0x58
WARNING: CPU: 0 PID: 854 at ./arch/m68k/include/asm/uaccess.h:303 
vt_do_kdsk_ioctl+0x38/0x28a
WARNING: CPU: 0 PID: 950 at ./arch/m68k/include/asm/uaccess.h:395 
tty_ioctl+0x27a/0x6c4

uaccess.h:303 is __constant_copy_from_user.

Cheers,

	Michael


> Gr{oetje,eeting}s,
>
>                         Geert
>

Re: [PATCH RFC v2] m68k: remove get_fs()/set_fs()

[Michael Schmitz]

Am 10.07.2021 um 07:25 schrieb Michael Schmitz:
> Hi Geert,
>
> Am 09.07.2021 um 23:20 schrieb Geert Uytterhoeven:
>> Hi Michael,
>>
>> On Fri, Jul 9, 2021 at 11:00 AM Michael Schmitz <schmitzmic@gmail.com>
>> wrote:
>>> Am 09.07.2021 um 20:53 schrieb Christoph Hellwig:
>>>> On Fri, Jul 09, 2021 at 08:34:07PM +1200, Michael Schmitz wrote:
>>>>> Just booting to a login prompt: Quite a few with 8 bytes, some with
>>>>> 4 bytes
>>>>> (those from the keyboard driver), the rest is 16, 24, 36 and 92
>>>>> bytes (a
>>>
>>> Over 400x 8 bytes, 20x 4 bytes on a count with the larger one
>>> suppressed.
>>>
>>>>
>>>> The 4/8 byte ones can usually be switched to just use get/put_user
>>>> easily.
>>>
>>> Need to log the call sites then ... that should be fun. Something to do
>>> tomorrow :-)
>>
>> WARN_ON_ONCE() is your friend.
>
> Sure - here's the result:
>
> WARNING: CPU: 0 PID: 1 at ./arch/m68k/include/asm/uaccess.h:395
> raw_copy_to_user.constprop.27+0x26/0x42

That one got called from sys_llseek+0x5c/0x86

Cheers,

	Michael


> WARNING: CPU: 0 PID: 819 at ./arch/m68k/include/asm/uaccess.h:395
> do_pipe2+0x4a/0xbc
> WARNING: CPU: 0 PID: 838 at ./arch/m68k/include/asm/uaccess.h:395
> __put_old_timespec32+0x36/0x58
> WARNING: CPU: 0 PID: 854 at ./arch/m68k/include/asm/uaccess.h:303
> vt_do_kdsk_ioctl+0x38/0x28a
> WARNING: CPU: 0 PID: 950 at ./arch/m68k/include/asm/uaccess.h:395
> tty_ioctl+0x27a/0x6c4
>
> uaccess.h:303 is __constant_copy_from_user.
>
> Cheers,
>
>     Michael
>
>
>> Gr{oetje,eeting}s,
>>
>>                         Geert
>>

Re: [PATCH RFC v2] m68k: remove get_fs()/set_fs()

[Linus Torvalds]

On Fri, Jul 9, 2021 at 12:26 PM Michael Schmitz <schmitzmic@gmail.com> wrote:
>
> WARNING: CPU: 0 PID: 854 at ./arch/m68k/include/asm/uaccess.h:303
> vt_do_kdsk_ioctl+0x38/0x28a

It's apparently the

        if (copy_from_user(&kbe, user_kbe, sizeof(struct kbentry)))

which is 4 bytes in size.

Honestly, it could be done with a get_user(), but it could also just
be ignored. We're not talking performance-sensitive code here.

x86 got rid of the constant-sized user copy games not because they
don't exist, but because they just weren't worth the pain.

If somebody really wants to do it, then I suspect it would be better
to special-case the 1/2/4/8 byte case in generic code, than have
architectures special-case the odd sizes.

But it really shouldn't matter.

On x86, we found that

 (a) inlining get_user() was bad, because it just caused lots of
duplicated code for the range checks.

 (b) turning copy_to_user() into get_user() didn't really matter and
caused tons of duplicated code

 (c) the (few) places that cared about performance should use neither,
and should use "uaccess_begin() + unsafe_get_user() + uaccess_end()"
because the reason they mattered was that they ended up doing multiple
accesses and the overhead was elsewhere.

Of course, on x86, the big reason for (c) is that the biggest cost
often ends up being the "enable/disable user space accesses" overhead,
which is about security and a feature that m68k doesn't have.

So the trace-offs are certainly a bit different on x86, but on the
whole I suspect the lessons from x86 are not wrong.

               Linus

Re: [PATCH RFC v2] m68k: remove get_fs()/set_fs()

[Linus Torvalds]

On Fri, Jul 9, 2021 at 12:52 PM Michael Schmitz <schmitzmic@gmail.com> wrote:
>
> That one got called from sys_llseek+0x5c/0x86

That's the

        if (offset >= 0) {
                retval = -EFAULT;
                if (!copy_to_user(result, &offset, sizeof(offset)))
                        retval = 0;
        }

and it might be worth doing a put_user() for. That would actually
simplify the code to just

        if (offset >= 0)
                retval = put_user(offset, result);

or something like that.

Except maybe there's some 32-bit architecture that doesn't support
8-byte get/put_user(), which may be why it's a copy_to_user().

I _think_ we made the rule be that everybody had to support 1/2/4/8
byte accesses, but maybe I remember incorrectly.

             Linus

Re: [PATCH RFC v2] m68k: remove get_fs()/set_fs()

[Linus Torvalds]

On Fri, Jul 9, 2021 at 1:03 PM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> Except maybe there's some 32-bit architecture that doesn't support
> 8-byte get/put_user(), which may be why it's a copy_to_user().
>
> I _think_ we made the rule be that everybody had to support 1/2/4/8
> byte accesses, but maybe I remember incorrectly.

We're _almost_ correct.

nios2 doesn't seem to support 8-byte user accesses.

Everybody else does seem to have a 'case 8:' at least according to my
simplistic grep (but that grep might have been _too_ simplistic).

Added nios2 and the arch list.

I think we could easily just say "you have to support a 8-byte
get/put_user()". At worst, an architecture can just implement it using
copy_from_user() like some already do, ie something like

        case 8: {                                                       \
                u64 __x;                                                \
                if (unlikely(copy_from_user(&__x, ptr, 8))) {         \
                        retval = -EFAULT;                               \
                        (x) = (__typeof__(*(ptr)))0;                    \
                } else {                                                \
                        (x) = *(__force __typeof__(*(ptr)) *)&__x;      \
                }                                                       \
                break;                                                  \
        }                                                               \

so if somebody wants to make 'sys_llseek()' use put_user(), I'm ok with that.

              Linus

Re: [PATCH RFC v2] m68k: remove get_fs()/set_fs()

[Michael Schmitz]

Hi Linus,

Am 10.07.2021 um 07:53 schrieb Linus Torvalds:
> On Fri, Jul 9, 2021 at 12:26 PM Michael Schmitz <schmitzmic@gmail.com> wrote:
>>
>> WARNING: CPU: 0 PID: 854 at ./arch/m68k/include/asm/uaccess.h:303
>> vt_do_kdsk_ioctl+0x38/0x28a
>
> It's apparently the
>
>         if (copy_from_user(&kbe, user_kbe, sizeof(struct kbentry)))
>
> which is 4 bytes in size.
>
> Honestly, it could be done with a get_user(), but it could also just
> be ignored. We're not talking performance-sensitive code here.

Yep - that one was quite rare. But I haven't actually done anything 
meaningful on the system yet :-)

The 8 byte copies are by far the majority. Got another one, just after 
logging in:

tty_ioctl+0x2ca

I'll run that instrumented version on my 030 for a IO stress test once 
the current test has finished. Just for the record - the 
WARN_ON_ONCE(in_interrupt) that Christoph added in set_fc() hasn't 
triggered yet.

Cheers,

	Michael


> x86 got rid of the constant-sized user copy games not because they
> don't exist, but because they just weren't worth the pain.
>
> If somebody really wants to do it, then I suspect it would be better
> to special-case the 1/2/4/8 byte case in generic code, than have
> architectures special-case the odd sizes.
>
> But it really shouldn't matter.
>
> On x86, we found that
>
>  (a) inlining get_user() was bad, because it just caused lots of
> duplicated code for the range checks.
>
>  (b) turning copy_to_user() into get_user() didn't really matter and
> caused tons of duplicated code
>
>  (c) the (few) places that cared about performance should use neither,
> and should use "uaccess_begin() + unsafe_get_user() + uaccess_end()"
> because the reason they mattered was that they ended up doing multiple
> accesses and the overhead was elsewhere.
>
> Of course, on x86, the big reason for (c) is that the biggest cost
> often ends up being the "enable/disable user space accesses" overhead,
> which is about security and a feature that m68k doesn't have.
>
> So the trace-offs are certainly a bit different on x86, but on the
> whole I suspect the lessons from x86 are not wrong.
>
>                Linus
>

Re: [PATCH RFC v2] m68k: remove get_fs()/set_fs()

[Linus Torvalds]

On Fri, Jul 9, 2021 at 2:08 PM Michael Schmitz <schmitzmic@gmail.com> wrote:
>
> The 8 byte copies are by far the majority.

llseek might actually not be unusual. And the timeval structure is - I
think - 8 bytes on 32-bit architectures and common for select timeouts
etc. And select/poll can be one of the most common system calls out
there depending on loads (usually graphical programs).

But again - I don't think it's even worth worrying about in
architecture code. If you actually can measure it, I think we should
fix it in generic code rather than have architectures work around some
issue one by one.

I think the main issue for m68k should be "is it stable and works". At
least first. And then worry about copy_to/from_user() with a constant
size as a very very distant second concern.

              Linus

Re: [PATCH RFC v2] m68k: remove get_fs()/set_fs()

[Michael Schmitz]

Hi Linus,

Am 10.07.2021 um 09:18 schrieb Linus Torvalds:
> On Fri, Jul 9, 2021 at 2:08 PM Michael Schmitz <schmitzmic@gmail.com> wrote:
>>
>> The 8 byte copies are by far the majority.
>
> llseek might actually not be unusual. And the timeval structure is - I
> think - 8 bytes on 32-bit architectures and common for select timeouts
> etc. And select/poll can be one of the most common system calls out
> there depending on loads (usually graphical programs).

There's more const short reads in the old IDE code (1, 4 and 7 bytes, 
the latter two in the ioctl syscall), and one 8 byte one in the libata 
code (ata_get_identity()). None of this looks anywhere near performance 
critical as llseek or select/poll.

> But again - I don't think it's even worth worrying about in
> architecture code. If you actually can measure it, I think we should
> fix it in generic code rather than have architectures work around some
> issue one by one.

I doubt it makes enough of a difference to measure, but maybe the llseek 
impact is large enough to show in IO benchmarks. I'll run tests on 
Christoph's latest patch series anyway so I'll have data to decide that.

> I think the main issue for m68k should be "is it stable and works". At
> least first. And then worry about copy_to/from_user() with a constant
> size as a very very distant second concern.

So far, it looks like there's no regressions on either 030 or 040. Can't 
see much difference on 060 other than its use of sfc/dfc in the ifpsp060 
support code, but that's all inside 'unimplemented instruction' trap 
handlers that can't be preempted AFAIK.

I'll ask Adrian to test these patches on 060 as well though.

Cheers,

	Michael

>
>               Linus
>