IS_ERR Threshold Value

IS_ERR Threshold Value

[Erik Frederiksen]

from include/asm-mips/errno.h
#define EDQUOT      1133    /* Quota exceeded */

I noticed that the errno value for EDQUOT on MIPS is considerably larger
than all others.  This can lead to a situation where functions using
ERR_PTR() to return error codes in pointers cannot return this error
code without IS_ERR() thinking that the pointer is valid.  In my case,
it caused an alignment exception in the XFS open call when quota has
been exceeded in the linux-mips 2.6.14 kernel.  I think that the XFS
code has changed enough that this bug isn't in newer versions, though I
haven't done a thorough investigation.  

I've supplied a patch that addresses this situation by changing the
threshold used by IS_ERR if EMAXERRNO is defined and greater than 1000. 
Perhaps permanently raising the threshold value to something >1133 is
sufficient.

Looking forward to your feedback.  

Erik Frederiksen
Firmware Design Engineer Co-op
PMC-Sierra Saskatoon




diff -Nau [ab]/include/linux/err.h
--- a/include/linux/err.h       2005-10-30 13:14:22.000000000 -0600
+++ b/include/linux/err.h       2006-06-28 10:38:43.000000000 -0600
@@ -12,8 +12,23 @@
  *
  * This should be a per-architecture thing, to allow different
  * error and pointer decisions.
+ *
+ * Updated by Erik Frederiksen (erik_frederiksen@pmc-sierra.com)
+ * errno values on MIPS go up to 1133 for EDQUOT.  The threshold
+ * is adjusted so that returning large errnos in a pointer
+ * does not result in a valid pointer according to IS_ERR.
  */
