| Message ID | 20190628104007.2721479-1-arnd@arndb.de |
|---|---|
| State | New |
| Headers | show |
| Series | f2fs: fix 32-bit linking | expand |
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 <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
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 <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 { \
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
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
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 <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
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
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);
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(-) -- 2.20.0