Fixes for uaccess.h with gcc >= 4.0.1

Fixes for uaccess.h with gcc >= 4.0.1

[Ralf Baechle]

On Wed, Jan 18, 2006 at 01:37:59PM +0000, Martin Michlmayr wrote:

> * Ralf Baechle <ralf@linux-mips.org> [2006-01-18 13:05]:
> > > 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
> > 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(-)

Index: linux-mips/include/asm-mips/uaccess.h
===================================================================
--- linux-mips.orig/include/asm-mips/uaccess.h
+++ 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)					\
 ({									\

Re: Fixes for uaccess.h with gcc >= 4.0.1

[P. Christeas]

On Monday 23 January 2006 5:05 pm, Ralf Baechle wrote:

> 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..

Re: Fixes for uaccess.h with gcc >= 4.0.1

[P. Christeas]

On Monday 23 January 2006 5:05 pm, Ralf Baechle wrote:

> 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..

Re: Fixes for uaccess.h with gcc >= 4.0.1

[Ralf Baechle]

On Mon, Jan 23, 2006 at 05:18:38PM +0200, P. Christeas wrote:

> On Monday 23 January 2006 5:05 pm, Ralf Baechle wrote:
> 
> > 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.

> 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

Re: Fixes for uaccess.h with gcc >= 4.0.1

[Martin Michlmayr]

* P. Christeas <p_christ@hol.gr> [2006-01-23 17:18]:
> > I'd appreciate if somebody with gcc 4.0.1 could test this kernel patch
> > below.
> Is that for 2.4? 
> 2.6 doesn't seem to have that problem..

2.6.  You get the problem when you build a 64-bit kernel with the
32-bit COMPAT stuff turned on.  See
http://www.linux-mips.org/archives/linux-mips/2006-01/msg00199.html
-- 
Martin Michlmayr
http://www.cyrius.com/

Re: Fixes for uaccess.h with gcc >= 4.0.1

[P. Christeas]

On Monday 23 January 2006 5:37 pm, Ralf Baechle wrote:
> On Mon, Jan 23, 2006 at 05:18:38PM +0200, P. Christeas wrote:
> > On Monday 23 January 2006 5:05 pm, Ralf Baechle wrote:
> > > 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.
>
> > 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

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?

Re: Fixes for uaccess.h with gcc >= 4.0.1

[Rojhalat Ibrahim]

Ralf Baechle wrote:
> 
> 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

Re: Fixes for uaccess.h with gcc >= 4.0.1

[Thiemo Seufer]

On Mon, Jan 23, 2006 at 05:50:53PM +0200, P. Christeas wrote:
> On Monday 23 January 2006 5:37 pm, Ralf Baechle wrote:
> > On Mon, Jan 23, 2006 at 05:18:38PM +0200, P. Christeas wrote:
> > > On Monday 23 January 2006 5:05 pm, Ralf Baechle wrote:
> > > > 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.
> >
> > > 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
> 
> 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.

> 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

Re: Fixes for uaccess.h with gcc >= 4.0.1

[Martin Michlmayr]

* Ralf Baechle <ralf@linux-mips.org> [2006-01-23 15:05]:
> 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

-- 
Martin Michlmayr
http://www.cyrius.com/

Re: Fixes for uaccess.h with gcc >= 4.0.1

[Ralf Baechle]

On Mon, Jan 23, 2006 at 05:04:06PM +0100, Rojhalat Ibrahim wrote:

> > 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

Re: Fixes for uaccess.h with gcc >= 4.0.1

[Stuart Anderson]

On Mon, 23 Jan 2006, Ralf Baechle wrote:

> On Mon, Jan 23, 2006 at 05:04:06PM +0100, Rojhalat Ibrahim wrote:
>
>>> 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                               anderson@netsweng.com
Network & Software Engineering                   http://www.netsweng.com/
1024D/37A79149:                                  0791 D3B8 9A4C 2CDC A31F
                                                  BD03 0A62 E534 37A7 9149

Re: Fixes for uaccess.h with gcc >= 4.0.1

[Richard Sandiford]

Ralf Baechle <ralf@linux-mips.org> writes:
> 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


diff --git a/include/asm-mips/uaccess.h b/include/asm-mips/uaccess.h
index 91d813a..1208cae 100644
--- a/include/asm-mips/uaccess.h
+++ b/include/asm-mips/uaccess.h
@@ -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;						\
 }
 
 /*

Re: Fixes for uaccess.h with gcc >= 4.0.1

[Ralf Baechle]

On Tue, Feb 07, 2006 at 08:21:11PM +0000, Richard Sandiford wrote:

> Ralf Baechle <ralf@linux-mips.org> writes:
> > 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.

> 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.

Re: Fixes for uaccess.h with gcc >= 4.0.1

[Atsushi Nemoto]

>>>>> On Fri, 10 Feb 2006 01:34:40 +0000, Ralf Baechle <ralf@linux-mips.org> said:

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 <anemo@mba.ocn.ne.jp>

diff --git a/include/asm-mips/uaccess.h b/include/asm-mips/uaccess.h
index 7a553e9..252caba 100644
--- a/include/asm-mips/uaccess.h
+++ b/include/asm-mips/uaccess.h
@@ -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;				\
 }
 
 /*

Re: Fixes for uaccess.h with gcc >= 4.0.1

[Atsushi Nemoto]

>>>>> On Sat, 11 Feb 2006 22:44:20 +0900 (JST), Atsushi Nemoto <anemo@mba.ocn.ne.jp> said:
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 <anemo@mba.ocn.ne.jp>

diff --git a/include/asm-mips/uaccess.h b/include/asm-mips/uaccess.h
index 7a553e9..b96f3e0 100644
--- a/include/asm-mips/uaccess.h
+++ b/include/asm-mips/uaccess.h
@@ -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;				\
 }
 
 /*