[PATCH] asm-generic: uaccess: fix up local access_ok() usage

[PATCH] asm-generic: uaccess: fix up local access_ok() usage

[Mike Frysinger]

There's no reason that I can see to use the short __access_ok() form
directly when the access_ok() is clearer in intent and for more people,
expands to the same C code (i.e. always specify the first field -- access
type).  Not all no-mmu systems lack memory protection, so the read/write
could feasibly be checked.

Also, the strnlen_user() function was missing a access_ok() check on the
pointer given.  We've had cases on Blackfin systems where test cases
caused kernel crashes here because userspace passed up a NULL/-1 pointer
and the kernel gladly attempted to run strlen() on it.

Signed-off-by: Mike Frysinger <vapier@gentoo.org>
---
 include/asm-generic/uaccess.h |   14 ++++++++------
 1 files changed, 8 insertions(+), 6 deletions(-)

diff --git a/include/asm-generic/uaccess.h b/include/asm-generic/uaccess.h
index 6d8cab2..705410b 100644
--- a/include/asm-generic/uaccess.h
+++ b/include/asm-generic/uaccess.h
@@ -163,7 +163,7 @@ static inline __must_check long __copy_to_user(void __user *to,
 #define put_user(x, ptr)					\
 ({								\
 	might_sleep();						\
-	__access_ok(ptr, sizeof (*ptr)) ?			\
+	access_ok(VERIFY_WRITE, ptr, sizeof (*ptr)) ?			\
 		__put_user(x, ptr) :				\
 		-EFAULT;					\
 })
@@ -219,7 +219,7 @@ extern int __put_user_bad(void) __attribute__((noreturn));
 #define get_user(x, ptr)					\
 ({								\
 	might_sleep();						\
-	__access_ok(ptr, sizeof (*ptr)) ?			\
+	access_ok(VERIFY_READ, ptr, sizeof (*ptr)) ?			\
 		__get_user(x, ptr) :				\
 		-EFAULT;					\
 })
@@ -244,7 +244,7 @@ static inline long copy_from_user(void *to,
 		const void __user * from, unsigned long n)
 {
 	might_sleep();
-	if (__access_ok(from, n))
+	if (access_ok(VERIFY_READ, from, n))
 		return __copy_from_user(to, from, n);
 	else
 		return n;
@@ -254,7 +254,7 @@ static inline long copy_to_user(void __user *to,
 		const void *from, unsigned long n)
 {
 	might_sleep();
-	if (__access_ok(to, n))
+	if (access_ok(VERIFY_WRITE, to, n))
 		return __copy_to_user(to, from, n);
 	else
 		return n;
@@ -278,7 +278,7 @@ __strncpy_from_user(char *dst, const char __user *src, long count)
 static inline long
 strncpy_from_user(char *dst, const char __user *src, long count)
 {
-	if (!__access_ok(src, 1))
+	if (!access_ok(VERIFY_READ, src, 1))
 		return -EFAULT;
 	return __strncpy_from_user(dst, src, count);
 }
@@ -291,6 +291,8 @@ strncpy_from_user(char *dst, const char __user *src, long count)
 #ifndef strnlen_user
 static inline long strnlen_user(const char __user *src, long n)
 {
+	if (!access_ok(VERIFY_READ, src, 1))
+		return 0;
 	return strlen((void * __force)src) + 1;
 }
 #endif
@@ -316,7 +318,7 @@ static inline __must_check unsigned long
 clear_user(void __user *to, unsigned long n)
 {
 	might_sleep();
-	if (!__access_ok(to, n))
+	if (!access_ok(VERIFY_WRITE, to, n))
 		return n;
 
 	return __clear_user(to, n);
-- 
1.6.3.1

[PATCH] asm-generic: hard_irqs: handle NR_IRQS > 256 automatically

[Mike Frysinger]

If we're going to automatically set HARDIRQ_BITS for the arch, might as
well be a little bit smart about it and set it to 9 automatically if
NR_IRQS is larger than 8 bits.

Signed-off-by: Mike Frysinger <vapier@gentoo.org>
---
 include/asm-generic/hardirq.h |    6 +++++-
 1 files changed, 5 insertions(+), 1 deletions(-)

diff --git a/include/asm-generic/hardirq.h b/include/asm-generic/hardirq.h
index 3d5d2c9..d93acec 100644
--- a/include/asm-generic/hardirq.h
+++ b/include/asm-generic/hardirq.h
@@ -12,7 +12,11 @@ typedef struct {
 #include <linux/irq_cpustat.h>	/* Standard mappings for irq_cpustat_t above */
 
 #ifndef HARDIRQ_BITS
-#define HARDIRQ_BITS	8
+# if NR_IRQS > 256
+#  define HARDIRQ_BITS	9
+# else
+#  define HARDIRQ_BITS	8
+# endif
 #endif
 
 /*
-- 
1.6.3.1

Re: [PATCH] asm-generic: hard_irqs: handle NR_IRQS > 256 automatically

[H. Peter Anvin]

Mike Frysinger wrote:
> If we're going to automatically set HARDIRQ_BITS for the arch, might as
> well be a little bit smart about it and set it to 9 automatically if
> NR_IRQS is larger than 8 bits.
> 

Why would the only possible values be 8 or 9?

>  
>  #ifndef HARDIRQ_BITS
> -#define HARDIRQ_BITS	8
> +# if NR_IRQS > 256
> +#  define HARDIRQ_BITS	9
> +# else
> +#  define HARDIRQ_BITS	8
> +# endif
>  #endif
>  
>  /*


-- 
H. Peter Anvin, Intel Open Source Technology Center
I work for Intel.  I don't speak on their behalf.

Re: [PATCH] asm-generic: hard_irqs: handle NR_IRQS > 256 automatically

[Arnd Bergmann]

On Saturday 13 June 2009, H. Peter Anvin wrote:
> Mike Frysinger wrote:
> > If we're going to automatically set HARDIRQ_BITS for the arch, might as
> > well be a little bit smart about it and set it to 9 automatically if
> > NR_IRQS is larger than 8 bits.
> > 
> 
> Why would the only possible values be 8 or 9?

All architectures that define this either set it to 8 or 9, I chose
8 because it is the more common constant, but I now realized that
we also have (in include/linux/hardirq.h, last touched by Steven):

#define MAX_HARDIRQ_BITS 10
#ifndef HARDIRQ_BITS
# define HARDIRQ_BITS   MAX_HARDIRQ_BITS
#endif
#if HARDIRQ_BITS > MAX_HARDIRQ_BITS
#error HARDIRQ_BITS too high!
#endif

Not sure why we even need to make this overridable from the architecture,
10 still seems like a reasonable default that should always work.

I'd suggest we either drop the definition for HARDIRQ_BITS from
asm-generic/hardirq.h, or we use

 #ifndef HARDIRQ_BITS
-#define HARDIRQ_BITS   8
+# if NR_IRQS > 255
+#  define HARDIRQ_BITS 9
+# elif NR_IRQS > 511
+#  define HARDIRQ_BITS 10
+# elif NR_IRQS > 1023
+#  warning too many interrupts for HARDIRQ_BITS
+# endif
 #endif

	Arnd <><

Re: [PATCH] asm-generic: hard_irqs: handle NR_IRQS > 256 automatically

[Mike Frysinger]

On Sat, Jun 13, 2009 at 17:18, Arnd Bergmann wrote:
> On Saturday 13 June 2009, H. Peter Anvin wrote:
>> Mike Frysinger wrote:
>> > If we're going to automatically set HARDIRQ_BITS for the arch, might as
>> > well be a little bit smart about it and set it to 9 automatically if
>> > NR_IRQS is larger than 8 bits.
>> >
>>
>> Why would the only possible values be 8 or 9?
>
> All architectures that define this either set it to 8 or 9, I chose
> 8 because it is the more common constant

right, i was going by what was in use -- there are no requirements
here, just defaults

> but I now realized that
> we also have (in include/linux/hardirq.h, last touched by Steven):
>
> #define MAX_HARDIRQ_BITS 10
> #ifndef HARDIRQ_BITS
> # define HARDIRQ_BITS   MAX_HARDIRQ_BITS
> #endif
> #if HARDIRQ_BITS > MAX_HARDIRQ_BITS
> #error HARDIRQ_BITS too high!
> #endif
>
> Not sure why we even need to make this overridable from the architecture,
> 10 still seems like a reasonable default that should always work.
>
> I'd suggest we either drop the definition for HARDIRQ_BITS from
> asm-generic/hardirq.h, or we use
>
>  #ifndef HARDIRQ_BITS
> -#define HARDIRQ_BITS   8
> +# if NR_IRQS > 255
> +#  define HARDIRQ_BITS 9
> +# elif NR_IRQS > 511
> +#  define HARDIRQ_BITS 10
> +# elif NR_IRQS > 1023
> +#  warning too many interrupts for HARDIRQ_BITS
> +# endif
>  #endif

is there any downsides to using a "too large" value ?  i.e. if my
system has less than 256, does it make any difference at all if it's
set to 10 ?
-mike

[PATCH] asm-generic: drop HARDIRQ_BITS definition from hardirq.h

[Arnd Bergmann]

linux/hardirq.h contains a fallback for HARDIRQ_BITS to 10
if it's not defined, so it is pointless to define a default
of 8 in asm/hardirq.h. There does not seem to be a good
reason why an architecture would want to limit the number
of hardirqs this way.

Reported-by: Mike Frysinger <vapier@gentoo.org>
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 include/asm-generic/hardirq.h |   13 -------------
 1 files changed, 0 insertions(+), 13 deletions(-)
On Sunday 14 June 2009, Mike Frysinger wrote:

Mike Frysinger wrote:
> is there any downsides to using a "too large" value ?  i.e. if my
> system has less than 256, does it make any difference at all if it's
> set to 10 ?
> -mike

None that I know of. I'm queuing this patch in my asm-generic tree now,
unless Steven or someone else has a better idea.

	Arnd <><

diff --git a/include/asm-generic/hardirq.h b/include/asm-generic/hardirq.h
index 3d5d2c9..23bb4da 100644
--- a/include/asm-generic/hardirq.h
+++ b/include/asm-generic/hardirq.h
@@ -11,19 +11,6 @@ typedef struct {
 
 #include <linux/irq_cpustat.h>	/* Standard mappings for irq_cpustat_t above */
 
-#ifndef HARDIRQ_BITS
-#define HARDIRQ_BITS	8
-#endif
-
-/*
- * The hardirq mask has to be large enough to have
- * space for potentially all IRQ sources in the system
- * nesting on a single CPU:
- */
-#if (1 << HARDIRQ_BITS) < NR_IRQS
-# error HARDIRQ_BITS is too low!
-#endif
-
 #ifndef ack_bad_irq
 static inline void ack_bad_irq(unsigned int irq)
 {
-- 
1.6.3.1

Re: [PATCH] asm-generic: drop HARDIRQ_BITS definition from hardirq.h

[Steven Rostedt]

On Sun, 2009-06-14 at 22:43 +0200, Arnd Bergmann wrote:
> linux/hardirq.h contains a fallback for HARDIRQ_BITS to 10
> if it's not defined, so it is pointless to define a default
> of 8 in asm/hardirq.h. There does not seem to be a good
> reason why an architecture would want to limit the number
> of hardirqs this way.
> 
> Reported-by: Mike Frysinger <vapier@gentoo.org>
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
>  include/asm-generic/hardirq.h |   13 -------------
>  1 files changed, 0 insertions(+), 13 deletions(-)
> On Sunday 14 June 2009, Mike Frysinger wrote:
> 
> Mike Frysinger wrote:
> > is there any downsides to using a "too large" value ?  i.e. if my
> > system has less than 256, does it make any difference at all if it's
> > set to 10 ?
> > -mike
> 
> None that I know of. I'm queuing this patch in my asm-generic tree now,
> unless Steven or someone else has a better idea.

ARGH!!! I guess the best patch would be to comment this better. That
"HARDIRQ_BITS" has nothing to do with the number of interrupts a machine
may have. It is the number of nested interrupts a machine may do.

If you plan on having more than 256 interrupts nesting, I suggest you
need to fix the stack problems first ;-)

Please, we only have a few bits in the preempt count (on 32 bit
machines) and this is the number of bits used to record the nesting of
interrupts.

-- Steve


> 
> 	Arnd <><
> 
> diff --git a/include/asm-generic/hardirq.h b/include/asm-generic/hardirq.h
> index 3d5d2c9..23bb4da 100644
> --- a/include/asm-generic/hardirq.h
> +++ b/include/asm-generic/hardirq.h
> @@ -11,19 +11,6 @@ typedef struct {
>  
>  #include <linux/irq_cpustat.h>	/* Standard mappings for irq_cpustat_t above */
>  
> -#ifndef HARDIRQ_BITS
> -#define HARDIRQ_BITS	8
> -#endif
> -
> -/*
> - * The hardirq mask has to be large enough to have
> - * space for potentially all IRQ sources in the system
> - * nesting on a single CPU:
> - */
> -#if (1 << HARDIRQ_BITS) < NR_IRQS
> -# error HARDIRQ_BITS is too low!
> -#endif
> -
>  #ifndef ack_bad_irq
>  static inline void ack_bad_irq(unsigned int irq)
>  {

Re: [PATCH] asm-generic: drop HARDIRQ_BITS definition from hardirq.h

[Mike Frysinger]

On Sun, Jun 14, 2009 at 16:43, Arnd Bergmann wrote:
> linux/hardirq.h contains a fallback for HARDIRQ_BITS to 10
> if it's not defined, so it is pointless to define a default
> of 8 in asm/hardirq.h. There does not seem to be a good
> reason why an architecture would want to limit the number
> of hardirqs this way.
>
> Reported-by: Mike Frysinger <vapier@gentoo.org>
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
>  include/asm-generic/hardirq.h |   13 -------------
>  1 files changed, 0 insertions(+), 13 deletions(-)
> On Sunday 14 June 2009, Mike Frysinger wrote:
>
> Mike Frysinger wrote:
>> is there any downsides to using a "too large" value ?  i.e. if my
>> system has less than 256, does it make any difference at all if it's
>> set to 10 ?
>
> None that I know of. I'm queuing this patch in my asm-generic tree now,
> unless Steven or someone else has a better idea.

based on Steven's comments, this patch makes sense to me
Acked-by: Mike Frysinger <vapier@gentoo.org>
-mike

Re: [PATCH] asm-generic: drop HARDIRQ_BITS definition from hardirq.h

[Steven Rostedt]

[ Note: I prefer email to rostedt@goodmis.org over srostedt@redhat.com,
  just because I don't read my RH email as often. ]

On Sun, Jun 14, 2009 at 10:43:31PM +0200, Arnd Bergmann wrote:
> linux/hardirq.h contains a fallback for HARDIRQ_BITS to 10
> if it's not defined, so it is pointless to define a default
> of 8 in asm/hardirq.h. There does not seem to be a good
> reason why an architecture would want to limit the number
> of hardirqs this way.

After reading the thread again (from the beginning) I have a better idea
of what you are talking about. But the above description is misleading.

The HARDIRQ_BITS has nothing to do with the number of IRQS, it has to
do with the number of nested irqs allowed. Heck, 8 should be goog enough.

But the only thing I object to about this patch is that discription.

> 
> Reported-by: Mike Frysinger <vapier@gentoo.org>
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
>  include/asm-generic/hardirq.h |   13 -------------
>  1 files changed, 0 insertions(+), 13 deletions(-)
> On Sunday 14 June 2009, Mike Frysinger wrote:
> 
> Mike Frysinger wrote:
> > is there any downsides to using a "too large" value ?  i.e. if my
> > system has less than 256, does it make any difference at all if it's
> > set to 10 ?
> > -mike
> 
> None that I know of. I'm queuing this patch in my asm-generic tree now,
> unless Steven or someone else has a better idea.
> 
> 	Arnd <><
> 
> diff --git a/include/asm-generic/hardirq.h b/include/asm-generic/hardirq.h
> index 3d5d2c9..23bb4da 100644
> --- a/include/asm-generic/hardirq.h
> +++ b/include/asm-generic/hardirq.h
> @@ -11,19 +11,6 @@ typedef struct {
>  
>  #include <linux/irq_cpustat.h>	/* Standard mappings for irq_cpustat_t above */
>  
> -#ifndef HARDIRQ_BITS
> -#define HARDIRQ_BITS	8
> -#endif
> -
> -/*
> - * The hardirq mask has to be large enough to have
> - * space for potentially all IRQ sources in the system
> - * nesting on a single CPU:
> - */
> -#if (1 << HARDIRQ_BITS) < NR_IRQS
> -# error HARDIRQ_BITS is too low!
> -#endif

For the content of the patch:

 Acked-by: Steven Rostedt <rostedt@goodmis.org>

Because it too is misleading.

The only reason I did not remove all arch defined HARDIQR_BITS and just default
it to 10 or 8, is because one of the archs had that hardcoded in the asm, and
I did not want to break it.

-- Steve

> -
>  #ifndef ack_bad_irq
>  static inline void ack_bad_irq(unsigned int irq)
>  {
> -- 
> 1.6.3.1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

[PATCH v3] asm-generic: drop HARDIRQ_BITS definition from hardirq.h

[Arnd Bergmann]

Architechtures normally don't need to set a HARDIRQ_BITS
unless they have hardcoded a specific value in assembly.
This drops the definition from asm-generic/hardirq.h, which
results in linux/hardirq.h setting its default of 10.

Both the old default of 8 and the linux/hardirq.h default
of 10 are sufficient because they only limit the number
of nested hardirqs, and we normally run out of stack space
much earlier than exceeding 256 or even 1024 nested interrupts.

Reported-by: Mike Frysinger <vapier@gentoo.org>
Acked-by: Steven Rostedt <rostedt@goodmis.org>
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 include/asm-generic/hardirq.h |   13 -------------
 1 files changed, 0 insertions(+), 13 deletions(-)

---

v3: Updated the description.
v2: removed HARDIRQ_BITS definition
v1: original patch from Mike

diff --git a/include/asm-generic/hardirq.h b/include/asm-generic/hardirq.h
index 3d5d2c9..23bb4da 100644
--- a/include/asm-generic/hardirq.h
+++ b/include/asm-generic/hardirq.h
@@ -11,19 +11,6 @@ typedef struct {
 
 #include <linux/irq_cpustat.h>	/* Standard mappings for irq_cpustat_t above */
 
-#ifndef HARDIRQ_BITS
-#define HARDIRQ_BITS	8
-#endif
-
-/*
- * The hardirq mask has to be large enough to have
- * space for potentially all IRQ sources in the system
- * nesting on a single CPU:
- */
-#if (1 << HARDIRQ_BITS) < NR_IRQS
-# error HARDIRQ_BITS is too low!
-#endif
-
 #ifndef ack_bad_irq
 static inline void ack_bad_irq(unsigned int irq)
 {
-- 
1.6.3.1

Re: [PATCH] asm-generic: uaccess: fix up local access_ok() usage

[Mike Frysinger]

On Sat, Jun 13, 2009 at 10:30, Mike Frysinger wrote:
> There's no reason that I can see to use the short __access_ok() form
> directly when the access_ok() is clearer in intent and for more people,
> expands to the same C code (i.e. always specify the first field -- access
> type).  Not all no-mmu systems lack memory protection, so the read/write
> could feasibly be checked.

hmm, and it also fixes the crazy amount of warnings you get otherwise
because the access_ok() expansion includes an unsigned long cast:
  CC [M]  fs/smbfs/request.o
In file included from
/usr/local/src/linux/linux-2.6/arch/blackfin/include/asm/uaccess.h:17,
                 from include/net/checksum.h:26,
                 from include/linux/skbuff.h:28,
                 from include/linux/ip.h:109,
                 from include/net/ip.h:27,
                 from fs/smbfs/smbiod.c:23:
include/asm-generic/uaccess.h: In function ‘copy_from_user’:
include/asm-generic/uaccess.h:247: warning: passing argument 1 of
‘_access_ok’ makes integer from pointer without a cast
include/asm-generic/uaccess.h: In function ‘copy_to_user’:
include/asm-generic/uaccess.h:257: warning: passing argument 1 of
‘_access_ok’ makes integer from pointer without a cast
include/asm-generic/uaccess.h: In function ‘strncpy_from_user’:
include/asm-generic/uaccess.h:281: warning: passing argument 1 of
‘_access_ok’ makes integer from pointer without a cast
include/asm-generic/uaccess.h: In function ‘clear_user’:
include/asm-generic/uaccess.h:319: warning: passing argument 1 of
‘_access_ok’ makes integer from pointer without a cast
-mike

Re: [PATCH] asm-generic: uaccess: fix up local access_ok() usage

[Arnd Bergmann]

On Saturday 13 June 2009, Mike Frysinger wrote:
> There's no reason that I can see to use the short __access_ok() form
> directly when the access_ok() is clearer in intent and for more people,
> expands to the same C code (i.e. always specify the first field -- access
> type).  Not all no-mmu systems lack memory protection, so the read/write
> could feasibly be checked.

Ah, I didn't consider this. I checked all the architectures and could not
find a case where access_ok actually evaluates the the first argument, so
I chose the slightly terser variant. I also don't let you override
access_ok() at this moment, which means that you don't have a choice
to use the generic uaccess.h and still differentiate between read and
write accesses.

What I really got wrong was the prototype for __access_ok(), as you
showed in your follow-up. I only tested this with the microblaze
patch that overrides __access_ok() with an architecture specific
version that gets this part right.

Would this simpler patch help you as well?

--- a/include/asm-generic/uaccess.h
+++ b/include/asm-generic/uaccess.h
@@ -37,14 +37,14 @@ static inline void set_fs(mm_segment_t fs)
 #define VERIFY_READ	0
 #define VERIFY_WRITE	1
 
-#define access_ok(type, addr, size) __access_ok((unsigned long)(addr),(size))
+#define access_ok(type, addr, size) __access_ok((addr), (size))
 
 /*
  * The architecture should really override this if possible, at least
  * doing a check on the get_fs()
  */
 #ifndef __access_ok
-static inline int __access_ok(unsigned long addr, unsigned long size)
+static inline int __access_ok(void __user *ptr, unsigned long size)
 {
 	return 1;
 }

It may not be clearer in intent, but it's what the majority (by a small
margin) of architecture do anyway.

> Also, the strnlen_user() function was missing a access_ok() check on the
> pointer given.  We've had cases on Blackfin systems where test cases
> caused kernel crashes here because userspace passed up a NULL/-1 pointer
> and the kernel gladly attempted to run strlen() on it.

Right, well spotted. I'll take this fix as a separate patch, ok?

	Arnd <><

Re: [PATCH] asm-generic: uaccess: fix up local access_ok() usage

[Mike Frysinger]

On Sat, Jun 13, 2009 at 16:53, Arnd Bergmann wrote:
> On Saturday 13 June 2009, Mike Frysinger wrote:
>> There's no reason that I can see to use the short __access_ok() form
>> directly when the access_ok() is clearer in intent and for more people,
>> expands to the same C code (i.e. always specify the first field -- access
>> type).  Not all no-mmu systems lack memory protection, so the read/write
>> could feasibly be checked.
>
> Ah, I didn't consider this. I checked all the architectures and could not
> find a case where access_ok actually evaluates the the first argument, so
> I chose the slightly terser variant. I also don't let you override
> access_ok() at this moment, which means that you don't have a choice
> to use the generic uaccess.h and still differentiate between read and
> write accesses.

well, if you dont mind a bit of cruft, you can undef it ;)
#include <asm-generic/uaccess.h>
#undef access_ok

the Blackfin port does have hardware memory protection (MPU) and it
does handle r/w/x bits, but we havent merged this into access_ok yet,
just the vma lists

> What I really got wrong was the prototype for __access_ok(), as you
> showed in your follow-up. I only tested this with the microblaze
> patch that overrides __access_ok() with an architecture specific
> version that gets this part right.

yeah, that looks good, but i'd still like the __access_ok -> access_ok

>> Also, the strnlen_user() function was missing a access_ok() check on the
>> pointer given.  We've had cases on Blackfin systems where test cases
>> caused kernel crashes here because userspace passed up a NULL/-1 pointer
>> and the kernel gladly attempted to run strlen() on it.
>
> Right, well spotted. I'll take this fix as a separate patch, ok?

np
-mike

Re: [PATCH] asm-generic: uaccess: fix up local access_ok() usage

[Arnd Bergmann]

On Sunday 14 June 2009, Mike Frysinger wrote:
> well, if you dont mind a bit of cruft, you can undef it ;)
> #include <asm-generic/uaccess.h>
> #undef access_ok

That will only work for the users outside of uaccess.h, so it doesn't
solve this problem.

> the Blackfin port does have hardware memory protection (MPU) and it
> does handle r/w/x bits, but we havent merged this into access_ok yet,
> just the vma lists

Hmm, if the hardware can catch memory protection errors, why would
you want to check them again in access_ok()? Are the checks disabled
in kernel mode? Most implementations of access_ok only check if the
address is a kernel or user pointer, because the kernel can access
both on most architectures, and the MMU only protects you from
passing invalid pointers, not valid kernel pointers.

> > What I really got wrong was the prototype for __access_ok(), as you
> > showed in your follow-up. I only tested this with the microblaze
> > patch that overrides __access_ok() with an architecture specific
> > version that gets this part right.
> 
> yeah, that looks good, but i'd still like the __access_ok -> access_ok

Ok, no problem. I can take that change as well, don't care much either way.

	Arnd <><

Re: [PATCH] asm-generic: uaccess: fix up local access_ok() usage

[Mike Frysinger]

On Sun, Jun 14, 2009 at 06:10, Arnd Bergmann wrote:
> On Sunday 14 June 2009, Mike Frysinger wrote:
>> well, if you dont mind a bit of cruft, you can undef it ;)
>> #include <asm-generic/uaccess.h>
>> #undef access_ok
>
> That will only work for the users outside of uaccess.h, so it doesn't
> solve this problem.

hmm, yeah, so that's no good

>> the Blackfin port does have hardware memory protection (MPU) and it
>> does handle r/w/x bits, but we havent merged this into access_ok yet,
>> just the vma lists
>
> Hmm, if the hardware can catch memory protection errors, why would
> you want to check them again in access_ok()? Are the checks disabled
> in kernel mode? Most implementations of access_ok only check if the
> address is a kernel or user pointer, because the kernel can access
> both on most architectures, and the MMU only protects you from
> passing invalid pointers, not valid kernel pointers.

in the Blackfin implementation, a protection violation is an
exception, exceptions cannot be nested, there is no prioritization
between exceptions, and a double exception is (hardware)
unrecoverable.  so we need to catch pointers given to us from
userspace.  if the kernel attempted to utilize that bad pointer, that
is an exception in supervisor mode which is (software) unrecoverable
-- our exception handler detects this and forces the system to panic.

>> > What I really got wrong was the prototype for __access_ok(), as you
>> > showed in your follow-up. I only tested this with the microblaze
>> > patch that overrides __access_ok() with an architecture specific
>> > version that gets this part right.
>>
>> yeah, that looks good, but i'd still like the __access_ok -> access_ok
>
> Ok, no problem. I can take that change as well, don't care much either way.

the patchset i posted obsoletes this one patch.
-mike

Re: [PATCH] asm-generic: uaccess: fix up local access_ok() usage

[Arnd Bergmann]

On Sunday 14 June 2009, Mike Frysinger wrote:
> in the Blackfin implementation, a protection violation is an
> exception, exceptions cannot be nested, there is no prioritization
> between exceptions, and a double exception is (hardware)
> unrecoverable.  so we need to catch pointers given to us from
> userspace.  if the kernel attempted to utilize that bad pointer, that
> is an exception in supervisor mode which is (software) unrecoverable
> -- our exception handler detects this and forces the system to panic.

Ok, I see. In that case it's obviously better to to include your patch 3/4.

	Arnd <><