[PATCH v3] arch/microblaze: add support for get_user() of size 8 bytes

[PATCH v3] arch/microblaze: add support for get_user() of size 8 bytes

[Randy Dunlap]

From: Randy Dunlap <rdunlap@infradead.org>

arch/microblaze/ is missing support for get_user() of size 8 bytes,
so add it by using __copy_from_user().

Fixes these build errors:
   drivers/infiniband/core/uverbs_main.o: In function `ib_uverbs_write':
   drivers/infiniband/core/.tmp_gl_uverbs_main.o:(.text+0x13a4): undefined reference to `__user_bad'
   drivers/android/binder.o: In function `binder_thread_write':
   drivers/android/.tmp_gl_binder.o:(.text+0xda6c): undefined reference to `__user_bad'
   drivers/android/.tmp_gl_binder.o:(.text+0xda98): undefined reference to `__user_bad'
   drivers/android/.tmp_gl_binder.o:(.text+0xdf10): undefined reference to `__user_bad'
   drivers/android/.tmp_gl_binder.o:(.text+0xe498): undefined reference to `__user_bad'
   drivers/android/binder.o:drivers/android/.tmp_gl_binder.o:(.text+0xea78): more undefined references to `__user_bad' follow

'make allmodconfig' now builds successfully for arch/microblaze/.

Fixes: 538722ca3b76 ("microblaze: fix get_user/put_user side-effects")
Reported-by: kbuild test robot <lkp@intel.com>
Signed-off-by: Randy Dunlap <rdunlap@infradead.org>
Cc: Al Viro <viro@zeniv.linux.org.uk>
Cc: Steven J. Magnani <steve@digidescorp.com>
Cc: Michal Simek <monstr@monstr.eu>
Cc: Jason Gunthorpe <jgg@mellanox.com>
Cc: Leon Romanovsky <leonro@mellanox.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Doug Ledford <dledford@redhat.com>
---
v3: fix return value in error case (comments from Linus).

What is a reasonable path for having this patch merged?
I have sent several emails to Micahl Simek but he seems to have
dropped active maintenance of arch/microblaze/.

 arch/microblaze/include/asm/uaccess.h |   10 ++++++++++
 1 file changed, 10 insertions(+)

--- lnx-53-rc6.orig/arch/microblaze/include/asm/uaccess.h
+++ lnx-53-rc6/arch/microblaze/include/asm/uaccess.h
@@ -186,6 +186,11 @@ extern long __user_bad(void);
 			__get_user_asm("lw", __gu_addr, __gu_val,	\
 				       __gu_err);			\
 			break;						\
+		case 8:							\
+			__gu_err = __copy_from_user(&__gu_val, __gu_addr, 8);\
+			if (__gu_err) /* bytes remaining */		\
+				__gu_err = -EFAULT;			\
+			break;						\
 		default:						\
 			__gu_err = __user_bad();			\
 			break;						\
@@ -212,6 +217,11 @@ extern long __user_bad(void);
 	case 4:								\
 		__get_user_asm("lw", (ptr), __gu_val, __gu_err);	\
 		break;							\
+	case 8:								\
+		__gu_err = __copy_from_user(&__gu_val, ptr, 8);		\
+		if (__gu_err) /* bytes remaining */			\
+			__gu_err = -EFAULT;				\
+		break;							\
 	default:							\
 		/* __gu_val = 0; __gu_err = -EINVAL;*/ __gu_err = __user_bad();\
 	}								\

Re: [PATCH v3] arch/microblaze: add support for get_user() of size 8 bytes

[Linus Torvalds]

On Sun, Sep 1, 2019 at 7:55 AM Randy Dunlap <rdunlap@infradead.org> wrote:
>
> What is a reasonable path for having this patch merged?
> I have sent several emails to Micahl Simek but he seems to have
> dropped active maintenance of arch/microblaze/.

Yeah, I haven't gotten a pull request from him since March, and that
was a trivial fixup.

I guess I'll apply it. I'm not sure why you _care_ about microblaze, but ...

                 Linus

