[PATCH] arch/openrisc: Fix issues with access_ok()

[PATCH] arch/openrisc: Fix issues with access_ok()

[Stafford Horne]

The commit 594cc251fdd0 ("make 'user_access_begin()' do 'access_ok()'")
exposed incorrect implementations of access_ok() macro in several
architectures.  This change fixes 2 issues found in OpenRISC.

OpenRISC was not properly using parenthesis for arguments and also using
arguments twice.  This patch fixes those 2 issues.

I test booted this patch with v5.0-rc1 on qemu and it's working fine.

Cc: Guenter Roeck <linux@roeck-us.net>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Reported-by: Linus Torvalds <torvalds@linux-foundation.org>
Signed-off-by: Stafford Horne <shorne@gmail.com>
---
 arch/openrisc/include/asm/uaccess.h | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/arch/openrisc/include/asm/uaccess.h b/arch/openrisc/include/asm/uaccess.h
index bc8191a34db7..a44682c8adc3 100644
--- a/arch/openrisc/include/asm/uaccess.h
+++ b/arch/openrisc/include/asm/uaccess.h
@@ -58,8 +58,12 @@
 /* Ensure that addr is below task's addr_limit */
 #define __addr_ok(addr) ((unsigned long) addr < get_fs())
 
-#define access_ok(addr, size) \
-	__range_ok((unsigned long)addr, (unsigned long)size)
+#define access_ok(addr, size)						\
+({ 									\
+	unsigned long __ao_addr = (unsigned long)(addr);		\
+	unsigned long __ao_size = (unsigned long)(size);		\
+	__range_ok(__ao_addr, __ao_size);				\
+})
 
 /*
  * These are the main single-value transfer routines.  They automatically
-- 
2.19.1

[OpenRISC] [PATCH] arch/openrisc: Fix issues with access_ok()

[Stafford Horne]

The commit 594cc251fdd0 ("make 'user_access_begin()' do 'access_ok()'")
exposed incorrect implementations of access_ok() macro in several
architectures.  This change fixes 2 issues found in OpenRISC.

OpenRISC was not properly using parenthesis for arguments and also using
arguments twice.  This patch fixes those 2 issues.

I test booted this patch with v5.0-rc1 on qemu and it's working fine.

Cc: Guenter Roeck <linux@roeck-us.net>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Reported-by: Linus Torvalds <torvalds@linux-foundation.org>
Signed-off-by: Stafford Horne <shorne@gmail.com>
---
 arch/openrisc/include/asm/uaccess.h | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/arch/openrisc/include/asm/uaccess.h b/arch/openrisc/include/asm/uaccess.h
index bc8191a34db7..a44682c8adc3 100644
--- a/arch/openrisc/include/asm/uaccess.h
+++ b/arch/openrisc/include/asm/uaccess.h
@@ -58,8 +58,12 @@
 /* Ensure that addr is below task's addr_limit */
 #define __addr_ok(addr) ((unsigned long) addr < get_fs())
 
