linux-cifs.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* Fwd: [EXTERNAL] Re: ioctl.c:undefined reference to `__get_user_bad'
       [not found]   ` <MN2PR21MB15184963469FEC9B13433964E42D9@MN2PR21MB1518.namprd21.prod.outlook.com>
@ 2021-05-17 17:13     ` Steve French
  2021-05-17 21:04       ` Randy Dunlap
  0 siblings, 1 reply; 6+ messages in thread
From: Steve French @ 2021-05-17 17:13 UTC (permalink / raw)
  To: Randy Dunlap, CIFS, linux-fsdevel

> If you fix the issue, kindly add following tag as appropriate
> Reported-by: kernel test robot <lkp@intel.com>
>
> All errors (new ones prefixed by >>):
>
>    arm-linux-gnueabi-ld: fs/cifs/ioctl.o: in function `cifs_dump_full_key':
>>> ioctl.c:(.text+0x44): undefined reference to `__get_user_bad'
>

<snip>

># CONFIG_MMU is not set
>
>and arch/arm/include/asm/uaccess.h does not implement get_user(size 8 bytes)
> for the non-MMU case:

I see another place in fs/cifs/ioctl.c where we already had been doing
a get_user() into a u64 - any idea what you are supposed to do
instead?  Any example code where people have worked around this.

-- 
Thanks,

Steve

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: Fwd: [EXTERNAL] Re: ioctl.c:undefined reference to `__get_user_bad'
  2021-05-17 17:13     ` Fwd: [EXTERNAL] Re: ioctl.c:undefined reference to `__get_user_bad' Steve French
@ 2021-05-17 21:04       ` Randy Dunlap
  2021-05-17 21:06         ` Randy Dunlap
  2021-05-19  9:11         ` Aurélien Aptel
  0 siblings, 2 replies; 6+ messages in thread
From: Randy Dunlap @ 2021-05-17 21:04 UTC (permalink / raw)
  To: Steve French, CIFS, linux-fsdevel

On 5/17/21 10:13 AM, Steve French wrote:
>> If you fix the issue, kindly add following tag as appropriate
>> Reported-by: kernel test robot <lkp@intel.com>
>>
>> All errors (new ones prefixed by >>):
>>
>>    arm-linux-gnueabi-ld: fs/cifs/ioctl.o: in function `cifs_dump_full_key':
>>>> ioctl.c:(.text+0x44): undefined reference to `__get_user_bad'
>>
> 
> <snip>
> 
>> # CONFIG_MMU is not set
>>
>> and arch/arm/include/asm/uaccess.h does not implement get_user(size 8 bytes)
>> for the non-MMU case:
> 
> I see another place in fs/cifs/ioctl.c where we already had been doing
> a get_user() into a u64 - any idea what you are supposed to do
> instead?  Any example code where people have worked around this.

Hi Steve,

This change in cifs_dump_full_key() makes it build OK:

-       if (get_user(suid, (__u64 __user *)arg))
+       if (get_user(suid, (unsigned int __user *)arg))


That is what the other call uses:

		case FS_IOC_SETFLAGS:
			cifs_dbg(VFS, "cifs ioctl FS_IOC_SETFLAGS:\n");
			if (pSMBFile == NULL)
				break;
			tcon = tlink_tcon(pSMBFile->tlink);
			caps = le64_to_cpu(tcon->fsUnixInfo.Capability);

			if (get_user(ExtAttrBits, (int __user *)arg)) {
				rc = -EFAULT;
				break;
			}