-#define IS_ERR_VALUE(x) unlikely((x) > (unsigned long)-1000L)
+
+#define ERR_PTR_THRESHOLD 1000
+#define IS_ERR_VALUE(x) \
+       unlikely((x) > (unsigned long)-(long)ERR_PTR_THRESHOLD )
+#ifdef EMAXERRNO
+# if EMAXERRNO >= ERR_PTR_THRESHOLD
+#  undef IS_ERR_VALUE
+#  define IS_ERR_VALUE(x) \
+       unlikely((x) >= (unsigned long)-(long)EMAXERRNO )
+# endif
+#endif
  
 static inline void *ERR_PTR(long error)
 {

Re: IS_ERR Threshold Value

[Randy.Dunlap]

On Wed, 28 Jun 2006 14:57:07 -0600 Erik Frederiksen wrote:

> 
> from include/asm-mips/errno.h
> #define EDQUOT      1133    /* Quota exceeded */
> 
> I noticed that the errno value for EDQUOT on MIPS is considerably larger
> than all others.  This can lead to a situation where functions using
> ERR_PTR() to return error codes in pointers cannot return this error
> code without IS_ERR() thinking that the pointer is valid.  In my case,
> it caused an alignment exception in the XFS open call when quota has
> been exceeded in the linux-mips 2.6.14 kernel.  I think that the XFS
> code has changed enough that this bug isn't in newer versions, though I
> haven't done a thorough investigation.  
> 
> I've supplied a patch that addresses this situation by changing the
> threshold used by IS_ERR if EMAXERRNO is defined and greater than 1000. 
> Perhaps permanently raising the threshold value to something >1133 is
> sufficient.
> 
> Looking forward to your feedback.  
> 
> Erik Frederiksen
> Firmware Design Engineer Co-op
> PMC-Sierra Saskatoon

Hi,
Peter Anvin mentioned just a few days ago that this threshold value
should be 4095 for all arches.  I think we need to get that patch
done & submitted to Andrew for -mm.


> diff -Nau [ab]/include/linux/err.h
> --- a/include/linux/err.h       2005-10-30 13:14:22.000000000 -0600
> +++ b/include/linux/err.h       2006-06-28 10:38:43.000000000 -0600
> @@ -12,8 +12,23 @@
>   *
>   * This should be a per-architecture thing, to allow different
>   * error and pointer decisions.
> + *
> + * Updated by Erik Frederiksen (erik_frederiksen@pmc-sierra.com)
> + * errno values on MIPS go up to 1133 for EDQUOT.  The threshold
> + * is adjusted so that returning large errnos in a pointer
> + * does not result in a valid pointer according to IS_ERR.
>   */
> -#define IS_ERR_VALUE(x) unlikely((x) > (unsigned long)-1000L)
> +
> +#define ERR_PTR_THRESHOLD 1000
> +#define IS_ERR_VALUE(x) \
> +       unlikely((x) > (unsigned long)-(long)ERR_PTR_THRESHOLD )
> +#ifdef EMAXERRNO
> +# if EMAXERRNO >= ERR_PTR_THRESHOLD
> +#  undef IS_ERR_VALUE
> +#  define IS_ERR_VALUE(x) \
> +       unlikely((x) >= (unsigned long)-(long)EMAXERRNO )
> +# endif
> +#endif
>   
>  static inline void *ERR_PTR(long error)
>  {
> 
> 
> -

---
~Randy

Re: IS_ERR Threshold Value

[H. Peter Anvin]

Randy.Dunlap wrote:
> On Wed, 28 Jun 2006 14:57:07 -0600 Erik Frederiksen wrote:
> 
>> from include/asm-mips/errno.h
>> #define EDQUOT      1133    /* Quota exceeded */
>>
>> I noticed that the errno value for EDQUOT on MIPS is considerably larger
>> than all others.  This can lead to a situation where functions using
>> ERR_PTR() to return error codes in pointers cannot return this error
>> code without IS_ERR() thinking that the pointer is valid.  In my case,
>> it caused an alignment exception in the XFS open call when quota has
>> been exceeded in the linux-mips 2.6.14 kernel.  I think that the XFS
>> code has changed enough that this bug isn't in newer versions, though I
>> haven't done a thorough investigation.  
>>
>> I've supplied a patch that addresses this situation by changing the
>> threshold used by IS_ERR if EMAXERRNO is defined and greater than 1000. 
>> Perhaps permanently raising the threshold value to something >1133 is
>> sufficient.
>>
>> Looking forward to your feedback.  
>>
>> Erik Frederiksen
>> Firmware Design Engineer Co-op
>> PMC-Sierra Saskatoon
> 
> Hi,
> Peter Anvin mentioned just a few days ago that this threshold value
> should be 4095 for all arches.  I think we need to get that patch
> done & submitted to Andrew for -mm.
> 

Indeed.

	-hpa

Re: IS_ERR Threshold Value

[Ralf Baechle]

On Wed, Jun 28, 2006 at 02:08:25PM -0700, Randy.Dunlap wrote:

> Hi,
> Peter Anvin mentioned just a few days ago that this threshold value
> should be 4095 for all arches.  I think we need to get that patch
> done & submitted to Andrew for -mm.

So here the patch is:

 o Raise the maximum error number in IS_ERR_VALUE to 4095.
 o Make that number available as a new constant MAX_ERRNO.

Signed-off-by: Ralf Baechle <ralf@linux-mips.org>

diff --git a/include/linux/err.h b/include/linux/err.h
index ff71d2a..cd3b367 100644
--- a/include/linux/err.h
+++ b/include/linux/err.h
@@ -13,7 +13,9 @@ #include <asm/errno.h>
  * This should be a per-architecture thing, to allow different
  * error and pointer decisions.
  */
-#define IS_ERR_VALUE(x) unlikely((x) > (unsigned long)-1000L)
+#define MAX_ERRNO	4095
+
+#define IS_ERR_VALUE(x) unlikely((x) >= (unsigned long)-MAX_ERRNO)
 
 static inline void *ERR_PTR(long error)
 {

Re: IS_ERR Threshold Value

[Randy.Dunlap]

On Thu, 29 Jun 2006 19:10:13 +0100 Ralf Baechle wrote:

> On Wed, Jun 28, 2006 at 02:08:25PM -0700, Randy.Dunlap wrote:
> 
> > Hi,
> > Peter Anvin mentioned just a few days ago that this threshold value
> > should be 4095 for all arches.  I think we need to get that patch
> > done & submitted to Andrew for -mm.
> 
> So here the patch is:
> 
>  o Raise the maximum error number in IS_ERR_VALUE to 4095.
>  o Make that number available as a new constant MAX_ERRNO.
> 
> Signed-off-by: Ralf Baechle <ralf@linux-mips.org>
> 
> diff --git a/include/linux/err.h b/include/linux/err.h
> index ff71d2a..cd3b367 100644
> --- a/include/linux/err.h
> +++ b/include/linux/err.h
> @@ -13,7 +13,9 @@ #include <asm/errno.h>
>   * This should be a per-architecture thing, to allow different
>   * error and pointer decisions.
>   */
> -#define IS_ERR_VALUE(x) unlikely((x) > (unsigned long)-1000L)
> +#define MAX_ERRNO	4095
> +
> +#define IS_ERR_VALUE(x) unlikely((x) >= (unsigned long)-MAX_ERRNO)
>  
>  static inline void *ERR_PTR(long error)
>  {

Are changes also needed in asm-*/unistd.h::syscall_return() macros?
or is syscall_return() just not used?

e.g.,
arm26 uses -125 to detect error
arm uses -129 to detect error
frv uses -4095 to detect error
i386 uses -129
h8300, m32r, s390, sh64, v850 use -125
m68k[nommu] uses -125
sh uses -124
x86_64 uses -127


---
~Randy

Re: IS_ERR Threshold Value

[H. Peter Anvin]

Randy.Dunlap wrote:

> 
> Are changes also needed in asm-*/unistd.h::syscall_return() macros?
> or is syscall_return() just not used?
> 
> e.g.,
> arm26 uses -125 to detect error
> arm uses -129 to detect error
> frv uses -4095 to detect error
> i386 uses -129
> h8300, m32r, s390, sh64, v850 use -125
> m68k[nommu] uses -125
> sh uses -124
> x86_64 uses -127
> 

... and they're pretty much all wrong (in some cases, they're actually 
less than actual errno values on that architecture!)

It pretty much works because they're not used.  They should either be 
fixed or removed.

	-hpa

Re: IS_ERR Threshold Value

[Ralf Baechle]

So MAX_ERRNO of EMAXERRNO which was also being used in assembler code.
Other architectures may have the same issue, so I propose wrapping the
C parts with #ifndef __ASSEMBLY__ to keep as happy.

Signed-off-by: Ralf Baechle <ralf@linux-mips.org>

diff --git a/include/linux/err.h b/include/linux/err.h
index cd3b367..1ab1d44 100644
--- a/include/linux/err.h
+++ b/include/linux/err.h
@@ -15,6 +15,8 @@ #include <asm/errno.h>
  */
 #define MAX_ERRNO	4095
 
+#ifndef __ASSEMBLY__
+
 #define IS_ERR_VALUE(x) unlikely((x) >= (unsigned long)-MAX_ERRNO)
 
 static inline void *ERR_PTR(long error)
@@ -32,4 +34,6 @@ static inline long IS_ERR(const void *pt
 	return IS_ERR_VALUE((unsigned long)ptr);
 }
 
+#endif
+
 #endif /* _LINUX_ERR_H */

Re: IS_ERR Threshold Value

[H. Peter Anvin]

Ralf Baechle wrote:
> So MAX_ERRNO of EMAXERRNO which was also being used in assembler code.
> Other architectures may have the same issue, so I propose wrapping the
> C parts with #ifndef __ASSEMBLY__ to keep as happy.
> 

Indeed; this should definitely be accessible to assembly code.  I'd like 
to change the hard-coded constants in klibc over time to use this.

	-hpa

Re: IS_ERR Threshold Value

[Randy.Dunlap]

On Sun, 2 Jul 2006 17:15:20 +0100 Ralf Baechle wrote:

> So MAX_ERRNO of EMAXERRNO which was also being used in assembler code.
> Other architectures may have the same issue, so I propose wrapping the
> C parts with #ifndef __ASSEMBLY__ to keep as happy.

Ack, fixes 'as' whinging for me.


> Signed-off-by: Ralf Baechle <ralf@linux-mips.org>
> 
> diff --git a/include/linux/err.h b/include/linux/err.h
> index cd3b367..1ab1d44 100644
> --- a/include/linux/err.h
> +++ b/include/linux/err.h
> @@ -15,6 +15,8 @@ #include <asm/errno.h>
>   */
>  #define MAX_ERRNO	4095
>  
> +#ifndef __ASSEMBLY__
> +
>  #define IS_ERR_VALUE(x) unlikely((x) >= (unsigned long)-MAX_ERRNO)
>  
>  static inline void *ERR_PTR(long error)
> @@ -32,4 +34,6 @@ static inline long IS_ERR(const void *pt
>  	return IS_ERR_VALUE((unsigned long)ptr);
>  }
>  
> +#endif
> +
>  #endif /* _LINUX_ERR_H */
> -


---
~Randy

[PATCH] consistently use MAX_ERRNO in __syscall_return

[Randy.Dunlap]

On Sat, 01 Jul 2006 15:23:31 -0700 H. Peter Anvin wrote:

> > Are changes also needed in asm-*/unistd.h::syscall_return() macros?
> > or is syscall_return() just not used?
> > 
> > e.g.,
> > arm26 uses -125 to detect error
> > arm uses -129 to detect error
> > frv uses -4095 to detect error
> > i386 uses -129
> > h8300, m32r, s390, sh64, v850 use -125
> > m68k[nommu] uses -125
> > sh uses -124
> > x86_64 uses -127
> > 
> 
> ... and they're pretty much all wrong (in some cases, they're actually 
> less than actual errno values on that architecture!)
> 
> It pretty much works because they're not used.  They should either be 
> fixed or removed.

From: Randy Dunlap <rdunlap@xenotime.net>

Consistently use MAX_ERRNO when checking for errors
in __syscall_return().

Signed-off-by: Randy Dunlap <rdunlap@xenotime.net>
---
 include/asm-arm/unistd.h       |    3 ++-
 include/asm-arm26/unistd.h     |    3 ++-
 include/asm-frv/unistd.h       |    3 ++-
 include/asm-h8300/unistd.h     |    6 +++---
 include/asm-i386/unistd.h      |    5 +++--
 include/asm-m32r/unistd.h      |    5 +++--
 include/asm-m68k/unistd.h      |    5 +++--
 include/asm-m68knommu/unistd.h |    5 +++--
 include/asm-s390/unistd.h      |    4 +++-
 include/asm-sh/unistd.h        |    7 +++++--
 include/asm-sh64/unistd.h      |    6 ++++--
 include/asm-v850/unistd.h      |    5 +++--
 include/asm-x86_64/unistd.h    |    5 +++--
 13 files changed, 39 insertions(+), 23 deletions(-)

--- linux-2617-g20.orig/include/asm-arm/unistd.h
+++ linux-2617-g20/include/asm-arm/unistd.h
@@ -377,6 +377,7 @@
 #endif
 
 #ifdef __KERNEL__
+#include <linux/err.h>
 #include <linux/linkage.h>
 
 #define __sys2(x) #x
@@ -396,7 +397,7 @@
 
 #define __syscall_return(type, res)					\
 do {									\
-	if ((unsigned long)(res) >= (unsigned long)(-129)) {		\
+	if ((unsigned long)(res) >= (unsigned long)(-MAX_ERRNO)) {	\
 		errno = -(res);						\
 		res = -1;						\
 	}								\
--- linux-2617-g20.orig/include/asm-arm26/unistd.h
+++ linux-2617-g20/include/asm-arm26/unistd.h
@@ -311,6 +311,7 @@
 #define __ARM_NR_usr26			(__ARM_NR_BASE+3)
 
 #ifdef __KERNEL__
+#include <linux/err.h>
 #include <linux/linkage.h>
 
 #define __sys2(x) #x
@@ -322,7 +323,7 @@
 
 #define __syscall_return(type, res)					\
 do {									\
-	if ((unsigned long)(res) >= (unsigned long)(-125)) {		\
+	if ((unsigned long)(res) >= (unsigned long)-MAX_ERRNO) {	\
 		errno = -(res);						\
 		res = -1;						\
 	}								\
--- linux-2617-g20.orig/include/asm-frv/unistd.h
+++ linux-2617-g20/include/asm-frv/unistd.h
@@ -320,6 +320,7 @@
 #ifdef __KERNEL__
 
 #define NR_syscalls 310
+#include <linux/err.h>
 
 /*
  * process the return value of a syscall, consigning it to one of two possible fates
@@ -329,7 +330,7 @@
 #define __syscall_return(type, res)					\
 do {									\
         unsigned long __sr2 = (res);					\
-	if (__builtin_expect(__sr2 >= (unsigned long)(-4095), 0)) {	\
+	if (__builtin_expect(__sr2 >= (unsigned long)(-MAX_ERRNO), 0)) { \
 		errno = (-__sr2);					\
 		__sr2 = ~0UL;						\
 	}								\
--- linux-2617-g20.orig/include/asm-h8300/unistd.h
+++ linux-2617-g20/include/asm-h8300/unistd.h
@@ -295,14 +295,14 @@
 #ifdef __KERNEL__
 
 #define NR_syscalls 289
+#include <linux/err.h>
 
-
-/* user-visible error numbers are in the range -1 - -122: see
+/* user-visible error numbers are in the range -1 - -MAX_ERRNO: see
    <asm-m68k/errno.h> */
 
 #define __syscall_return(type, res) \
 do { \
-	if ((unsigned long)(res) >= (unsigned long)(-125)) { \
+	if ((unsigned long)(res) >= (unsigned long)(-MAX_ERRNO)) { \
 	/* avoid using res which is declared to be in register d0; \
 	   errno might expand to a function call and clobber it.  */ \
 		int __err = -(res); \
--- linux-2617-g20.orig/include/asm-i386/unistd.h
+++ linux-2617-g20/include/asm-i386/unistd.h
@@ -327,14 +327,15 @@
 #ifdef __KERNEL__
 
 #define NR_syscalls 318
+#include <linux/err.h>
 
 /*
- * user-visible error numbers are in the range -1 - -128: see
+ * user-visible error numbers are in the range -1 - -MAX_ERRNO: see
  * <asm-i386/errno.h>
  */
 #define __syscall_return(type, res) \
 do { \
-	if ((unsigned long)(res) >= (unsigned long)(-(128 + 1))) { \
+	if ((unsigned long)(res) >= (unsigned long)(-MAX_ERRNO)) { \
 		errno = -(res); \
 		res = -1; \
 	} \
--- linux-2617-g20.orig/include/asm-m32r/unistd.h
+++ linux-2617-g20/include/asm-m32r/unistd.h
@@ -298,14 +298,15 @@
 #ifdef __KERNEL__
 
 #define NR_syscalls 285
+#include <linux/err.h>
 
-/* user-visible error numbers are in the range -1 - -124: see
+/* user-visible error numbers are in the range -1 - -MAX_ERRNO: see
  * <asm-m32r/errno.h>
  */
 
 #define __syscall_return(type, res) \
 do { \
-	if ((unsigned long)(res) >= (unsigned long)(-(124 + 1))) { \
+	if ((unsigned long)(res) >= (unsigned long)(-MAX_ERRNO)) { \
 	/* Avoid using "res" which is declared to be in register r0; \
 	   errno might expand to a function call and clobber it.  */ \
 		int __err = -(res); \
--- linux-2617-g20.orig/include/asm-m68k/unistd.h
+++ linux-2617-g20/include/asm-m68k/unistd.h
@@ -288,13 +288,14 @@
 #ifdef __KERNEL__
 
 #define NR_syscalls		282
+#include <linux/err.h>
 
-/* user-visible error numbers are in the range -1 - -124: see
+/* user-visible error numbers are in the range -1 - -MAX_ERRNO: see
    <asm-m68k/errno.h> */
 
 #define __syscall_return(type, res) \
 do { \
-	if ((unsigned long)(res) >= (unsigned long)(-125)) { \
+	if ((unsigned long)(res) >= (unsigned long)(-MAX_ERRNO)) { \
 	/* avoid using res which is declared to be in register d0; \
 	   errno might expand to a function call and clobber it.  */ \
 		int __err = -(res); \
--- linux-2617-g20.orig/include/asm-m68knommu/unistd.h
+++ linux-2617-g20/include/asm-m68knommu/unistd.h
@@ -289,13 +289,14 @@
 #ifdef __KERNEL__
 
 #define NR_syscalls		282
+#include <linux/err.h>
 
-/* user-visible error numbers are in the range -1 - -122: see
+/* user-visible error numbers are in the range -1 - -MAX_ERRNO: see
    <asm-m68k/errno.h> */
 
 #define __syscall_return(type, res) \
 do { \
-	if ((unsigned long)(res) >= (unsigned long)(-125)) { \
+	if ((unsigned long)(res) >= (unsigned long)(-MAX_ERRNO)) { \
 	/* avoid using res which is declared to be in register d0; \
 	   errno might expand to a function call and clobber it.  */ \
 		int __err = -(res); \
--- linux-2617-g20.orig/include/asm-s390/unistd.h
+++ linux-2617-g20/include/asm-s390/unistd.h
@@ -394,9 +394,11 @@
 
 #ifdef __KERNEL__
 
+#include <linux/err.h>
+
 #define __syscall_return(type, res)			     \
 do {							     \
-	if ((unsigned long)(res) >= (unsigned long)(-4095)) {\
+	if ((unsigned long)(res) >= (unsigned long)(-MAX_ERRNO)) { \
 		errno = -(res);				     \
 		res = -1;				     \
 	}						     \
--- linux-2617-g20.orig/include/asm-sh64/unistd.h
+++ linux-2617-g20/include/asm-sh64/unistd.h
@@ -347,8 +347,10 @@
 #ifdef __KERNEL__ 
 
 #define NR_syscalls 321
+#include <linux/err.h>
 
-/* user-visible error numbers are in the range -1 - -125: see <asm-sh64/errno.h> */
+/* user-visible error numbers are in the range -1 - -MAX_ERRNO:
+ * see <asm-sh64/errno.h> */
 
 #define __syscall_return(type, res) \
 do { \
@@ -358,7 +360,7 @@ do { \
 	**       life easier in the system call epilogue (see entry.S)      \
 	*/								    \
         register unsigned long __sr2 __asm__ ("r2") = res;		    \
-	if ((unsigned long)(res) >= (unsigned long)(-125)) { \
+	if ((unsigned long)(res) >= (unsigned long)(-MAX_ERRNO)) {	    \
 		errno = -(res);						    \
 		__sr2 = -1; 						    \
 	} \
--- linux-2617-g20.orig/include/asm-sh/unistd.h
+++ linux-2617-g20/include/asm-sh/unistd.h
@@ -306,11 +306,14 @@
 
 #ifdef __KERNEL__
 
-/* user-visible error numbers are in the range -1 - -124: see <asm-sh/errno.h> */
+#include <linux/err.h>
+
+/* user-visible error numbers are in the range -1 - -MAX_ERRNO:
+ * see <asm-sh/errno.h> */
 
 #define __syscall_return(type, res) \
 do { \
-	if ((unsigned long)(res) >= (unsigned long)(-124)) { \
+	if ((unsigned long)(res) >= (unsigned long)(-MAX_ERRNO)) { \
 	/* Avoid using "res" which is declared to be in register r0; \
 	   errno might expand to a function call and clobber it.  */ \
 		int __err = -(res); \
--- linux-2617-g20.orig/include/asm-v850/unistd.h
+++ linux-2617-g20/include/asm-v850/unistd.h
@@ -238,12 +238,13 @@
 #ifdef __KERNEL__
 
 #include <asm/clinkage.h>
+#include <linux/err.h>
 
 #define __syscall_return(type, res)					      \
   do {									      \
-	  /* user-visible error numbers are in the range -1 - -124:	      \
+	  /* user-visible error numbers are in the range -1 - -MAX_ERRNO:      \
 	     see <asm-v850/errno.h> */					      \
-	  if (__builtin_expect ((unsigned long)(res) >= (unsigned long)(-125), 0)) { \
+	  if (__builtin_expect ((unsigned long)(res) >= (unsigned long)(-MAX_ERRNO), 0)) { \
 		  errno = -(res);					      \
 		  res = -1;						      \
 	  }								      \
--- linux-2617-g20.orig/include/asm-x86_64/unistd.h
+++ linux-2617-g20/include/asm-x86_64/unistd.h
@@ -623,16 +623,17 @@ __SYSCALL(__NR_move_pages, sys_move_page
 #ifdef __KERNEL__
 
 #define __NR_syscall_max __NR_move_pages
+#include <linux/err.h>
 
 #ifndef __NO_STUBS
 
-/* user-visible error numbers are in the range -1 - -4095 */
+/* user-visible error numbers are in the range -1 - -MAX_ERRNO */
 
 #define __syscall_clobber "r11","rcx","memory" 
 
 #define __syscall_return(type, res) \
 do { \
-	if ((unsigned long)(res) >= (unsigned long)(-127)) { \
+	if ((unsigned long)(res) >= (unsigned long)(-MAX_ERRNO)) { \
 		errno = -(res); \
 		res = -1; \
 	} \


---

Re: [PATCH] consistently use MAX_ERRNO in __syscall_return

[Andrew Morton]

On Sun, 2 Jul 2006 11:27:22 -0700
"Randy.Dunlap" <rdunlap@xenotime.net> wrote:

> --- linux-2617-g20.orig/include/asm-i386/unistd.h
> +++ linux-2617-g20/include/asm-i386/unistd.h
> @@ -327,14 +327,15 @@
>  #ifdef __KERNEL__
>  
>  #define NR_syscalls 318
> +#include <linux/err.h>

include/linux/err.h: Assembler messages:
include/linux/err.h:20: Error: no such instruction: `static inline void *ERR_PTR(long error)'
include/linux/err.h:21: Error: junk at end of line, first unrecognized character is `{'
include/linux/err.h:22: Error: no such instruction: `return (void *)error'
include/linux/err.h:23: Error: junk at end of line, first unrecognized character is `}'
include/linux/err.h:25: Error: no such instruction: `static inline long PTR_ERR(const void *ptr)'
include/linux/err.h:26: Error: junk at end of line, first unrecognized character is `{'
include/linux/err.h:27: Error: no such instruction: `return (long)ptr'
include/linux/err.h:28: Error: junk at end of line, first unrecognized character is `}'
include/linux/err.h:30: Error: no such instruction: `static inline long IS_ERR(const void *ptr)'
include/linux/err.h:31: Error: junk at end of line, first unrecognized character is `{'
include/linux/err.h:32: Error: no such instruction: `return unlikely(((unsigned long)ptr)>=(unsigned long)-4095)'
include/linux/err.h:33: Error: junk at end of line, first unrecognized character is `}'
distcc[7619] ERROR: compile (null) on localhost failed
make[1]: *** [arch/i386/kernel/vsyscall-sysenter.o] Error 1
make: *** [arch/i386/kernel/vsyscall-sysenter.o] Error 2

Re: [PATCH] consistently use MAX_ERRNO in __syscall_return

[H. Peter Anvin]

Andrew Morton wrote:
> On Sun, 2 Jul 2006 11:27:22 -0700
> "Randy.Dunlap" <rdunlap@xenotime.net> wrote:
> 
>> --- linux-2617-g20.orig/include/asm-i386/unistd.h
>> +++ linux-2617-g20/include/asm-i386/unistd.h
>> @@ -327,14 +327,15 @@
>>  #ifdef __KERNEL__
>>  
>>  #define NR_syscalls 318
>> +#include <linux/err.h>
> 
> include/linux/err.h: Assembler messages:
> include/linux/err.h:20: Error: no such instruction: `static inline void *ERR_PTR(long error)'
> include/linux/err.h:21: Error: junk at end of line, first unrecognized character is `{'
> include/linux/err.h:22: Error: no such instruction: `return (void *)error'
> include/linux/err.h:23: Error: junk at end of line, first unrecognized character is `}'
> include/linux/err.h:25: Error: no such instruction: `static inline long PTR_ERR(const void *ptr)'
> include/linux/err.h:26: Error: junk at end of line, first unrecognized character is `{'
> include/linux/err.h:27: Error: no such instruction: `return (long)ptr'
> include/linux/err.h:28: Error: junk at end of line, first unrecognized character is `}'
> include/linux/err.h:30: Error: no such instruction: `static inline long IS_ERR(const void *ptr)'
> include/linux/err.h:31: Error: junk at end of line, first unrecognized character is `{'
> include/linux/err.h:32: Error: no such instruction: `return unlikely(((unsigned long)ptr)>=(unsigned long)-4095)'
> include/linux/err.h:33: Error: junk at end of line, first unrecognized character is `}'
> distcc[7619] ERROR: compile (null) on localhost failed
> make[1]: *** [arch/i386/kernel/vsyscall-sysenter.o] Error 1
> make: *** [arch/i386/kernel/vsyscall-sysenter.o] Error 2

unlikely() shouldn't be used in code exported to user space.  At least 
one architecture simply open-codes the __builtin_expect(); or we could
introduce __likely() and __unlikely() for the benefit of userspace.

	-hpa

Re: [PATCH] consistently use MAX_ERRNO in __syscall_return

[Randy.Dunlap]

On Mon, 03 Jul 2006 08:03:29 -0700 H. Peter Anvin wrote:

> Andrew Morton wrote:
> > On Sun, 2 Jul 2006 11:27:22 -0700
> > "Randy.Dunlap" <rdunlap@xenotime.net> wrote:
> > 
> >> --- linux-2617-g20.orig/include/asm-i386/unistd.h
> >> +++ linux-2617-g20/include/asm-i386/unistd.h
> >> @@ -327,14 +327,15 @@
> >>  #ifdef __KERNEL__
> >>  
> >>  #define NR_syscalls 318
> >> +#include <linux/err.h>
> > 
> > include/linux/err.h: Assembler messages:
> > include/linux/err.h:20: Error: no such instruction: `static inline void *ERR_PTR(long error)'
> > include/linux/err.h:21: Error: junk at end of line, first unrecognized character is `{'
> > include/linux/err.h:22: Error: no such instruction: `return (void *)error'
> > include/linux/err.h:23: Error: junk at end of line, first unrecognized character is `}'
> > include/linux/err.h:25: Error: no such instruction: `static inline long PTR_ERR(const void *ptr)'
> > include/linux/err.h:26: Error: junk at end of line, first unrecognized character is `{'
> > include/linux/err.h:27: Error: no such instruction: `return (long)ptr'
> > include/linux/err.h:28: Error: junk at end of line, first unrecognized character is `}'
> > include/linux/err.h:30: Error: no such instruction: `static inline long IS_ERR(const void *ptr)'
> > include/linux/err.h:31: Error: junk at end of line, first unrecognized character is `{'
> > include/linux/err.h:32: Error: no such instruction: `return unlikely(((unsigned long)ptr)>=(unsigned long)-4095)'
> > include/linux/err.h:33: Error: junk at end of line, first unrecognized character is `}'
> > distcc[7619] ERROR: compile (null) on localhost failed
> > make[1]: *** [arch/i386/kernel/vsyscall-sysenter.o] Error 1
> > make: *** [arch/i386/kernel/vsyscall-sysenter.o] Error 2

Built for me on i386 and x86_64.

> unlikely() shouldn't be used in code exported to user space.  At least 
> one architecture simply open-codes the __builtin_expect(); or we could
> introduce __likely() and __unlikely() for the benefit of userspace.

How did you determine that this had something to do with
userspace?

---
~Randy

Re: [PATCH] consistently use MAX_ERRNO in __syscall_return

[H. Peter Anvin]

Randy.Dunlap wrote:
>>>>  
>>>>  #define NR_syscalls 318
>>>> +#include <linux/err.h>
>>> include/linux/err.h: Assembler messages:
>>> include/linux/err.h:20: Error: no such instruction: `static inline void *ERR_PTR(long error)'
>>> include/linux/err.h:21: Error: junk at end of line, first unrecognized character is `{'
>>> include/linux/err.h:22: Error: no such instruction: `return (void *)error'
>>> include/linux/err.h:23: Error: junk at end of line, first unrecognized character is `}'
>>> include/linux/err.h:25: Error: no such instruction: `static inline long PTR_ERR(const void *ptr)'
>>> include/linux/err.h:26: Error: junk at end of line, first unrecognized character is `{'
>>> include/linux/err.h:27: Error: no such instruction: `return (long)ptr'
>>> include/linux/err.h:28: Error: junk at end of line, first unrecognized character is `}'
>>> include/linux/err.h:30: Error: no such instruction: `static inline long IS_ERR(const void *ptr)'
>>> include/linux/err.h:31: Error: junk at end of line, first unrecognized character is `{'
>>> include/linux/err.h:32: Error: no such instruction: `return unlikely(((unsigned long)ptr)>=(unsigned long)-4095)'
>>> include/linux/err.h:33: Error: junk at end of line, first unrecognized character is `}'
>>> distcc[7619] ERROR: compile (null) on localhost failed
>>> make[1]: *** [arch/i386/kernel/vsyscall-sysenter.o] Error 1
>>> make: *** [arch/i386/kernel/vsyscall-sysenter.o] Error 2
> 
> Built for me on i386 and x86_64.
> 
>> unlikely() shouldn't be used in code exported to user space.  At least 
>> one architecture simply open-codes the __builtin_expect(); or we could
>> introduce __likely() and __unlikely() for the benefit of userspace.
> 
> How did you determine that this had something to do with
> userspace?
> 

The vsyscalls are userspace code.  Doesn't mean they operate by 
userspace naming rules, of course, but still...

	-hpa

Re: IS_ERR Threshold Value

[Andreas Mohr]

Hi,

On Thu, Jun 29, 2006 at 07:10:13PM +0100, Ralf Baechle wrote:
> On Wed, Jun 28, 2006 at 02:08:25PM -0700, Randy.Dunlap wrote:
> 
> > Hi,
> > Peter Anvin mentioned just a few days ago that this threshold value
> > should be 4095 for all arches.  I think we need to get that patch
> > done & submitted to Andrew for -mm.
> 
> So here the patch is:
> 
>  o Raise the maximum error number in IS_ERR_VALUE to 4095.
>  o Make that number available as a new constant MAX_ERRNO.
> 
> Signed-off-by: Ralf Baechle <ralf@linux-mips.org>
> 
> diff --git a/include/linux/err.h b/include/linux/err.h
> index ff71d2a..cd3b367 100644
> --- a/include/linux/err.h
> +++ b/include/linux/err.h
> @@ -13,7 +13,9 @@ #include <asm/errno.h>
>   * This should be a per-architecture thing, to allow different
>   * error and pointer decisions.
>   */
> -#define IS_ERR_VALUE(x) unlikely((x) > (unsigned long)-1000L)
> +#define MAX_ERRNO	4095
> +
> +#define IS_ERR_VALUE(x) unlikely((x) >= (unsigned long)-MAX_ERRNO)
>  
>  static inline void *ERR_PTR(long error)
>  {

# cat /proc/likely_prof |grep -v "^ "

Likely Profiling Results
[+- ] Type | # True | # False | Function:Filename@Line
+unlikely |     1872|        0  IS_ERR()@:include/linux/err.h@34
-likely   |       88|     1033  remove_from_swapped_list()@:mm/swap_prefetch.c@140
-likely   |      311|     2679  tcp_transmit_skb()@:net/ipv4/tcp_output.c@372
-likely   |        0|   175176  __switch_to_xtra()@:arch/i386/kernel/process.c@569
+unlikely |       20|       19  remap_pte_range()@:mm/memory.c@1282
+unlikely |        9|        0  signal_pending()@:include/linux/sched.h@1543
+unlikely |        9|        0  signal_pending()@:include/linux/sched.h@1543
-likely   |    15591|    44490  generic_file_buffered_write()@:mm/filemap.c@2350
+unlikely |       45|        0  ll_front_merge_fn()@:block/ll_rw_blk.c@1505
+unlikely |        1|        0  simple_pin_fs()@:fs/libfs.c@419
+unlikely |      216|        0  inotify_find_update_watch()@:fs/inotify.c@597
+unlikely |     1820|        0  get_locked_pte()@:mm/memory.c@1194
+unlikely |    82529|        0  ll_back_merge_fn()@:block/ll_rw_blk.c@1467
+unlikely |   416831|   116869  need_resched()@:include/linux/sched.h@1548
+unlikely | 28334365| 23531065  __lock_acquire()@:kernel/lockdep.c@1994

# uname -a
Linux andi 2.6.19-rc1-mm1 #1 Thu Oct 19 23:55:19 CEST 2006 i686 GNU/Linux

Athlon 1200, Debian


Hmmmmmm...
(I strongly believe I've seen this particular unlikely place trigger sometime
before already!)


Could this be because

+#define IS_ERR_VALUE(x) unlikely((x) >= (unsigned long)-MAX_ERRNO)

should be made to be
#define IS_ERR_VALUE(x) unlikely((unsigned long)(x) >= (unsigned long)-MAX_ERRNO)
similar to what some other code in this discussion thread does?
(don't ask me which code is actually invoking this macro; obviously this
might be a problem due to unexpected (non-)failure which seems to be happening
here)


And some other (un)likely results here seem fishy as well, no matter
what kind of load I throw at my box... (pondering sending some patches
for the worst excesses). Any comments about those results above?

My results seem to strongly indicate that nobody else is doing unlikely
profiling??

Andreas Mohr

Re: IS_ERR Threshold Value

[Jan Engelhardt]

>Subject: Re: IS_ERR Threshold Value
>
>> > Peter Anvin mentioned just a few days ago that this threshold value
>> > should be 4095 for all arches.  I think we need to get that patch
>> > done & submitted to Andrew for -mm.
>> 
>> So here the patch is:
>> 
>>  o Raise the maximum error number in IS_ERR_VALUE to 4095.
>>
>> +#define IS_ERR_VALUE(x) unlikely((x) >= (unsigned long)-MAX_ERRNO)

There seems to be a slight problem with doing that. Running
`ldd /bin/bash` prints out

	linux-gate.so.1 =>  (0xffffe000)

and the topmost address a kernel function can return is 0xFFFFf000 when 
MAX_ERRNO=4095, but that is going to be tight with the vdso mapped at 
0xffffE000.

Or is ldd giving wrong output? Because actually, `cat /proc/$$/maps` 
($$=bash) says it is mapped a lot less high:

a7fbe000-a7fbf000 r-xp a7fbe000 00:00 0          [vdso]

(CONFIG_COMPAT_VDSO=y, CONFIG_PAGE_OFFSET=0xB0000000, 2.6.18)

	-`J'
-- 

Re: IS_ERR Threshold Value

[Nathan Scott]

Hi Erik,

On Wed, Jun 28, 2006 at 02:57:07PM -0600, Erik Frederiksen wrote:
> 
> from include/asm-mips/errno.h
> #define EDQUOT      1133    /* Quota exceeded */
> 
> I noticed that the errno value for EDQUOT on MIPS is considerably larger
> than all others.  This can lead to a situation where functions using
> ERR_PTR() to return error codes in pointers cannot return this error
> code without IS_ERR() thinking that the pointer is valid.  In my case,
> it caused an alignment exception in the XFS open call when quota has
> been exceeded in the linux-mips 2.6.14 kernel.  I think that the XFS
> code has changed enough that this bug isn't in newer versions, though I
> haven't done a thorough investigation.  

Hmm, I'm not sure I understand the XFS side of your report here - on
open, for quota to be coming into play we must be creating a new inode
and those code paths inside XFS have no use of IS_ERR/ERR_PTR magic...
did you mean there's generic problems here (I can see those macros are
used in the generic VFS open() code) ... or am I missing your point?

thanks.

-- 
Nathan

Re: IS_ERR Threshold Value

[Erik Frederiksen]

Hi Nathan,

On Wed, 2006-06-28 at 16:41, Nathan Scott wrote:
> 
> Hmm, I'm not sure I understand the XFS side of your report here - on
> open, for quota to be coming into play we must be creating a new inode
> and those code paths inside XFS have no use of IS_ERR/ERR_PTR magic...
> did you mean there's generic problems here (I can see those macros are
> used in the generic VFS open() code) ... or am I missing your point?

Yes, that's right.  The error is being returned from xfs_create when
quota has been exceeded.  It ends up carrying back to the filp_open call
in do_sys_open, which returns it as a pointer to a filp structure. 
Because the errno is so large, IS_ERR reports it as being a valid
pointer incorrectly.

XFS has acted correctly.  The only reason I bring it up is this is how
the bug was brought to my attention.  

If there won't be any strange side effects (I don't have the experience
to accurately comment on this), I think turning the threshold value up
to something we can get away with in IS_ERR_VALUE() would be
appropriate.

-erik

Re: IS_ERR Threshold Value

[Randy.Dunlap]

On Wed, 28 Jun 2006 17:13:33 -0600 Erik Frederiksen wrote:

> Hi Nathan,
> 
> On Wed, 2006-06-28 at 16:41, Nathan Scott wrote:
> > 
> > Hmm, I'm not sure I understand the XFS side of your report here - on
> > open, for quota to be coming into play we must be creating a new inode
> > and those code paths inside XFS have no use of IS_ERR/ERR_PTR magic...
> > did you mean there's generic problems here (I can see those macros are
> > used in the generic VFS open() code) ... or am I missing your point?
> 
> Yes, that's right.  The error is being returned from xfs_create when
> quota has been exceeded.  It ends up carrying back to the filp_open call
> in do_sys_open, which returns it as a pointer to a filp structure. 
> Because the errno is so large, IS_ERR reports it as being a valid
> pointer incorrectly.
> 
> XFS has acted correctly.  The only reason I bring it up is this is how
> the bug was brought to my attention.  
> 
> If there won't be any strange side effects (I don't have the experience
> to accurately comment on this), I think turning the threshold value up
> to something we can get away with in IS_ERR_VALUE() would be
> appropriate.

We need to get the threshold == 4095 patch into -mm to make sure
that it doesn't break anything and/or fix whatever it breaks.

Are you planning to do that patch?  If not, someone else (or I) can.

---
~Randy

Re: IS_ERR Threshold Value

[Nathan Scott]

On Wed, Jun 28, 2006 at 05:13:33PM -0600, Erik Frederiksen wrote:
> On Wed, 2006-06-28 at 16:41, Nathan Scott wrote:
> > Hmm, I'm not sure I understand the XFS side of your report here - on
> ...
> XFS has acted correctly.  The only reason I bring it up is this is how
> the bug was brought to my attention.  

Ah, OK.  When you said this...

| it caused an alignment exception in the XFS open call when quota has
| been exceeded in the linux-mips 2.6.14 kernel.  I think that the XFS
| code has changed enough that this bug isn't in newer versions, though I
| haven't done a thorough investigation.

... I couldn't think of anything we'd changed in XFS that would have
addressed this since that kernel version.

> If there won't be any strange side effects (I don't have the experience
> to accurately comment on this), I think turning the threshold value up
> to something we can get away with in IS_ERR_VALUE() would be
> appropriate.

Seems right to me too, FWIW.

cheers.

-- 
Nathan

Re: IS_ERR Threshold Value

[Bodo Eggert]

Jan Engelhardt <jengelh@linux01.gwdg.de> wrote:

>>> +#define IS_ERR_VALUE(x) unlikely((x) >= (unsigned long)-MAX_ERRNO)
> 
> There seems to be a slight problem with doing that. Running
> `ldd /bin/bash` prints out
> 
> linux-gate.so.1 =>  (0xffffe000)
> 
> and the topmost address a kernel function can return is 0xFFFFf000 when
> MAX_ERRNO=4095, but that is going to be tight with the vdso mapped at
> 0xffffE000.

http://www.trilithium.com/johan/2005/08/linux-gate/

"... an x86 box where processes live in plain old 32-bit address spaces
divided into pages of 4096 bytes, making ffffe000 the penultimate page.
The very last page is reserved to catch accesses through invalid pointers,
e.g. dereferencing a decremented NULL pointer or a MAP_FAILED pointer
returned from mmap."

Therefore, MAX_ERRNO < PAGE_SIZE is safe.
-- 
Ich danke GMX dafür, die Verwendung meiner Adressen mittels per SPF
verbreiteten Lügen zu sabotieren.

http://david.woodhou.se/why-not-spf.html