-#define access_ok(addr, size) \
-	__range_ok((unsigned long)addr, (unsigned long)size)
+#define access_ok(addr, size)						\
+({ 									\
+	unsigned long __ao_addr = (unsigned long)(addr);		\
+	unsigned long __ao_size = (unsigned long)(size);		\
+	__range_ok(__ao_addr, __ao_size);				\
+})
 
 /*
  * These are the main single-value transfer routines.  They automatically
-- 
2.19.1

Re: [PATCH] arch/openrisc: Fix issues with access_ok()

[Linus Torvalds]

On Tue, Jan 8, 2019 at 5:15 AM Stafford Horne <shorne@gmail.com> wrote:
>
> The commit 594cc251fdd0 ("make 'user_access_begin()' do 'access_ok()'")
> exposed incorrect implementations of access_ok() macro in several
> architectures.  This change fixes 2 issues found in OpenRISC.

Looks good to me. Should I apply this directly, or expect a pull
request with it later?

             Linus

[OpenRISC] [PATCH] arch/openrisc: Fix issues with access_ok()

[Linus Torvalds]

On Tue, Jan 8, 2019 at 5:15 AM Stafford Horne <shorne@gmail.com> wrote:
>
> The commit 594cc251fdd0 ("make 'user_access_begin()' do 'access_ok()'")
> exposed incorrect implementations of access_ok() macro in several
> architectures.  This change fixes 2 issues found in OpenRISC.

Looks good to me. Should I apply this directly, or expect a pull
request with it later?

             Linus

Re: [PATCH] arch/openrisc: Fix issues with access_ok()

[Linus Torvalds]

On Tue, Jan 8, 2019 at 10:10 AM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> On Tue, Jan 8, 2019 at 5:15 AM Stafford Horne <shorne@gmail.com> wrote:
> >
> > The commit 594cc251fdd0 ("make 'user_access_begin()' do 'access_ok()'")
> > exposed incorrect implementations of access_ok() macro in several
> > architectures.  This change fixes 2 issues found in OpenRISC.
>
> Looks good to me. Should I apply this directly, or expect a pull
> request with it later?

Oh, and replying to myself with a quick note: it might also be a good
idea to just make it an inline function.

The only reason I did alpha and SH as those macros with a statement
expression was that because I don't have a cross-build environment,
continuing to do it as a macro was the safest thing from a build
standpoint.

One big difference between a macro and an inline function is that the
inline function requires everything to be declared at the point of the
function definition, while the macro can use things that get declared
only later (like "get_fs()"). So a macro can use functions and other
macros that aren't declared yet, but will be declared by the time the
macro is actually _used_.

So when changing the macro "blind", it was simply safer to just keep
it a macro and just make it a bit more complex. But since you can
build-test your changes, making (for example) __range_ok() be an
inline function might have been the cleaner solution to the "use
twice" issue.

But your existing patch looks fine to me too, so don't worry too much
about it. I just wanted to point out that the reason I did alpha and
SH the way I did was not some "macros are better", but rather "Linus
is weak and lazy".

               Linus

[OpenRISC] [PATCH] arch/openrisc: Fix issues with access_ok()

[Linus Torvalds]

On Tue, Jan 8, 2019 at 10:10 AM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> On Tue, Jan 8, 2019 at 5:15 AM Stafford Horne <shorne@gmail.com> wrote:
> >
> > The commit 594cc251fdd0 ("make 'user_access_begin()' do 'access_ok()'")
> > exposed incorrect implementations of access_ok() macro in several
> > architectures.  This change fixes 2 issues found in OpenRISC.
>
> Looks good to me. Should I apply this directly, or expect a pull
> request with it later?

Oh, and replying to myself with a quick note: it might also be a good
idea to just make it an inline function.

The only reason I did alpha and SH as those macros with a statement
expression was that because I don't have a cross-build environment,
continuing to do it as a macro was the safest thing from a build
standpoint.

One big difference between a macro and an inline function is that the
inline function requires everything to be declared at the point of the
function definition, while the macro can use things that get declared
only later (like "get_fs()"). So a macro can use functions and other
macros that aren't declared yet, but will be declared by the time the
macro is actually _used_.

So when changing the macro "blind", it was simply safer to just keep
it a macro and just make it a bit more complex. But since you can
build-test your changes, making (for example) __range_ok() be an
inline function might have been the cleaner solution to the "use
twice" issue.

But your existing patch looks fine to me too, so don't worry too much
about it. I just wanted to point out that the reason I did alpha and
SH the way I did was not some "macros are better", but rather "Linus
is weak and lazy".

               Linus

Re: [PATCH] arch/openrisc: Fix issues with access_ok()

[Stafford Horne]

On Tue, Jan 08, 2019 at 10:16:39AM -0800, Linus Torvalds wrote:
> On Tue, Jan 8, 2019 at 10:10 AM Linus Torvalds
> <torvalds@linux-foundation.org> wrote:
> >
> > On Tue, Jan 8, 2019 at 5:15 AM Stafford Horne <shorne@gmail.com> wrote:
> > >
> > > The commit 594cc251fdd0 ("make 'user_access_begin()' do 'access_ok()'")
> > > exposed incorrect implementations of access_ok() macro in several
> > > architectures.  This change fixes 2 issues found in OpenRISC.
> >
> > Looks good to me. Should I apply this directly, or expect a pull
> > request with it later?
> 
> Oh, and replying to myself with a quick note: it might also be a good
> idea to just make it an inline function.
> 
> The only reason I did alpha and SH as those macros with a statement
> expression was that because I don't have a cross-build environment,
> continuing to do it as a macro was the safest thing from a build
> standpoint.
> 
> One big difference between a macro and an inline function is that the
> inline function requires everything to be declared at the point of the
> function definition, while the macro can use things that get declared
> only later (like "get_fs()"). So a macro can use functions and other
> macros that aren't declared yet, but will be declared by the time the
> macro is actually _used_.
> 
> So when changing the macro "blind", it was simply safer to just keep
> it a macro and just make it a bit more complex. But since you can
> build-test your changes, making (for example) __range_ok() be an
> inline function might have been the cleaner solution to the "use
> twice" issue.
> 
> But your existing patch looks fine to me too, so don't worry too much
> about it. I just wanted to point out that the reason I did alpha and
> SH the way I did was not some "macros are better", but rather "Linus
> is weak and lazy".

Hi Linus,

Please take this patch directly.

The inline's are a good point.  I will take some time to address this and some
other cleanups.

Note (for others) I had to apply patches from Masahiro Yamada to test this as
v5.0-rc1 build was broken for OpenRISC.  Those patches are already on Linus'
master.

-Stafford

[OpenRISC] [PATCH] arch/openrisc: Fix issues with access_ok()

[Stafford Horne]

On Tue, Jan 08, 2019 at 10:16:39AM -0800, Linus Torvalds wrote:
> On Tue, Jan 8, 2019 at 10:10 AM Linus Torvalds
> <torvalds@linux-foundation.org> wrote:
> >
> > On Tue, Jan 8, 2019 at 5:15 AM Stafford Horne <shorne@gmail.com> wrote:
> > >
> > > The commit 594cc251fdd0 ("make 'user_access_begin()' do 'access_ok()'")
> > > exposed incorrect implementations of access_ok() macro in several
> > > architectures.  This change fixes 2 issues found in OpenRISC.
> >
> > Looks good to me. Should I apply this directly, or expect a pull
> > request with it later?
> 
> Oh, and replying to myself with a quick note: it might also be a good
> idea to just make it an inline function.
> 
> The only reason I did alpha and SH as those macros with a statement
> expression was that because I don't have a cross-build environment,
> continuing to do it as a macro was the safest thing from a build
> standpoint.
> 
> One big difference between a macro and an inline function is that the
> inline function requires everything to be declared at the point of the
> function definition, while the macro can use things that get declared
> only later (like "get_fs()"). So a macro can use functions and other
> macros that aren't declared yet, but will be declared by the time the
> macro is actually _used_.
> 
> So when changing the macro "blind", it was simply safer to just keep
> it a macro and just make it a bit more complex. But since you can
> build-test your changes, making (for example) __range_ok() be an
> inline function might have been the cleaner solution to the "use
> twice" issue.
> 
> But your existing patch looks fine to me too, so don't worry too much
> about it. I just wanted to point out that the reason I did alpha and
> SH the way I did was not some "macros are better", but rather "Linus
> is weak and lazy".

Hi Linus,

Please take this patch directly.

The inline's are a good point.  I will take some time to address this and some
other cleanups.

Note (for others) I had to apply patches from Masahiro Yamada to test this as
v5.0-rc1 build was broken for OpenRISC.  Those patches are already on Linus'
master.

-Stafford