2019-09-01 15:48:30

by Randy Dunlap

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

From: Randy Dunlap <[email protected]>

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 <[email protected]>
Signed-off-by: Randy Dunlap <[email protected]>
Cc: Al Viro <[email protected]>
Cc: Steven J. Magnani <[email protected]>
Cc: Michal Simek <[email protected]>
Cc: Jason Gunthorpe <[email protected]>
Cc: Leon Romanovsky <[email protected]>
Cc: Andrew Morton <[email protected]>
Cc: Doug Ledford <[email protected]>
---
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();\
} \



2019-09-01 17:09:45

by Linus Torvalds

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

On Sun, Sep 1, 2019 at 7:55 AM Randy Dunlap <[email protected]> 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

2019-09-01 17:38:34

by Linus Torvalds

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

On Sun, Sep 1, 2019 at 10:07 AM Linus Torvalds
<[email protected]> 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


Attachments:
patch.diff (2.10 kB)

2019-09-01 17:41:23

by Michal Simek

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

Hi,

On 01. 09. 19 19:07, Linus Torvalds wrote:
> On Sun, Sep 1, 2019 at 7:55 AM Randy Dunlap <[email protected]> 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: http://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



Attachments:
signature.asc (201.00 B)
OpenPGP digital signature

2019-09-01 19:14:29

by Randy Dunlap

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

On 9/1/19 10:31 AM, Linus Torvalds wrote:
> On Sun, Sep 1, 2019 at 10:07 AM Linus Torvalds
> <[email protected]> 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

2019-09-01 19:29:40

by Randy Dunlap

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

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 <[email protected]> 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

2019-09-02 02:11:23

by Randy Dunlap

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

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
>> <[email protected]> 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

2019-09-02 05:54:02

by Linus Torvalds

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

On Sun, Sep 1, 2019 at 7:10 PM Randy Dunlap <[email protected]> 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


Attachments:
patch.diff (2.12 kB)

2019-09-02 10:14:01

by David Laight

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

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)

2019-09-02 13:20:15

by Michal Simek

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

On 02. 09. 19 6:58, Linus Torvalds wrote:
> On Sun, Sep 1, 2019 at 7:10 PM Randy Dunlap <[email protected]> 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: http://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



Attachments:
signature.asc (201.00 B)
OpenPGP digital signature

2019-09-02 14:09:10

by Randy Dunlap

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

On 9/1/19 9:58 PM, Linus Torvalds wrote:
> On Sun, Sep 1, 2019 at 7:10 PM Randy Dunlap <[email protected]> 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

2019-09-02 16:54:30

by Linus Torvalds

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

On Mon, Sep 2, 2019 at 6:17 AM Michal Simek <[email protected]> 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

2019-09-13 06:26:11

by Michal Simek

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

Hi Randy,

On 02. 09. 19 18:52, Linus Torvalds wrote:
> On Mon, Sep 2, 2019 at 6:17 AM Michal Simek <[email protected]> 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


Attachments:
signature.asc (201.00 B)
OpenPGP digital signature