Re: [PATCH v3] arch/microblaze: add support for get_user() of size 8 bytes

[Linus Torvalds]

[-- Attachment #1: Type: text/plain, Size: 415 bytes --]

On Sun, Sep 1, 2019 at 10:07 AM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> I guess I'll apply it. I'm not sure why you _care_ about microblaze, but ...

Ugh. As I was going to apply it, my code cleanliness conscience struck.

I can't deal with that unnecessary duplication of code. Does something
like the attached patch work instead?

Totally untested, but looks much cleaner.

               Linus

[-- Attachment #2: patch.diff --]
[-- Type: text/x-patch, Size: 2153 bytes --]

 arch/microblaze/include/asm/uaccess.h | 42 ++++++++---------------------------
 1 file changed, 9 insertions(+), 33 deletions(-)

diff --git a/arch/microblaze/include/asm/uaccess.h b/arch/microblaze/include/asm/uaccess.h
index bff2a71c828a..88bde06a41a3 100644
--- a/arch/microblaze/include/asm/uaccess.h
+++ b/arch/microblaze/include/asm/uaccess.h
@@ -163,44 +163,15 @@ extern long __user_bad(void);
  * Returns zero on success, or -EFAULT on error.
  * On error, the variable @x is set to zero.
  */
-#define get_user(x, ptr)						\
-	__get_user_check((x), (ptr), sizeof(*(ptr)))
-
-#define __get_user_check(x, ptr, size)					\
-({									\
-	unsigned long __gu_val = 0;					\
-	const typeof(*(ptr)) __user *__gu_addr = (ptr);			\
-	int __gu_err = 0;						\
-									\
-	if (access_ok(__gu_addr, size)) {			\
-		switch (size) {						\
-		case 1:							\
-			__get_user_asm("lbu", __gu_addr, __gu_val,	\
-				       __gu_err);			\
-			break;						\
-		case 2:							\
-			__get_user_asm("lhu", __gu_addr, __gu_val,	\
-				       __gu_err);			\
-			break;						\
-		case 4:							\
-			__get_user_asm("lw", __gu_addr, __gu_val,	\
-				       __gu_err);			\
-			break;						\
-		default:						\
-			__gu_err = __user_bad();			\
-			break;						\
-		}							\
-	} else {							\
-		__gu_err = -EFAULT;					\
-	}								\
-	x = (__force typeof(*(ptr)))__gu_val;				\
-	__gu_err;							\
+#define get_user(x, ptr) ({				\
+	typeof(ptr) __gu_ptr = (ptr);			\
+	access_ok(__gu_ptr, sizeof(*__gu_ptr)) ?	\
+		__get_user(x, __gu_ptr) : -EFAULT;	\
 })
 
 #define __get_user(x, ptr)						\
 ({									\
 	unsigned long __gu_val = 0;					\
-	/*unsigned long __gu_ptr = (unsigned long)(ptr);*/		\
 	long __gu_err;							\
 	switch (sizeof(*(ptr))) {					\
 	case 1:								\
@@ -212,6 +183,11 @@ extern long __user_bad(void);
 	case 4:								\
 		__get_user_asm("lw", (ptr), __gu_val, __gu_err);	\
 		break;							\
+	case 8:								\
+		__gu_err = __copy_from_user(&__gu_val, ptr, 8);		\
+		if (__gu_err)						\
+			__gu_err = -EFAULT;				\
+		break;							\
 	default:							\
 		/* __gu_val = 0; __gu_err = -EINVAL;*/ __gu_err = __user_bad();\
 	}								\

Re: [PATCH v3] arch/microblaze: add support for get_user() of size 8 bytes

[Michal Simek]

[-- Attachment #1.1: Type: text/plain, Size: 1100 bytes --]

Hi,

On 01. 09. 19 19:07, Linus Torvalds wrote:
> On Sun, Sep 1, 2019 at 7:55 AM Randy Dunlap <rdunlap@infradead.org> wrote:
>>
>> What is a reasonable path for having this patch merged?
>> I have sent several emails to Micahl Simek but he seems to have
>> dropped active maintenance of arch/microblaze/.
> 
> Yeah, I haven't gotten a pull request from him since March, and that
> was a trivial fixup.
> 
> I guess I'll apply it. I'm not sure why you _care_ about microblaze, but ...

I am still around. The reason why I didn't send any pull request was
simply there were no patches. For 5.4 there will be some patches.

Randy: I found one email which was sent July and this thread when I had
vacation. I will take a look at both tmr. If there is something else
please resend.

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



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

Re: [PATCH v3] arch/microblaze: add support for get_user() of size 8 bytes

[Randy Dunlap]

On 9/1/19 10:31 AM, Linus Torvalds wrote:
> On Sun, Sep 1, 2019 at 10:07 AM Linus Torvalds
> <torvalds@linux-foundation.org> wrote:
>>
>> I guess I'll apply it. I'm not sure why you _care_ about microblaze, but ...

It was just a response to the 0day build bot reporting build errors.


> Ugh. As I was going to apply it, my code cleanliness conscience struck.
> 
> I can't deal with that unnecessary duplication of code. Does something
> like the attached patch work instead?
> 
> Totally untested, but looks much cleaner.

Hm, I'm getting one (confusing) build error, in block/scsi_ioctl.c:

  CC      block/scsi_ioctl.o
In file included from ../include/linux/uaccess.h:11,
                 from ../include/linux/highmem.h:9,
                 from ../include/linux/pagemap.h:11,
                 from ../include/linux/blkdev.h:16,
                 from ../block/scsi_ioctl.c:9:
../block/scsi_ioctl.c: In function 'sg_scsi_ioctl':
../arch/microblaze/include/asm/uaccess.h:167:25: error: invalid initializer
  typeof(ptr) __gu_ptr = (ptr);   \
                         ^
../block/scsi_ioctl.c:426:6: note: in expansion of macro 'get_user'
  if (get_user(opcode, sic->data))
      ^~~~~~~~


-- 
~Randy

Re: [PATCH v3] arch/microblaze: add support for get_user() of size 8 bytes

[Randy Dunlap]

On 9/1/19 10:33 AM, Michal Simek wrote:
> Hi,
> 
> On 01. 09. 19 19:07, Linus Torvalds wrote:
>> On Sun, Sep 1, 2019 at 7:55 AM Randy Dunlap <rdunlap@infradead.org> wrote:
>>>
>>> What is a reasonable path for having this patch merged?
>>> I have sent several emails to Micahl Simek but he seems to have
>>> dropped active maintenance of arch/microblaze/.
>>
>> Yeah, I haven't gotten a pull request from him since March, and that
>> was a trivial fixup.
>>
>> I guess I'll apply it. I'm not sure why you _care_ about microblaze, but ...
> 
> I am still around. The reason why I didn't send any pull request was
> simply there were no patches. For 5.4 there will be some patches.
> 
> Randy: I found one email which was sent July and this thread when I had
> vacation. I will take a look at both tmr. If there is something else
> please resend.

Good to hear that you are still around.
I'll let you know if I find other lost patches.

thanks.
-- 
~Randy

Re: [PATCH v3] arch/microblaze: add support for get_user() of size 8 bytes

[Randy Dunlap]

On 9/1/19 12:10 PM, Randy Dunlap wrote:
> On 9/1/19 10:31 AM, Linus Torvalds wrote:
>> On Sun, Sep 1, 2019 at 10:07 AM Linus Torvalds
>> <torvalds@linux-foundation.org> wrote:
>>>
>>> I guess I'll apply it. I'm not sure why you _care_ about microblaze, but ...
> 
> It was just a response to the 0day build bot reporting build errors.
> 
> 
>> Ugh. As I was going to apply it, my code cleanliness conscience struck.
>>
>> I can't deal with that unnecessary duplication of code. Does something
>> like the attached patch work instead?
>>
>> Totally untested, but looks much cleaner.
> 
> Hm, I'm getting one (confusing) build error, in block/scsi_ioctl.c:
> 
>   CC      block/scsi_ioctl.o
> In file included from ../include/linux/uaccess.h:11,
>                  from ../include/linux/highmem.h:9,
>                  from ../include/linux/pagemap.h:11,
>                  from ../include/linux/blkdev.h:16,
>                  from ../block/scsi_ioctl.c:9:
> ../block/scsi_ioctl.c: In function 'sg_scsi_ioctl':
> ../arch/microblaze/include/asm/uaccess.h:167:25: error: invalid initializer
>   typeof(ptr) __gu_ptr = (ptr);   \
>                          ^
> ../block/scsi_ioctl.c:426:6: note: in expansion of macro 'get_user'
>   if (get_user(opcode, sic->data))
>       ^~~~~~~~

	if (get_user(opcode, sic->data))
		return -EFAULT;

where sic->data is unsigned char data[0] here:

typedef struct scsi_ioctl_command {
	unsigned int inlen;
	unsigned int outlen;
	unsigned char data[0];
} Scsi_Ioctl_Command;

On x86_64 this builds as a call to get_user_1().
(cannot do objdump on arch/microblaze/, unknown arch/machine)

I guess we need a way to coerce that to call get_user_1(),
such as a typecast.  This _seems_ to work (i.e., call get_user_1()):

	if (get_user(opcode, (unsigned char *)(sic->data)))
		return -EFAULT;

??

-- 
~Randy

Re: [PATCH v3] arch/microblaze: add support for get_user() of size 8 bytes

[Linus Torvalds]

[-- Attachment #1: Type: text/plain, Size: 299 bytes --]

On Sun, Sep 1, 2019 at 7:10 PM Randy Dunlap <rdunlap@infradead.org> wrote:
>
> I guess we need a way to coerce that to call get_user_1(),
> such as a typecast.  This _seems_ to work (i.e., call get_user_1()):

No, I oversimplified.

Try this slightly modified patch instead.

                 Linus

[-- Attachment #2: patch.diff --]
[-- Type: text/x-patch, Size: 2168 bytes --]

 arch/microblaze/include/asm/uaccess.h | 42 ++++++++---------------------------
 1 file changed, 9 insertions(+), 33 deletions(-)

diff --git a/arch/microblaze/include/asm/uaccess.h b/arch/microblaze/include/asm/uaccess.h
index bff2a71c828a..a1f206b90753 100644
--- a/arch/microblaze/include/asm/uaccess.h
+++ b/arch/microblaze/include/asm/uaccess.h
@@ -163,44 +163,15 @@ extern long __user_bad(void);
  * Returns zero on success, or -EFAULT on error.
  * On error, the variable @x is set to zero.
  */
-#define get_user(x, ptr)						\
-	__get_user_check((x), (ptr), sizeof(*(ptr)))
-
-#define __get_user_check(x, ptr, size)					\
-({									\
-	unsigned long __gu_val = 0;					\
-	const typeof(*(ptr)) __user *__gu_addr = (ptr);			\
-	int __gu_err = 0;						\
-									\
-	if (access_ok(__gu_addr, size)) {			\
-		switch (size) {						\
-		case 1:							\
-			__get_user_asm("lbu", __gu_addr, __gu_val,	\
-				       __gu_err);			\
-			break;						\
-		case 2:							\
-			__get_user_asm("lhu", __gu_addr, __gu_val,	\
-				       __gu_err);			\
-			break;						\
-		case 4:							\
-			__get_user_asm("lw", __gu_addr, __gu_val,	\
-				       __gu_err);			\
-			break;						\
-		default:						\
-			__gu_err = __user_bad();			\
-			break;						\
-		}							\
-	} else {							\
-		__gu_err = -EFAULT;					\
-	}								\
-	x = (__force typeof(*(ptr)))__gu_val;				\
-	__gu_err;							\
+#define get_user(x, ptr) ({				\
+	const typeof(*(ptr)) __user *__gu_ptr = (ptr);	\
+	access_ok(__gu_ptr, sizeof(*__gu_ptr)) ?	\
+		__get_user(x, __gu_ptr) : -EFAULT;	\
 })
 
 #define __get_user(x, ptr)						\
 ({									\
 	unsigned long __gu_val = 0;					\
-	/*unsigned long __gu_ptr = (unsigned long)(ptr);*/		\
 	long __gu_err;							\
 	switch (sizeof(*(ptr))) {					\
 	case 1:								\
@@ -212,6 +183,11 @@ extern long __user_bad(void);
 	case 4:								\
 		__get_user_asm("lw", (ptr), __gu_val, __gu_err);	\
 		break;							\
+	case 8:								\
+		__gu_err = __copy_from_user(&__gu_val, ptr, 8);		\
+		if (__gu_err)						\
+			__gu_err = -EFAULT;				\
+		break;							\
 	default:							\
 		/* __gu_val = 0; __gu_err = -EINVAL;*/ __gu_err = __user_bad();\
 	}								\

RE: [PATCH v3] arch/microblaze: add support for get_user() of size 8 bytes

[David Laight]

From: Randy Dunlap
> Sent: 01 September 2019 15:56
> arch/microblaze/ is missing support for get_user() of size 8 bytes,
> so add it by using __copy_from_user().

Ugg....

Use get_user() for 4 bytes twice and combine the returned values.

	David

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

Re: [PATCH v3] arch/microblaze: add support for get_user() of size 8 bytes

[Michal Simek]

[-- Attachment #1.1: Type: text/plain, Size: 883 bytes --]

On 02. 09. 19 6:58, Linus Torvalds wrote:
> On Sun, Sep 1, 2019 at 7:10 PM Randy Dunlap <rdunlap@infradead.org> wrote:
>>
>> I guess we need a way to coerce that to call get_user_1(),
>> such as a typecast.  This _seems_ to work (i.e., call get_user_1()):
> 
> No, I oversimplified.
> 
> Try this slightly modified patch instead.

This one looks good. I have also tested it on HW. I will run some LTP
tests to see if there is any new error. If there is better testsuite to
validate this please let me know.

Randy/Linus: Are you going create regular patch from this?

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



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

Re: [PATCH v3] arch/microblaze: add support for get_user() of size 8 bytes

[Randy Dunlap]

On 9/1/19 9:58 PM, Linus Torvalds wrote:
> On Sun, Sep 1, 2019 at 7:10 PM Randy Dunlap <rdunlap@infradead.org> wrote:
>>
>> I guess we need a way to coerce that to call get_user_1(),
>> such as a typecast.  This _seems_ to work (i.e., call get_user_1()):
> 
> No, I oversimplified.
> 
> Try this slightly modified patch instead.
> 

Yes, that builds cleanly.

thanks.
-- 
~Randy

Re: [PATCH v3] arch/microblaze: add support for get_user() of size 8 bytes

[Linus Torvalds]

On Mon, Sep 2, 2019 at 6:17 AM Michal Simek <monstr@monstr.eu> wrote:
>
> Randy/Linus: Are you going create regular patch from this?

Since I can't even test it, and Randy did most of the work (and that
last patch worked for him too), I'd suggest he just send it in as his.

You can add my acked-by or sign-of depending on how you want to do it
(but I really don't need authorship credit, it might as well go to
Randy).

              Linus

Re: [PATCH v3] arch/microblaze: add support for get_user() of size 8 bytes

[Michal Simek]

[-- Attachment #1.1: Type: text/plain, Size: 565 bytes --]

Hi Randy,

On 02. 09. 19 18:52, Linus Torvalds wrote:
> On Mon, Sep 2, 2019 at 6:17 AM Michal Simek <monstr@monstr.eu> wrote:
>>
>> Randy/Linus: Are you going create regular patch from this?
> 
> Since I can't even test it, and Randy did most of the work (and that
> last patch worked for him too), I'd suggest he just send it in as his.
> 
> You can add my acked-by or sign-of depending on how you want to do it
> (but I really don't need authorship credit, it might as well go to
> Randy).

Can you please send v4 on this one?

Thanks,
Michal


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 195 bytes --]