[PATCH] f2fs: fix 32-bit linking

[PATCH] f2fs: fix 32-bit linking

[Arnd Bergmann]

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 <arnd@arndb.de>
---
 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

Re: [PATCH] f2fs: fix 32-bit linking

[Christoph Hellwig]

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.

Re: [PATCH] f2fs: fix 32-bit linking

[Arnd Bergmann]

On Fri, Jun 28, 2019 at 2:44 PM Christoph Hellwig <hch@infradead.org> 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

Re: [PATCH] f2fs: fix 32-bit linking

[Christoph Hellwig]

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()?

Re: [PATCH] f2fs: fix 32-bit linking

[Arnd Bergmann]

On Fri, Jun 28, 2019 at 3:17 PM Christoph Hellwig <hch@infradead.org> 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 {                                                            \

Re: [PATCH] f2fs: fix 32-bit linking

[Russell King - ARM Linux admin]

On Fri, Jun 28, 2019 at 04:46:14PM +0200, Arnd Bergmann wrote:
> On Fri, Jun 28, 2019 at 3:17 PM Christoph Hellwig <hch@infradead.org> 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

Re: [PATCH] f2fs: fix 32-bit linking

[Arnd Bergmann]

On Fri, Jun 28, 2019 at 7:58 PM Russell King - ARM Linux admin
<linux@armlinux.org.uk> 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 <hch@infradead.org> 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

Re: [PATCH] f2fs: fix 32-bit linking

[Randy Dunlap]

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

Re: [PATCH] f2fs: fix 32-bit linking

[Jaegeuk Kim]

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 <arnd@arndb.de>
> ---
>  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

Re: [PATCH] f2fs: fix 32-bit linking

[Arnd Bergmann]

On Fri, Jun 28, 2019 at 5:59 PM Jaegeuk Kim <jaegeuk@kernel.org> 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