From: Stephen Rothwell <hidden> Date: 2016-01-27 02:40:28
Hi Benjamin,
On Tue, 12 Jan 2016 11:38:35 -0500 Benjamin LaHaise [off-list ref] wrote:
On Tue, Jan 12, 2016 at 04:40:34PM +1100, Stephen Rothwell wrote:
quoted
After merging the aio tree, today's linux-next build (arm
multi_v7_defconfig) failed like this:
fs/built-in.o: In function `aio_thread_op_foo_at':
file.c:(.text+0x43808): undefined reference to `__get_user_bad'
file.c:(.text+0x43838): undefined reference to `__get_user_bad'
This is very strange. It seems to imply that __get_user() doesn't
handle 64 bit values, which is completely broken behaviour on the
architecture's part if true. Can any ARM folks comment on the right
fix here?
Well, probably only if you cc them :-)
Indeed, __get_user on arm does not handle 64 bit objects, where as
get_user does ...
Background: new aio code is adding __get_user() calls referencing 64
bit quantities (__u64 and __s64).
--
Cheers,
Stephen Rothwell sfr at canb.auug.org.au
From: Russell King - ARM Linux <hidden> Date: 2016-01-29 11:31:11
On Wed, Jan 27, 2016 at 01:40:24PM +1100, Stephen Rothwell wrote:
Hi Benjamin,
On Tue, 12 Jan 2016 11:38:35 -0500 Benjamin LaHaise [off-list ref] wrote:
quoted
On Tue, Jan 12, 2016 at 04:40:34PM +1100, Stephen Rothwell wrote:
quoted
After merging the aio tree, today's linux-next build (arm
multi_v7_defconfig) failed like this:
fs/built-in.o: In function `aio_thread_op_foo_at':
file.c:(.text+0x43808): undefined reference to `__get_user_bad'
file.c:(.text+0x43838): undefined reference to `__get_user_bad'
This is very strange. It seems to imply that __get_user() doesn't
handle 64 bit values, which is completely broken behaviour on the
architecture's part if true. Can any ARM folks comment on the right
fix here?
Well, probably only if you cc them :-)
Indeed, __get_user on arm does not handle 64 bit objects, where as
get_user does ...
I believe adding 64-bit support to __get_user() is going to be a really
painful exercise. It took long enough to make get_user() accept it
without creating any new warnings, and then bug fix it for big endian.
__get_user() has the added complexity that it's inline assembler, and
we can't play the same games that we used in __get_user().
It's not a simple case of:
#define __get_user_err(x, ptr, err) \
do { \
unsigned long __gu_addr = (unsigned long)(ptr); \
- unsigned long __gu_val; \
+ unsigned long long __gu_val; \
unsigned int __ua_flags; \
__chk_user_ptr(ptr); \
might_fault(); \
__ua_flags = uaccess_save_and_enable(); \
switch (sizeof(*(ptr))) { \
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_val, __gu_addr, err); break; \
default: (__gu_val) = __get_user_bad(); \
} \
uaccess_restore(__ua_flags); \
(x) = (__typeof__(*(ptr)))__gu_val; \
} while (0)
as, while that works for integer 'x', it doesn't work when 'x' is a
pointer type, as the cast on the last line creates a GCC warning
about casting to a different size.
You can't move the cast into the switch to eliminate that, because
GCC still warns even though the code is unreachable. Same problem
exists if you move the variable declaration inside the switch.
Background: new aio code is adding __get_user() calls referencing 64
bit quantities (__u64 and __s64).
There's lots more architectures which do not support 64-bit get_user()
_or_ __get_user(): avr32, blackfin, metag for example, and m68k which
has this interesting thing "/* case 8: disabled because gcc-4.1 has a
broken typeof \" in its *get_user() implementation.
Even x86-32 doesn't support it:
#define __get_user(x, ptr) \
__get_user_nocheck((x), (ptr), sizeof(*(ptr)))
#define __get_user_nocheck(x, ptr, size) \
({ \
int __gu_err; \
unsigned long __gu_val; \
__uaccess_begin(); \
__get_user_size(__gu_val, (ptr), (size), __gu_err, -EFAULT); \
__uaccess_end(); \
(x) = (__force __typeof__(*(ptr)))__gu_val; \
__builtin_expect(__gu_err, 0); \
})
#define __get_user_size(x, ptr, size, retval, errret) \
do { \
retval = 0; \
__chk_user_ptr(ptr); \
switch (size) { \
...
case 8: \
__get_user_asm_u64(x, ptr, retval, errret); \
break; \
default: \
(x) = __get_user_bad(); \
} \
} while (0)
#ifdef CONFIG_X86_32
#define __get_user_asm_u64(x, ptr, retval, errret) (x) = __get_user_bad()
#define __get_user_asm_ex_u64(x, ptr) (x) = __get_user_bad()
Note that __get_user_nocheck() above always uses a 32-bit integer for
__gu_val, so fixing x86-32 will not be a case of just providing a
better __get_user_asm_u64() - it's the same problem that I have on ARM.
The problem for 32-bit architectures is being able to cope sanely with:
* different endians on the same arch
* gcc warning about casting to different sizes, even when the code is
unreachable
* preserving proper and correct typechecking in get_user() and friends
So, I suspect if AIO were to run a test build on x86-32, they would find
the same errors.
Anyone working on this needs to ensure that any implementation builds
without warning on all of these:
int test_8(unsigned char *v, unsigned char *p)
{
return __get_user(*v, p);
}
int test_8_constp(unsigned char *v, const unsigned char *p)
{
return __get_user(*v, p);
}
int test_8_volatilep(unsigned char *v, volatile unsigned char *p)
{
return __get_user(*v, p);
}
int test_16(unsigned short *v, unsigned short *p)
{
return __get_user(*v, p);
}
int test_16_constp(unsigned short *v, const unsigned short *p)
{
return __get_user(*v, p);
}
int test_32(unsigned int *v, unsigned int *p)
{
return __get_user(*v, p);
}
int test_32_constp(unsigned int *v, const unsigned int *p)
{
return __get_user(*v, p);
}
int test_64(unsigned long long *v, unsigned long long *p)
{
return __get_user(*v, p);
}
int test_64_constp(unsigned long long *v, const unsigned long long *p)
{
return __get_user(*v, p);
}
int test_ptr(unsigned int **v, unsigned int **p)
{
return __get_user(*v, p);
}
int test_const(unsigned int *v, const unsigned int *p)
{
return __get_user(*v, p);
}
int test_64_narrow(unsigned long *v, unsigned long long *p)
{
return __get_user(*v, p);
}
int test_32_wide(unsigned long long *v, unsigned long *p)
{
return __get_user(*v, p);
}
except for this which must warn:
int test_wrong(char **v, const char **p)
{
return __get_user(*v, p);
}
which comes from my test set for when get_user() was being worked on.
So, in summary, it seems that the 32-bit architectures I've looked at
so far do not support 64-bit __get_user(), even the popular ones.
--
RMK's Patch system: http://www.arm.linux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
On Fri, Jan 29, 2016 at 12:30 PM, Russell King - ARM Linux
[off-list ref] wrote:
quoted
Background: new aio code is adding __get_user() calls referencing 64
bit quantities (__u64 and __s64).
There's lots more architectures which do not support 64-bit get_user()
_or_ __get_user(): avr32, blackfin, metag for example, and m68k which
has this interesting thing "/* case 8: disabled because gcc-4.1 has a
broken typeof \" in its *get_user() implementation.
And if you enable it again, you get lots of "warning: cast to pointer from
integer of different size", like you mentioned.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert at linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
From: Stephen Rothwell <hidden> Date: 2016-02-04 02:20:04
Hi Benjamin,
On Fri, 29 Jan 2016 13:03:39 +0100 Geert Uytterhoeven [off-list ref] wrote:
On Fri, Jan 29, 2016 at 12:30 PM, Russell King - ARM Linux
[off-list ref] wrote:
quoted
quoted
Background: new aio code is adding __get_user() calls referencing 64
bit quantities (__u64 and __s64).
There's lots more architectures which do not support 64-bit get_user()
_or_ __get_user(): avr32, blackfin, metag for example, and m68k which
has this interesting thing "/* case 8: disabled because gcc-4.1 has a
broken typeof \" in its *get_user() implementation.
And if you enable it again, you get lots of "warning: cast to pointer from
integer of different size", like you mentioned.
Any thoughts? I am still using the version of tha aio tree from
next-20160111.
--
Cheers,
Stephen Rothwell
From: Benjamin LaHaise <bcrl@kvack.org> Date: 2016-02-04 13:41:45
On Thu, Feb 04, 2016 at 01:19:59PM +1100, Stephen Rothwell wrote:
Hi Benjamin,
On Fri, 29 Jan 2016 13:03:39 +0100 Geert Uytterhoeven [off-list ref] wrote:
quoted
On Fri, Jan 29, 2016 at 12:30 PM, Russell King - ARM Linux
[off-list ref] wrote:
quoted
quoted
Background: new aio code is adding __get_user() calls referencing 64
bit quantities (__u64 and __s64).
There's lots more architectures which do not support 64-bit get_user()
_or_ __get_user(): avr32, blackfin, metag for example, and m68k which
has this interesting thing "/* case 8: disabled because gcc-4.1 has a
broken typeof \" in its *get_user() implementation.
And if you enable it again, you get lots of "warning: cast to pointer from
integer of different size", like you mentioned.
Any thoughts? I am still using the version of tha aio tree from
next-20160111.
I am still convinced that this is an architecture issue. Given that 64 bit
values work in the *get_user implementations on other architectures, I see
no reason there should need to be a workaround for this in common code.
-ben
From: Russell King - ARM Linux <hidden> Date: 2016-02-04 13:51:11
On Thu, Feb 04, 2016 at 08:41:42AM -0500, Benjamin LaHaise wrote:
On Thu, Feb 04, 2016 at 01:19:59PM +1100, Stephen Rothwell wrote:
quoted
Hi Benjamin,
On Fri, 29 Jan 2016 13:03:39 +0100 Geert Uytterhoeven [off-list ref] wrote:
quoted
On Fri, Jan 29, 2016 at 12:30 PM, Russell King - ARM Linux
[off-list ref] wrote:
quoted
quoted
Background: new aio code is adding __get_user() calls referencing 64
bit quantities (__u64 and __s64).
There's lots more architectures which do not support 64-bit get_user()
_or_ __get_user(): avr32, blackfin, metag for example, and m68k which
has this interesting thing "/* case 8: disabled because gcc-4.1 has a
broken typeof \" in its *get_user() implementation.
And if you enable it again, you get lots of "warning: cast to pointer from
integer of different size", like you mentioned.
Any thoughts? I am still using the version of tha aio tree from
next-20160111.
I am still convinced that this is an architecture issue. Given that 64 bit
values work in the *get_user implementations on other architectures, I see
no reason there should need to be a workaround for this in common code.
So you're happy to break x86-32 then...
--
RMK's Patch system: http://www.arm.linux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
From: Benjamin LaHaise <bcrl@kvack.org> Date: 2016-02-04 14:08:24
On Thu, Feb 04, 2016 at 01:50:56PM +0000, Russell King - ARM Linux wrote:
quoted
I am still convinced that this is an architecture issue. Given that 64 bit
values work in the *get_user implementations on other architectures, I see
no reason there should need to be a workaround for this in common code.
So you're happy to break x86-32 then...
x86-32 works fine.
-ben
--
"Thought is the essence of where you are now."
From: Russell King - ARM Linux <hidden> Date: 2016-02-04 14:13:05
On Thu, Feb 04, 2016 at 09:08:22AM -0500, Benjamin LaHaise wrote:
On Thu, Feb 04, 2016 at 01:50:56PM +0000, Russell King - ARM Linux wrote:
quoted
quoted
I am still convinced that this is an architecture issue. Given that 64 bit
values work in the *get_user implementations on other architectures, I see
no reason there should need to be a workaround for this in common code.
So you're happy to break x86-32 then...
x86-32 works fine.
Let me repeat the quote from my previous email:
#define __get_user(x, ptr) \
__get_user_nocheck((x), (ptr), sizeof(*(ptr)))
#define __get_user_nocheck(x, ptr, size) \
({ \
int __gu_err; \
unsigned long __gu_val; \
__uaccess_begin(); \
__get_user_size(__gu_val, (ptr), (size), __gu_err, -EFAULT); \
__uaccess_end(); \
(x) = (__force __typeof__(*(ptr)))__gu_val; \
__builtin_expect(__gu_err, 0); \
})
#define __get_user_size(x, ptr, size, retval, errret) \
do { \
retval = 0; \
__chk_user_ptr(ptr); \
switch (size) { \
case 1: \
__get_user_asm(x, ptr, retval, "b", "b", "=q", errret); \
break; \
case 2: \
__get_user_asm(x, ptr, retval, "w", "w", "=r", errret); \
break; \
case 4: \
__get_user_asm(x, ptr, retval, "l", "k", "=r", errret); \
break; \
case 8: \
__get_user_asm_u64(x, ptr, retval, errret); \
break; \
default: \
(x) = __get_user_bad(); \
} \
} while (0)
#ifdef CONFIG_X86_32
#define __get_user_asm_u64(x, ptr, retval, errret) (x) = __get_user_bad()
#define __get_user_asm_ex_u64(x, ptr) (x) = __get_user_bad()
#else
#define __get_user_asm_u64(x, ptr, retval, errret) \
__get_user_asm(x, ptr, retval, "q", "", "=r", errret)
#define __get_user_asm_ex_u64(x, ptr) \
__get_user_asm_ex(x, ptr, "q", "", "=r")
#endif
Hence, __get_user() on x86-32 with a 64-bit quantity results in
__get_user_bad() being called, which is an undefined function.
Only if you build with x86-64 support enabled (iow, CONFIG_X86_32 not
defined) then you get the 64-bit __get_user() support.
Given this, I fail to see how x86-32 can possibly work.
--
RMK's Patch system: http://www.arm.linux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
From: Benjamin LaHaise <bcrl@kvack.org> Date: 2016-02-04 14:32:08
On Thu, Feb 04, 2016 at 02:12:53PM +0000, Russell King - ARM Linux wrote:
Hence, __get_user() on x86-32 with a 64-bit quantity results in
__get_user_bad() being called, which is an undefined function.
Only if you build with x86-64 support enabled (iow, CONFIG_X86_32 not
defined) then you get the 64-bit __get_user() support.
Given this, I fail to see how x86-32 can possibly work.
You're right; mea culpa. It compiles without warning on x86-32, but it
does not link. I still think this is broken archtecture stupidity since
put_user() works for 64 bit data types.
-ben
--
"Thought is the essence of where you are now."
From: Russell King - ARM Linux <hidden> Date: 2016-02-04 14:39:26
On Thu, Feb 04, 2016 at 09:32:04AM -0500, Benjamin LaHaise wrote:
On Thu, Feb 04, 2016 at 02:12:53PM +0000, Russell King - ARM Linux wrote:
quoted
Hence, __get_user() on x86-32 with a 64-bit quantity results in
__get_user_bad() being called, which is an undefined function.
Only if you build with x86-64 support enabled (iow, CONFIG_X86_32 not
defined) then you get the 64-bit __get_user() support.
Given this, I fail to see how x86-32 can possibly work.
You're right; mea culpa. It compiles without warning on x86-32, but it
does not link. I still think this is broken archtecture stupidity since
put_user() works for 64 bit data types.
Indeed, and you'll find that several other architectures besides ARM and
x86-32 have exactly the same problem - as I listed in my message from a
few days ago.
Okay, so now I get to set you a challenge, since you're the one wanting
64-bit __get_user(): try implementing it on x86-32 :)
Also in my previous message from a few days ago I provided a set of
functions which test out the implementation. Here they are... again.
All these should not produce any warnings, and should produce correct
code - especially the narrowing/widening tests:
int test_8(unsigned char *v, unsigned char *p)
{ return __get_user(*v, p); }
int test_8_constp(unsigned char *v, const unsigned char *p)
{ return __get_user(*v, p); }
int test_8_volatilep(unsigned char *v, volatile unsigned char *p)
{ return __get_user(*v, p); }
int test_16(unsigned short *v, unsigned short *p)
{ return __get_user(*v, p); }
int test_16_constp(unsigned short *v, const unsigned short *p)
{ return __get_user(*v, p); }
int test_32(unsigned int *v, unsigned int *p)
{ return __get_user(*v, p); }
int test_32_constp(unsigned int *v, const unsigned int *p)
{ return __get_user(*v, p); }
int test_64(unsigned long long *v, unsigned long long *p)
{ return __get_user(*v, p); }
int test_64_constp(unsigned long long *v, const unsigned long long *p)
{ return __get_user(*v, p); }
int test_ptr(unsigned int **v, unsigned int **p)
{ return __get_user(*v, p); }
int test_const(unsigned int *v, const unsigned int *p)
{ return __get_user(*v, p); }
int test_64_narrow(unsigned long *v, unsigned long long *p)
{ return __get_user(*v, p); }
int test_32_wide(unsigned long long *v, unsigned long *p)
{ return __get_user(*v, p); }
However, this one should warn:
int test_wrong(char **v, const char **p)
{ return __get_user(*v, p); }
Good luck (I think you'll need lots of it to get a working solution)! :)
--
RMK's Patch system: http://www.arm.linux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
From: Benjamin LaHaise <bcrl@kvack.org> Date: 2016-02-04 16:01:04
On Thu, Feb 04, 2016 at 02:39:07PM +0000, Russell King - ARM Linux wrote:
However, this one should warn:
int test_wrong(char **v, const char **p)
{ return __get_user(*v, p); }
Good luck (I think you'll need lots of it to get a working solution)! :)
This works with your test cases on x86-32. Note that it's only compile +
link tested at present.
-ben
--
"Thought is the essence of where you are now."
From: Russell King - ARM Linux <hidden> Date: 2016-02-04 16:17:56
On Thu, Feb 04, 2016 at 11:01:01AM -0500, Benjamin LaHaise wrote:
On Thu, Feb 04, 2016 at 02:39:07PM +0000, Russell King - ARM Linux wrote:
quoted
However, this one should warn:
int test_wrong(char **v, const char **p)
{ return __get_user(*v, p); }
Good luck (I think you'll need lots of it to get a working solution)! :)
This works with your test cases on x86-32. Note that it's only compile +
link tested at present.
That's the easy bit!
The problem you're going to run into is here:
#define __get_user_nocheck(x, ptr, size) \
({ \
int __gu_err; \
unsigned long __gu_val; \
__uaccess_begin(); \
__get_user_size(__gu_val, (ptr), (size), __gu_err, -EFAULT); \
__uaccess_end(); \
(x) = (__force __typeof__(*(ptr)))__gu_val; \
__gu_val will be 32-bit, even when you're wanting a 64-bit quantity.
That's where the fun and games start...
--
RMK's Patch system: http://www.arm.linux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
From: Benjamin LaHaise <bcrl@kvack.org> Date: 2016-02-04 16:27:54
On Thu, Feb 04, 2016 at 04:17:42PM +0000, Russell King - ARM Linux wrote:
On Thu, Feb 04, 2016 at 11:01:01AM -0500, Benjamin LaHaise wrote:
quoted
On Thu, Feb 04, 2016 at 02:39:07PM +0000, Russell King - ARM Linux wrote:
quoted
However, this one should warn:
int test_wrong(char **v, const char **p)
{ return __get_user(*v, p); }
Good luck (I think you'll need lots of it to get a working solution)! :)
This works with your test cases on x86-32. Note that it's only compile +
link tested at present.
That's the easy bit!
The problem you're going to run into is here:
#define __get_user_nocheck(x, ptr, size) \
({ \
int __gu_err; \
unsigned long __gu_val; \
__uaccess_begin(); \
__get_user_size(__gu_val, (ptr), (size), __gu_err, -EFAULT); \
__uaccess_end(); \
(x) = (__force __typeof__(*(ptr)))__gu_val; \
__gu_val will be 32-bit, even when you're wanting a 64-bit quantity.
That's where the fun and games start...
Ugh. You're making me install a 32 bit distro.....!
-ben
From: Benjamin LaHaise <bcrl@kvack.org> Date: 2016-02-04 16:47:22
On Thu, Feb 04, 2016 at 04:17:42PM +0000, Russell King - ARM Linux wrote:
That's the easy bit!
The problem you're going to run into is here:
#define __get_user_nocheck(x, ptr, size) \
({ \
int __gu_err; \
unsigned long __gu_val; \
__uaccess_begin(); \
__get_user_size(__gu_val, (ptr), (size), __gu_err, -EFAULT); \
__uaccess_end(); \
(x) = (__force __typeof__(*(ptr)))__gu_val; \
__gu_val will be 32-bit, even when you're wanting a 64-bit quantity.
That's where the fun and games start...
You're right -- it's quite non-trivial. How evil would it be to make a
separate __get_user64() macro?
-ben
--
"Thought is the essence of where you are now."
From: Benjamin LaHaise <bcrl@kvack.org> Date: 2016-02-04 18:48:27
On Thu, Feb 04, 2016 at 04:17:42PM +0000, Russell King - ARM Linux wrote:
__gu_val will be 32-bit, even when you're wanting a 64-bit quantity.
That's where the fun and games start...
Okay, I figured out how to do it: instead of using a 64 bit unsigned long
long for __gu_val, use an array of 2 unsigned longs. See the patch below
which I verified boots, passes your tests and doesn't truncate 64 bit
values. Comments? A simple test module to verify things is located at
http://www.kvack.org/~bcrl/test_module2.c .
-ben
--
"Thought is the essence of where you are now."