Skip to contentSkip to conversation

Fixes for uaccess.h with gcc >= 4.0.1

PATCH Ralf Baechle 8 participants 14 messages Jan 23, 2006 – Feb 14, 2006 MIPS

lore
Share & tools
lore

Ralf Baechle

author
#0 lore raw
Quoted 7 lines · Martin Michlmayr
Quoted 6 lines · Ralf Baechle
Quoted 6 lines

With current linux-mips git I still get the problem. As a reminder, the error is:

CC fs/compat_ioctl.o
fs/compat_ioctl.c: In function 'fd_ioctl_trans':
fs/compat_ioctl.c:1831: error: read-only variable '__gu_val' used as 'asm' output

Quoted 8 lines
Quoted 3 lines

I have not been able to find some construct that keeps gcc happy and at the same time doesn't result in significantly worse code. That matters because get_user / put_user are used very often throughout the kernel.

Who could help with that? Maciej? David Daney?

(See the original discussion and the proposed patch at
http://www.spinics.net/lists/mips/msg20671.html)

I'd appreciate if somebody with gcc 4.0.1 could test this kernel patch below.

Ralf

 include/asm-mips/uaccess.h |   71 ++++++++++++++++++++++-----------------------
 1 files changed, 36 insertions(+), 35 deletions(-)
include/asm-mips/uaccess.h +36 -35
Index: linux-mips/include/asm-mips/uaccess.h
===================================================================
@@ -202,49 +202,49 @@ struct __large_struct { unsigned long bu
  * Yuck.  We need two variants, one for 64bit operation and one
  * for 32 bit mode and old iron.
  */
-#ifdef __mips64
-#define __GET_USER_DW(ptr) __get_user_asm("ld", ptr)
-#else
-#define __GET_USER_DW(ptr) __get_user_asm_ll32(ptr)
+#ifdef CONFIG_32BIT
+#define __GET_USER_DW(val, ptr) __get_user_asm_ll32(val, ptr)
+#endif
+#ifdef CONFIG_64BIT
+#define __GET_USER_DW(val, ptr) __get_user_asm(val, "ld", ptr)
 #endif
 
-#define __get_user_nocheck(x,ptr,size)					\
-({									\
-	__typeof(*(ptr)) __gu_val =  (__typeof(*(ptr))) 0;		\
-	long __gu_err = 0;						\
-									\
+extern void __get_user_unknown(void);
+
+#define __get_user_common(val, size, ptr)				\
+do {									\
 	switch (size) {							\
-	case 1: __get_user_asm("lb", ptr); break;			\
-	case 2: __get_user_asm("lh", ptr); break;			\
-	case 4: __get_user_asm("lw", ptr); break;			\
-	case 8: __GET_USER_DW(ptr); break;				\
+	case 1: __get_user_asm(val, "lb", ptr); break;			\
+	case 2: __get_user_asm(val, "lh", ptr); break;			\
+	case 4: __get_user_asm(val, "lw", ptr); break;			\
+	case 8: __GET_USER_DW(val, ptr); break;				\
 	default: __get_user_unknown(); break;				\
 	}								\
-	(x) = (__typeof__(*(ptr))) __gu_val;				\
+} while (0)
+
+#define __get_user_nocheck(x,ptr,size)					\
+({									\
+	long __gu_err;							\
+									\
+	__get_user_common((x), size, ptr);				\
 	__gu_err;							\
 })
 
 #define __get_user_check(x,ptr,size)					\
 ({									\
-	const __typeof__(*(ptr)) __user * __gu_addr = (ptr);		\
-	__typeof__(*(ptr)) __gu_val = 0;				\
 	long __gu_err = -EFAULT;					\
+	void * __gu_ptr = (ptr);					\
+									\
+	if (likely(access_ok(VERIFY_READ,  __gu_ptr, size)))		\
+		__get_user_common((x), size, __gu_ptr);			\
 									\
-	if (likely(access_ok(VERIFY_READ,  __gu_addr, size))) {		\
-		switch (size) {						\
-		case 1: __get_user_asm("lb", __gu_addr); break;		\
-		case 2: __get_user_asm("lh", __gu_addr); break;		\
-		case 4: __get_user_asm("lw", __gu_addr); break;		\
-		case 8: __GET_USER_DW(__gu_addr); break;		\
-		default: __get_user_unknown(); break;			\
-		}							\
-	}								\
-	(x) = (__typeof__(*(ptr))) __gu_val;				\
 	__gu_err;							\
 })
 
-#define __get_user_asm(insn, addr)					\
+#define __get_user_asm(val, insn, addr)					\
 {									\
+	long __gu_tmp;							\
+									\
 	__asm__ __volatile__(						\
 	"1:	" insn "	%1, %3				\n"	\
 	"2:							\n"	\

@@ -255,14 +255,16 @@ struct __large_struct { unsigned long bu
 	"	.section __ex_table,\"a\"			\n"	\
 	"	"__UA_ADDR "\t1b, 3b				\n"	\
 	"	.previous					\n"	\
-	: "=r" (__gu_err), "=r" (__gu_val)				\
+	: "=r" (__gu_err), "=r" (__gu_tmp)				\
 	: "0" (0), "o" (__m(addr)), "i" (-EFAULT));			\
+									\
+	(val) = (__typeof__(val)) __gu_tmp;				\
 }
 
 /*
  * Get a long long 64 using 32 bit registers.
  */
-#define __get_user_asm_ll32(addr)					\
+#define __get_user_asm_ll32(val, addr)					\
 {									\
 	__asm__ __volatile__(						\
 	"1:	lw	%1, (%3)				\n"	\

@@ -278,21 +280,20 @@ struct __large_struct { unsigned long bu
 	"	" __UA_ADDR "	1b, 4b				\n"	\
 	"	" __UA_ADDR "	2b, 4b				\n"	\
 	"	.previous					\n"	\
-	: "=r" (__gu_err), "=&r" (__gu_val)				\
+	: "=r" (__gu_err), "=&r" (val)					\
 	: "0" (0), "r" (addr), "i" (-EFAULT));				\
 }
 
-extern void __get_user_unknown(void);
-
 /*
  * Yuck.  We need two variants, one for 64bit operation and one
  * for 32 bit mode and old iron.
  */
-#ifdef __mips64
-#define __PUT_USER_DW(ptr) __put_user_asm("sd", ptr)
-#else
+#ifdef CONFIG_32BIT
 #define __PUT_USER_DW(ptr) __put_user_asm_ll32(ptr)
 #endif
+#ifdef CONFIG_64BIT
+#define __PUT_USER_DW(ptr) __put_user_asm("sd", ptr)
+#endif
 
 #define __put_user_nocheck(x,ptr,size)					\
 ({									\
#1 lore raw
Quoted 4 lines · Ralf Baechle

I'd appreciate if somebody with gcc 4.0.1 could test this kernel patch below.

Ralf

Is that for 2.4?
2.6 doesn't seem to have that problem..

author
#2 lore raw
Quoted 7 lines · P. Christeas
Quoted 4 lines · Ralf Baechle

I'd appreciate if somebody with gcc 4.0.1 could test this kernel patch below.

Ralf

Is that for 2.4?

2.4 is a no go for all architectures with gcc >= 4.0.0 and in case of MIPS even gcc 3.4 is somewhat dubious.

Quoted 1 line

2.6 doesn't seem to have that problem..

It's probably a matter of configuration then. Basically with our current uaccess.h and gcc >= 4.0.1 the attempt to pass a pointer to a const variable as the pointer argument to get_user or __get_user will blow up. It's always been a bug - but gcc before 4.0.1 were accepting this silently.

Ralf

#3 lore raw
In reply to Ralf Baechle
Quoted 14 lines · Ralf Baechle
Quoted 7 lines · P. Christeas
Quoted 4 lines · Ralf Baechle

I'd appreciate if somebody with gcc 4.0.1 could test this kernel patch below.

Ralf

Is that for 2.4?

2.4 is a no go for all architectures with gcc >= 4.0.0 and in case of MIPS even gcc 3.4 is somewhat dubious.

Quoted 1 line

2.6 doesn't seem to have that problem..

Quoted 7 lines

It's probably a matter of configuration then. Basically with our current uaccess.h and gcc >= 4.0.1 the attempt to pass a pointer to a const variable as the pointer argument to get_user or __get_user will blow up. It's always been a bug - but gcc before 4.0.1 were accepting this silently.

Ralf

I 've been compiling with gcc 4.0.2 (my tree is Linus') and haven't seen any message like that. It all compiles fine. Is there a point in testing your patch as well?

#6 lore raw
In reply to Ralf Baechle
Quoted 4 lines · Ralf Baechle

I'd appreciate if somebody with gcc 4.0.1 could test this kernel patch below.

Works for me. The compilation errors are gone and the kernel seems to be running fine.

Thanks
Rojhalat Ibrahim

#4 lore raw
In reply to P. Christeas
Quoted 21 lines · P. Christeas
Quoted 20 lines · Ralf Baechle
Quoted 7 lines · P. Christeas
Quoted 4 lines · Ralf Baechle

I'd appreciate if somebody with gcc 4.0.1 could test this kernel patch below.

Ralf

Is that for 2.4?

2.4 is a no go for all architectures with gcc >= 4.0.0 and in case of MIPS even gcc 3.4 is somewhat dubious.

Quoted 1 line

2.6 doesn't seem to have that problem..

It's probably a matter of configuration then. Basically with our current uaccess.h and gcc >= 4.0.1 the attempt to pass a pointer to a const variable as the pointer argument to get_user or __get_user will blow up. It's always been a bug - but gcc before 4.0.1 were accepting this silently.

Quoted 4 lines
Quoted 1 line

Ralf

I 've been compiling with gcc 4.0.2 (my tree is Linus') and haven't seen any message like that.

The case I saw happened for 32bit compat ioctls in a 64bit kernel.

Quoted 2 lines

It all compiles fine. Is there a point in testing your patch as well?

Well, if you want to be sure it doesn't break your system...

Thiemo

#9 lore raw
In reply to Ralf Baechle
Quoted 2 lines · Ralf Baechle

I'd appreciate if somebody with gcc 4.0.1 could test this kernel patch below.

I get the following warning that I didn't get with Stuart's patch but apart from that it compiles and boots (Cobalt 64-bit kernel, 32-bit userland).

CC arch/mips/kernel/linux32.o
arch/mips/kernel/linux32.c: In function ‘sysn32_rt_sigtimedwait’:
arch/mips/kernel/linux32.c:1464: warning: initialization discards qualifiers from pointer target type arch/mips/kernel/linux32.c:1465: warning: initialization discards qualifiers from pointer target type
CC arch/mips/kernel/signal32.o

Signature · 2 lines
Martin Michlmayr
http://www.cyrius.com/
author
#7 lore raw
In reply to Rojhalat Ibrahim
Quoted 6 lines · Rojhalat Ibrahim
Quoted 3 lines

I'd appreciate if somebody with gcc 4.0.1 could test this kernel patch below.

Works for me. The compilation errors are gone and the kernel seems to be running fine.

Excellent, thanks for testing. I pushed the patch to lmo's git repository.

Ralf

#8 lore raw
Quoted 10 lines · Ralf Baechle
Quoted 6 lines · Rojhalat Ibrahim
Quoted 3 lines

I'd appreciate if somebody with gcc 4.0.1 could test this kernel patch below.

Works for me. The compilation errors are gone and the kernel seems to be running fine.

Excellent, thanks for testing. I pushed the patch to lmo's git repository.

I'm still playing catch up a bit from traveling, but I replaced my fix with this one in git, and confirmed that the kernel built and ran as well as it did with the old fix.

Stuart

Stuart R. Anderson                               [email protected]
Network & Software Engineering                   http://www.netsweng.com/
1024D/37A79149:                                  0791 D3B8 9A4C 2CDC A31F
                                                 BD03 0A62 E534 37A7 9149
#10 lore raw
In reply to Ralf Baechle
Quoted 2 lines · Ralf Baechle

I'd appreciate if somebody with gcc 4.0.1 could test this kernel patch below.

Sorry in advance if this is a dup, but...

This patch caused a miscompilation of the restore_gp_regs() block in restore_sigcontext(). This was in a 32-bit kernel compiled with GCC CVS head.

restore_gp_regs() copies 64-bit user fields into 32-bit variables, and in this combination, the new __get_user_asm_ll32() clobbers too many registers. It says:

/*
* Get a long long 64 using 32 bit registers.
*/
#define __get_user_asm_ll32(val, addr) \

{									\
	__asm__ __volatile__(						\
	"1:	lw	%1, (%3)				\n"	\
	"2:	lw	%D1, 4(%3)				\n"	\
	"	move	%0, $0					\n"	\
	"3:	.section	.fixup,\"ax\"			\n"	\
	"4:	li	%0, %4					\n"	\
	"	move	%1, $0					\n"	\
	"	move	%D1, $0					\n"	\
	"	j	3b					\n"	\
	"	.previous					\n"	\
	"	.section	__ex_table,\"a\"		\n"	\
	"	" __UA_ADDR "	1b, 4b				\n"	\
	"	" __UA_ADDR "	2b, 4b				\n"	\
	"	.previous					\n"	\
	: "=r" (__gu_err), "=&r" (val)					\
	: "0" (0), "r" (addr), "i" (-EFAULT));				\
}

and this requires val (%1) to be a 64-bit value. In the case I saw, gcc was using $3 for the 32-bit val, and wasn't expecting $4 to be clobbered.

FWIW, the patch below fixes it for me.

Richard

include/asm-mips/uaccess.h +3 -1
index 91d813a..1208cae 100644
@@ -266,6 +266,7 @@ do {									\
  */
 #define __get_user_asm_ll32(val, addr)					\
 {									\
+        unsigned long long __gu_tmp;					\
 	__asm__ __volatile__(						\
 	"1:	lw	%1, (%3)				\n"	\
 	"2:	lw	%D1, 4(%3)				\n"	\

@@ -280,8 +281,9 @@ do {									\
 	"	" __UA_ADDR "	1b, 4b				\n"	\
 	"	" __UA_ADDR "	2b, 4b				\n"	\
 	"	.previous					\n"	\
-	: "=r" (__gu_err), "=&r" (val)					\
+	: "=r" (__gu_err), "=&r" (__gu_tmp)				\
 	: "0" (0), "r" (addr), "i" (-EFAULT));				\
+	(val) = __gu_tmp;						\
 }
 
 /*
author
#11 lore raw
Quoted 5 lines · Richard Sandiford
Quoted 2 lines · Ralf Baechle

I'd appreciate if somebody with gcc 4.0.1 could test this kernel patch below.

Sorry in advance if this is a dup, but...

No, not a dupe.

Quoted 3 lines

and this requires val (%1) to be a 64-bit value. In the case I saw, gcc was using $3 for the 32-bit val, and wasn't expecting $4 to be clobbered.

Thanks, makes perfect sense. I tried various other obscure things and your patch was holding up, so I just applied it.

Thanks,

Ralf

PS: If in the future you could include a Signed-off-by: line.

#12 lore raw
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line · Ralf Baechle

ralf> Thanks, makes perfect sense. I tried various other obscure ralf> things and your patch was holding up, so I just applied it.

Please add this cast to fix compiler/sparse warnings?

Signed-off-by Atsushi Nemoto <[email protected]>
include/asm-mips/uaccess.h +1 -1
index 7a553e9..252caba 100644
@@ -284,7 +284,7 @@ do {									\
 	"	.previous					\n"	\
 	: "=r" (__gu_err), "=&r" (__gu_tmp)				\
 	: "0" (0), "r" (addr), "i" (-EFAULT));				\
-	(val) = __gu_tmp;						\
+	(val) = (__typeof__(val)) __gu_tmp;				\
 }
 
 /*
#13 lore raw
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line · Atsushi Nemoto

anemo> Please add this cast to fix compiler/sparse warnings?

Sorry, please ignore this patch. It would be wrong.

It seems current get_user() incorrectly sign-extend an unsigned int value on 64bit kernel. I think this is because '(__typeof__(val))' cast in final assignment. I suppose the cast should be '(__typeof__(*(addr))'.

Signed-off-by Atsushi Nemoto <[email protected]>
include/asm-mips/uaccess.h +3 -3
index 7a553e9..b96f3e0 100644
@@ -233,7 +233,7 @@ do {									\
 #define __get_user_check(x,ptr,size)					\
 ({									\
 	long __gu_err = -EFAULT;					\
-	const void __user * __gu_ptr = (ptr);				\
+	const __typeof__(*(ptr)) __user * __gu_ptr = (ptr);		\
 									\
 	if (likely(access_ok(VERIFY_READ,  __gu_ptr, size)))		\
 		__get_user_common((x), size, __gu_ptr);			\

@@ -258,7 +258,7 @@ do {									\
 	: "=r" (__gu_err), "=r" (__gu_tmp)				\
 	: "0" (0), "o" (__m(addr)), "i" (-EFAULT));			\
 									\
-	(val) = (__typeof__(val)) __gu_tmp;				\
+	(val) = (__typeof__(*(addr))) __gu_tmp;				\
 }
 
 /*

@@ -284,7 +284,7 @@ do {									\
 	"	.previous					\n"	\
 	: "=r" (__gu_err), "=&r" (__gu_tmp)				\
 	: "0" (0), "r" (addr), "i" (-EFAULT));				\
-	(val) = __gu_tmp;						\
+	(val) = (__typeof__(*(addr))) __gu_tmp;				\
 }
 
 /*