Not all architectures support get_user() with a 64-bit argument:
ERROR: "__get_user_bad" [fs/f2fs/f2fs.ko] undefined!
Use copy_from_user() here, this will always work.
Fixes: d2ae7494d043 ("f2fs: ioctl for removing a range from F2FS")
Signed-off-by: Arnd Bergmann <[email protected]>
---
fs/f2fs/file.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
index 998affe31419..465853029b8e 100644
--- a/fs/f2fs/file.c
+++ b/fs/f2fs/file.c
@@ -3066,7 +3066,8 @@ static int f2fs_ioc_resize_fs(struct file *filp, unsigned long arg)
if (f2fs_readonly(sbi->sb))
return -EROFS;
- if (get_user(block_count, (__u64 __user *)arg))
+ if (copy_from_user(&block_count, (void __user *)arg,
+ sizeof(block_count)))
return -EFAULT;
ret = f2fs_resize_fs(sbi, block_count);
--
2.20.0
On Fri, Jun 28, 2019 at 12:39:52PM +0200, Arnd Bergmann wrote:
> Not all architectures support get_user() with a 64-bit argument:
Which architectures are still missing? I think we finally need to
get everyone in line instead of repeatedly working around the lack
of minor arch support.
On Fri, Jun 28, 2019 at 2:44 PM Christoph Hellwig <[email protected]> wrote:
>
> On Fri, Jun 28, 2019 at 12:39:52PM +0200, Arnd Bergmann wrote:
> > Not all architectures support get_user() with a 64-bit argument:
>
> Which architectures are still missing? I think we finally need to
> get everyone in line instead of repeatedly working around the lack
> of minor arch support.
I came across this on arm-nommu (which disables
CONFIG_CPU_SPECTRE) during randconfig testing.
I don't see an easy way to add this in there, short of rewriting the
whole __get_user_err() function. Any suggestions?
Arnd
On Fri, Jun 28, 2019 at 03:09:47PM +0200, Arnd Bergmann wrote:
> I came across this on arm-nommu (which disables
> CONFIG_CPU_SPECTRE) during randconfig testing.
>
> I don't see an easy way to add this in there, short of rewriting the
> whole __get_user_err() function. Any suggestions?
Can't we just fall back to using copy_from_user with a little wrapper
that switches based on sizeof()?
On Fri, Jun 28, 2019 at 3:17 PM Christoph Hellwig <[email protected]> wrote:
>
> On Fri, Jun 28, 2019 at 03:09:47PM +0200, Arnd Bergmann wrote:
> > I came across this on arm-nommu (which disables
> > CONFIG_CPU_SPECTRE) during randconfig testing.
> >
> > I don't see an easy way to add this in there, short of rewriting the
> > whole __get_user_err() function. Any suggestions?
>
> Can't we just fall back to using copy_from_user with a little wrapper
> that switches based on sizeof()?
I came up with something now. It's not pretty, but seems to satisfy the
compiler. Not a proper patch yet, but let me know if you find a bug.
This might contain a double uaccess_save_and_enable/uaccess_restore,
not sure how much we care about that.
Arnd
index 7e0d2727c6b5..c21cdecadf26 100644
--- a/arch/arm/include/asm/uaccess.h
+++ b/arch/arm/include/asm/uaccess.h
@@ -307,6 +307,7 @@ static inline void set_fs(mm_segment_t fs)
do { \
unsigned long __gu_addr = (unsigned long)(ptr); \
unsigned long __gu_val; \
+ unsigned long long __gu_val8; \
unsigned int __ua_flags; \
__chk_user_ptr(ptr); \
might_fault(); \
@@ -315,10 +316,13 @@ do {
\
case 1: __get_user_asm_byte(__gu_val, __gu_addr, err); break; \
case 2: __get_user_asm_half(__gu_val, __gu_addr, err); break; \
case 4: __get_user_asm_word(__gu_val, __gu_addr, err); break; \
+ case 8: __get_user_asm_dword(__gu_val8, __gu_addr, err);break; \
default: (__gu_val) = __get_user_bad(); \
} \
uaccess_restore(__ua_flags); \
- (x) = (__typeof__(*(ptr)))__gu_val; \
+ (x) = __builtin_choose_expr(sizeof(*(ptr)) == 8, \
+ (__typeof__(*(ptr)))__gu_val8, \
+ (__typeof__(*(ptr)))__gu_val); \
} while (0)
#define __get_user_asm(x, addr, err, instr) \
@@ -373,6 +377,8 @@ do {
\
__get_user_asm(x, addr, err, ldr)
#endif
+#define __get_user_asm_dword(x, addr, err) \
+ do { err = raw_copy_from_user(&x, (void __user *)addr, 8) ?
-EFAULT : 0; } while (0)
#define __put_user_switch(x, ptr, __err, __fn) \
do { \
Hi Arnd,
If you don't mind, can I integrate this into the original patch in the queue?
Thanks,
On 06/28, Arnd Bergmann wrote:
> Not all architectures support get_user() with a 64-bit argument:
>
> ERROR: "__get_user_bad" [fs/f2fs/f2fs.ko] undefined!
>
> Use copy_from_user() here, this will always work.
>
> Fixes: d2ae7494d043 ("f2fs: ioctl for removing a range from F2FS")
> Signed-off-by: Arnd Bergmann <[email protected]>
> ---
> fs/f2fs/file.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
> index 998affe31419..465853029b8e 100644
> --- a/fs/f2fs/file.c
> +++ b/fs/f2fs/file.c
> @@ -3066,7 +3066,8 @@ static int f2fs_ioc_resize_fs(struct file *filp, unsigned long arg)
> if (f2fs_readonly(sbi->sb))
> return -EROFS;
>
> - if (get_user(block_count, (__u64 __user *)arg))
> + if (copy_from_user(&block_count, (void __user *)arg,
> + sizeof(block_count)))
> return -EFAULT;
>
> ret = f2fs_resize_fs(sbi, block_count);
> --
> 2.20.0
On Fri, Jun 28, 2019 at 04:46:14PM +0200, Arnd Bergmann wrote:
> On Fri, Jun 28, 2019 at 3:17 PM Christoph Hellwig <[email protected]> wrote:
> >
> > On Fri, Jun 28, 2019 at 03:09:47PM +0200, Arnd Bergmann wrote:
> > > I came across this on arm-nommu (which disables
> > > CONFIG_CPU_SPECTRE) during randconfig testing.
> > >
> > > I don't see an easy way to add this in there, short of rewriting the
> > > whole __get_user_err() function. Any suggestions?
> >
> > Can't we just fall back to using copy_from_user with a little wrapper
> > that switches based on sizeof()?
>
> I came up with something now. It's not pretty, but seems to satisfy the
> compiler. Not a proper patch yet, but let me know if you find a bug.
Have you checked what the behaviour is when "ptr" is a pointer to a
pointer? I think you'll end up with a compiler warning for every
case, complaining about casting an unsigned long long to a pointer.
>
> This might contain a double uaccess_save_and_enable/uaccess_restore,
> not sure how much we care about that.
>
> Arnd
>
> index 7e0d2727c6b5..c21cdecadf26 100644
> --- a/arch/arm/include/asm/uaccess.h
> +++ b/arch/arm/include/asm/uaccess.h
> @@ -307,6 +307,7 @@ static inline void set_fs(mm_segment_t fs)
> do { \
> unsigned long __gu_addr = (unsigned long)(ptr); \
> unsigned long __gu_val; \
> + unsigned long long __gu_val8; \
> unsigned int __ua_flags; \
> __chk_user_ptr(ptr); \
> might_fault(); \
> @@ -315,10 +316,13 @@ do {
> \
> case 1: __get_user_asm_byte(__gu_val, __gu_addr, err); break; \
> case 2: __get_user_asm_half(__gu_val, __gu_addr, err); break; \
> case 4: __get_user_asm_word(__gu_val, __gu_addr, err); break; \
> + case 8: __get_user_asm_dword(__gu_val8, __gu_addr, err);break; \
> default: (__gu_val) = __get_user_bad(); \
> } \
> uaccess_restore(__ua_flags); \
> - (x) = (__typeof__(*(ptr)))__gu_val; \
> + (x) = __builtin_choose_expr(sizeof(*(ptr)) == 8, \
> + (__typeof__(*(ptr)))__gu_val8, \
> + (__typeof__(*(ptr)))__gu_val); \
> } while (0)
>
> #define __get_user_asm(x, addr, err, instr) \
> @@ -373,6 +377,8 @@ do {
> \
> __get_user_asm(x, addr, err, ldr)
> #endif
>
> +#define __get_user_asm_dword(x, addr, err) \
> + do { err = raw_copy_from_user(&x, (void __user *)addr, 8) ?
> -EFAULT : 0; } while (0)
>
> #define __put_user_switch(x, ptr, __err, __fn) \
> do { \
>
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up
On 6/28/19 5:44 AM, Christoph Hellwig wrote:
> On Fri, Jun 28, 2019 at 12:39:52PM +0200, Arnd Bergmann wrote:
>> Not all architectures support get_user() with a 64-bit argument:
>
> Which architectures are still missing? I think we finally need to
> get everyone in line instead of repeatedly working around the lack
> of minor arch support.
>
arch/microblaze/include/asm/uaccess.c does not support get_user()
with a size of 8.
--
~Randy
On Fri, Jun 28, 2019 at 5:59 PM Jaegeuk Kim <[email protected]> wrote:
>
> Hi Arnd,
>
> If you don't mind, can I integrate this into the original patch in the queue?
Yes, I think that would be good anyway, it may take a little longer to fix all
the architectures.
Arnd
On Fri, Jun 28, 2019 at 7:58 PM Russell King - ARM Linux admin
<[email protected]> wrote:
>
> On Fri, Jun 28, 2019 at 04:46:14PM +0200, Arnd Bergmann wrote:
> > On Fri, Jun 28, 2019 at 3:17 PM Christoph Hellwig <[email protected]> wrote:
> > >
> > > On Fri, Jun 28, 2019 at 03:09:47PM +0200, Arnd Bergmann wrote:
> > > > I came across this on arm-nommu (which disables
> > > > CONFIG_CPU_SPECTRE) during randconfig testing.
> > > >
> > > > I don't see an easy way to add this in there, short of rewriting the
> > > > whole __get_user_err() function. Any suggestions?
> > >
> > > Can't we just fall back to using copy_from_user with a little wrapper
> > > that switches based on sizeof()?
> >
> > I came up with something now. It's not pretty, but seems to satisfy the
> > compiler. Not a proper patch yet, but let me know if you find a bug.
>
> Have you checked what the behaviour is when "ptr" is a pointer to a
> pointer? I think you'll end up with a compiler warning for every
> case, complaining about casting an unsigned long long to a pointer.
I have built lots of kernels using this patch as a test, though my autobuilder
is currently configured to use clang-8, and other compilers or versions
might show more warnings.
> > uaccess_restore(__ua_flags); \
> > - (x) = (__typeof__(*(ptr)))__gu_val; \
> > + (x) = __builtin_choose_expr(sizeof(*(ptr)) == 8, \
> > + (__typeof__(*(ptr)))__gu_val8, \
> > + (__typeof__(*(ptr)))__gu_val); \
> > } while (0)
The __builtin_choose_expr() here is supposed to take care of the case
of a pointer type, gcc and clang should both ignore the non-taken
branch and only produce warnings for the case they actually use.
Arnd