However, my reading/understanding is that the one immediately above
is incorrect, as is the -/+ patch above it, since get_user() gets its
data size (1, 2, 4, 8) from the type of the pointer that is passed to it.
For 8 bytes (64 bits), 'int' is not sufficient, so IMO the get_user()
call that builds:
			if (get_user(ExtAttrBits, (int __user *)arg)) {
is a bug. It should be:
			if (get_user(ExtAttrBits, (__u64 __user *)arg)) {
and if I make that latter change in the source file, the build says:

arm-linux-gnueabi-ld: fs/cifs/ioctl.o: in function `cifs_dump_full_key':
ioctl.c:(.text+0x14): undefined reference to `__get_user_bad'
arm-linux-gnueabi-ld: fs/cifs/ioctl.o: in function `cifs_ioctl':
ioctl.c:(.text+0x1f2): undefined reference to `__get_user_bad'

so now both of them fail on the get_user() of 8 bytes.

Hope that clarifies things.  It tells me that arm no-MMU still
needs support for get_user() of size 8 bytes.

-- 
~Randy


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: Fwd: [EXTERNAL] Re: ioctl.c:undefined reference to `__get_user_bad'
  2021-05-17 21:04       ` Randy Dunlap
@ 2021-05-17 21:06         ` Randy Dunlap
  2021-05-18 10:03           ` Russell King (Oracle)
  2021-05-19  9:11         ` Aurélien Aptel
  1 sibling, 1 reply; 6+ messages in thread
From: Randy Dunlap @ 2021-05-17 21:06 UTC (permalink / raw)
  To: Steve French, CIFS, linux-fsdevel, LAK

[adding back linux-arm-kernel; what happened to it? ]


On 5/17/21 2:04 PM, Randy Dunlap wrote:
> On 5/17/21 10:13 AM, Steve French wrote:
>>> If you fix the issue, kindly add following tag as appropriate
>>> Reported-by: kernel test robot <lkp@intel.com>
>>>
>>> All errors (new ones prefixed by >>):
>>>
>>>    arm-linux-gnueabi-ld: fs/cifs/ioctl.o: in function `cifs_dump_full_key':
>>>>> ioctl.c:(.text+0x44): undefined reference to `__get_user_bad'
>>>
>>
>> <snip>
>>
>>> # CONFIG_MMU is not set
>>>
>>> and arch/arm/include/asm/uaccess.h does not implement get_user(size 8 bytes)
>>> for the non-MMU case:
>>
>> I see another place in fs/cifs/ioctl.c where we already had been doing
>> a get_user() into a u64 - any idea what you are supposed to do
>> instead?  Any example code where people have worked around this.
> 
> Hi Steve,
> 
> This change in cifs_dump_full_key() makes it build OK:
> 
> -       if (get_user(suid, (__u64 __user *)arg))
> +       if (get_user(suid, (unsigned int __user *)arg))
> 
> 
> That is what the other call uses:
> 
> 		case FS_IOC_SETFLAGS:
> 			cifs_dbg(VFS, "cifs ioctl FS_IOC_SETFLAGS:\n");
> 			if (pSMBFile == NULL)
> 				break;
> 			tcon = tlink_tcon(pSMBFile->tlink);
> 			caps = le64_to_cpu(tcon->fsUnixInfo.Capability);
> 
> 			if (get_user(ExtAttrBits, (int __user *)arg)) {
> 				rc = -EFAULT;
> 				break;
> 			}
> 
> However, my reading/understanding is that the one immediately above
> is incorrect, as is the -/+ patch above it, since get_user() gets its
> data size (1, 2, 4, 8) from the type of the pointer that is passed to it.
> For 8 bytes (64 bits), 'int' is not sufficient, so IMO the get_user()
> call that builds:
> 			if (get_user(ExtAttrBits, (int __user *)arg)) {
> is a bug. It should be:
> 			if (get_user(ExtAttrBits, (__u64 __user *)arg)) {
> and if I make that latter change in the source file, the build says:
> 
> arm-linux-gnueabi-ld: fs/cifs/ioctl.o: in function `cifs_dump_full_key':
> ioctl.c:(.text+0x14): undefined reference to `__get_user_bad'
> arm-linux-gnueabi-ld: fs/cifs/ioctl.o: in function `cifs_ioctl':
> ioctl.c:(.text+0x1f2): undefined reference to `__get_user_bad'
> 
> so now both of them fail on the get_user() of 8 bytes.
> 
> Hope that clarifies things.  It tells me that arm no-MMU still
> needs support for get_user() of size 8 bytes.
> 

-- 
~Randy


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: Fwd: [EXTERNAL] Re: ioctl.c:undefined reference to `__get_user_bad'
  2021-05-17 21:06         ` Randy Dunlap
@ 2021-05-18 10:03           ` Russell King (Oracle)
  2021-05-19  1:48             ` Randy Dunlap
  0 siblings, 1 reply; 6+ messages in thread
From: Russell King (Oracle) @ 2021-05-18 10:03 UTC (permalink / raw)
  To: Randy Dunlap; +Cc: Steve French, CIFS, linux-fsdevel, LAK

On Mon, May 17, 2021 at 02:06:33PM -0700, Randy Dunlap wrote:
> [adding back linux-arm-kernel; what happened to it? ]

Nothing. I'm not interested in trying to do major disgusting
contortions to make get_user() work for 8-byte values. If someone
else wants to put the effort in and come up with an elegant solution
that doesn't add warnings over the rest of the kernel, that's fine.

As far as I remember, everything in __get_user_err() relies on
__gu_val _not_ being 64-bit. If we use the same trick that we do
in __get_user_check():

	__inttype(x) __gu_val = (x);

then if get_user() is called with a 64-bit integer value and a
pointer-to-32-bit location to fetch from, we'd end up passing a
64-bit integer into the __get_user_asm() which could access the
wrong 32-bit half of the value in BE mode. Similar issue with
64-bit vs pointer-to-16-bit.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: Fwd: [EXTERNAL] Re: ioctl.c:undefined reference to `__get_user_bad'
  2021-05-18 10:03           ` Russell King (Oracle)
@ 2021-05-19  1:48             ` Randy Dunlap
  0 siblings, 0 replies; 6+ messages in thread
From: Randy Dunlap @ 2021-05-19  1:48 UTC (permalink / raw)
  To: Russell King (Oracle); +Cc: Steve French, CIFS, linux-fsdevel, LAK

On 5/18/21 3:03 AM, Russell King (Oracle) wrote:
> On Mon, May 17, 2021 at 02:06:33PM -0700, Randy Dunlap wrote:
>> [adding back linux-arm-kernel; what happened to it? ]

That was about what I thought was Steve F. dropping LAK on his response email.

> Nothing. I'm not interested in trying to do major disgusting
> contortions to make get_user() work for 8-byte values. If someone
> else wants to put the effort in and come up with an elegant solution
> that doesn't add warnings over the rest of the kernel, that's fine.
> 
> As far as I remember, everything in __get_user_err() relies on
> __gu_val _not_ being 64-bit. If we use the same trick that we do
> in __get_user_check():
> 
> 	__inttype(x) __gu_val = (x);
> 
> then if get_user() is called with a 64-bit integer value and a
> pointer-to-32-bit location to fetch from, we'd end up passing a
> 64-bit integer into the __get_user_asm() which could access the
> wrong 32-bit half of the value in BE mode. Similar issue with
> 64-bit vs pointer-to-16-bit.

Yes, trying to handle get_user() of size 8 bytes is quite messy.
I have a few versions and they are all ugly and cause build warnings.

So we are down to what bugzilla calls WONTFIX. I'm OK with that.

Thanks.

-- 
~Randy


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: Fwd: [EXTERNAL] Re: ioctl.c:undefined reference to `__get_user_bad'
  2021-05-17 21:04       ` Randy Dunlap
  2021-05-17 21:06         ` Randy Dunlap
@ 2021-05-19  9:11         ` Aurélien Aptel
  1 sibling, 0 replies; 6+ messages in thread
From: Aurélien Aptel @ 2021-05-19  9:11 UTC (permalink / raw)
  To: Randy Dunlap, Steve French, CIFS, linux-fsdevel


I am working on a rewrite of that CIFS ioctl like we discussed, and I'm
not using the get_user() helper so it should be ok.

Cheers,
-- 
Aurélien Aptel / SUSE Labs Samba Team
GPG: 1839 CB5F 9F5B FB9B AA97  8C99 03C8 A49B 521B D5D3
SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg, DE
GF: Felix Imendörffer, Mary Higgins, Sri Rasiah HRB 247165 (AG München)


^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2021-05-19  9:12 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
     [not found] <202105110829.MHq04tJz-lkp@intel.com>
     [not found] ` <a022694d-426a-0415-83de-4cc5cd9d1d38@infradead.org>
     [not found]   ` <MN2PR21MB15184963469FEC9B13433964E42D9@MN2PR21MB1518.namprd21.prod.outlook.com>
2021-05-17 17:13     ` Fwd: [EXTERNAL] Re: ioctl.c:undefined reference to `__get_user_bad' Steve French
2021-05-17 21:04       ` Randy Dunlap
2021-05-17 21:06         ` Randy Dunlap
2021-05-18 10:03           ` Russell King (Oracle)
2021-05-19  1:48             ` Randy Dunlap
2021-05-19  9:11         ` Aurélien Aptel

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).