let ERR_PTR BUILD_BUG_ON when we know its argument is not a valid errno

Signed-off-by Marcin Slusarz <[email protected]>

Cc Andrew Morton <[email protected]>

---
allmodconfig compile tested (on x86_64)

should be applied after:
net/sunrpc/xprtrdma: fix svc_rdma_create out of memory error path
jfs: 0 is not valid errno value

---
 include/linux/err.h |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

include/linux/err.h +0 -1

@@ -19,11 +19,13 @@
 
 #define IS_ERR_VALUE(x) unlikely((x) >= (unsigned long)-MAX_ERRNO)
 
-static inline void *ERR_PTR(long error)

Quoted 20 lines · On Sun, 11 May 2008 22:12:14 +0200 Marcin Slusarz <[email protected]>…

Signed-off-by Marcin Slusarz <[email protected]>

Cc Andrew Morton <[email protected]>

---
allmodconfig compile tested (on x86_64)

should be applied after:
net/sunrpc/xprtrdma: fix svc_rdma_create out of memory error path
jfs: 0 is not valid errno value

---
 include/linux/err.h |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

include/linux/err.h +0 -1

@@ -19,11 +19,13 @@
 
 #define IS_ERR_VALUE(x) unlikely((x) >= (unsigned long)-MAX_ERRNO)
 
-static inline void *ERR_PTR(long error)

include/linux/err.h line 31

Quoted 10 lines

include/linux/err.h +3 -0

+static inline void *__ERR_PTR(long error)
 {
 	return (void *) error;
 }
 
+#define ERR_PTR(error) (BUILD_BUG_ON(!IS_ERR_VALUE(error)), __ERR_PTR(error))
+
 static inline long PTR_ERR(const void *ptr)
 {
 	return (long) ptr;

Not sure about this one. BUILD_BUG_ON only makes sense if the value is a compile-time constant. I think the code as you have it will take this:

	int e = foo();

	p = ERR_PTR(e);

and will attempt to evaluate sizeof() on a negative-sized array at runtime. The conmpile will laugh and throw that all away, but it's a bit weird.

Plus I'd have thought that the amount of code which does ERR_PTR(-EFOO) is fairly small, but perhaps that's wrong.

If I _am_ wrong then I do think it'd be saner to only do the BUILD_BUG_ON() if __builtin_constant_p(error) evaluates true. And even

then I do think we'd like to see a more lengthy justification of why the kernel needs this check.

More lengthy than zero, anyway...

(If a compile-time check is needed then why not a runtime one also?)

ERR_PTR is easy to call with wrong argument (positive errno), and this error lead to catastrophic event - oops or kernel panic (dereference of invalid pointer).

As most of error handling code paths are rarely tested, this kind of bug can be hidden for years.

(Currently there are > 1400 calls of ERR_PTR with constant argument.)

Quoted 21 lines · Marcin Slusarz

ERR_PTR is easy to call with wrong argument (positive errno), and this error lead to catastrophic event - oops or kernel panic (dereference of invalid pointer).

As most of error handling code paths are rarely tested, this kind of bug can be hidden for years.

(Currently there are > 1400 calls of ERR_PTR with constant argument.)

include/linux/err.h +1 -1

@@ -19,11 +19,13 @@
 
 #define IS_ERR_VALUE(x) unlikely((x) >= (unsigned long)-MAX_ERRNO)
 
-static inline void *ERR_PTR(long error)
+static inline void *__ERR_PTR(long error)
 {
 	return (void *) error;
 }
 

include/linux/err.h line 27

Quoted 1 line

include/linux/err.h +1 -0

+#define ERR_PTR(error) (BUILD_BUG_ON(__builtin_constant_p(error) && !IS_ERR_VALUE(error)), __ERR_PTR(error))

This needs comment that this construct must be a macro, otherwise gcc does something stupid and doesn't break the build (at least in the case below).

BTW, I used grep(1) to find it.

[PATCH] xprtrdma: fix ERR_PTR(E typo

Signed-off-by Alexey Dobriyan <[email protected]>

---

 net/sunrpc/xprtrdma/svc_rdma_transport.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

net/sunrpc/xprtrdma/svc_rdma_transport.c +1 -1

@@ -661,7 +661,7 @@ static struct svc_xprt *svc_rdma_create(struct svc_serv *serv,
 
 	cma_xprt = rdma_create_xprt(serv, 1);
 	if (!cma_xprt)
-		return ERR_PTR(ENOMEM);
+		return ERR_PTR(-ENOMEM);
 	xprt = &cma_xprt->sc_xprt;
 
 	listen_id = rdma_create_id(rdma_listen_handler, cma_xprt, RDMA_PS_TCP);

Some codepaths call ERR_PTR with possibly 0 argument, which is not a valid errno and rely on conversion from 0 to NULL pointer. Add ERR_OR_0_PTR function which accepts errnos and 0 as an argument.

Signed-off-by Marcin Slusarz <[email protected]>

Cc Andrew Morton <[email protected]>

---
 include/linux/err.h |    7 +++++++
 1 files changed, 7 insertions(+), 0 deletions(-)

include/linux/err.h +0 -0

index 7b5daa6..cdec8b6 100644

Some codepaths call ERR_PTR with possibly 0 argument, which is not a valid errno and rely on conversion from 0 to NULL pointer. Add ERR_OR_0_PTR function which accepts errnos and 0 as an argument.

Quoted 3 lines · Marcin Slusarz

Some codepaths call ERR_PTR with possibly 0 argument, which is not a valid errno and rely on conversion from 0 to NULL pointer. Add ERR_OR_0_PTR function which accepts errnos and 0 as an argument.

Sorry, no. ERR_PTR(0) is perfectly valid, you just don't want to return the actualy value. E.g. we have a common idiom of:

	some_ptr = ERR_PTR(err);
	if (IS_ERR(some_ptr))
		goto out_handle_err;

and obsfucating this with new syntactic sugar is not a good idea.

open_exec is needlessly indented, calls ERR_PTR with 0 argument (which is not valid errno) and jumps into middle of function just to return value.
So clean it up a bit.

Quoted 4 lines · Marcin Slusarz

open_exec is needlessly indented, calls ERR_PTR with 0 argument (which is not valid errno) and jumps into middle of function just to return value.
So clean it up a bit.

Still looks rather messy. See below for a better version.

Signed-off-by Christoph Hellwig <[email protected]>

Index: linux-2.6/fs/exec.c
===================================================================

fs/exec.c +29 -27

@@ -654,38 +654,40 @@ EXPORT_SYMBOL(setup_arg_pages);
 struct file *open_exec(const char *name)
 {
 	struct nameidata nd;
-	int err;
 	struct file *file;
+	int err;
 
-	err = path_lookup_open(AT_FDCWD, name, LOOKUP_FOLLOW, &nd, FMODE_READ|FMODE_EXEC);
-	file = ERR_PTR(err);
+	err = path_lookup_open(AT_FDCWD, name, LOOKUP_FOLLOW, &nd,
+				FMODE_READ|FMODE_EXEC);
+	if (err)
+		goto out;
 
-	if (!err) {
-		struct inode *inode = nd.path.dentry->d_inode;
-		file = ERR_PTR(-EACCES);
-		if (S_ISREG(inode->i_mode)) {
-			int err = vfs_permission(&nd, MAY_EXEC);
-			file = ERR_PTR(err);
-			if (!err) {
-				file = nameidata_to_filp(&nd,
-							O_RDONLY|O_LARGEFILE);
-				if (!IS_ERR(file)) {
-					err = deny_write_access(file);
-					if (err) {
-						fput(file);
-						file = ERR_PTR(err);
-					}
-				}
-out:
-				return file;
-			}
-		}
-		release_open_intent(&nd);
-		path_put(&nd.path);
+	err = -EACCES;
+	if (!S_ISREG(nd.path.dentry->d_inode->i_mode))
+		goto out_path_put;
+
+	err = vfs_permission(&nd, MAY_EXEC);
+	if (err)
+		goto out_path_put;
+
+	file = nameidata_to_filp(&nd, O_RDONLY|O_LARGEFILE);
+	if (IS_ERR(file))
+		return file;
+
+	err = deny_write_access(file);
+	if (err) {
+		fput(file);
+		goto out;
 	}
-	goto out;
-}
 
+	return file;
+
+ out_path_put:
+	release_open_intent(&nd);
+	path_put(&nd.path);
+ out:
+	return ERR_PTR(err);
+}
 EXPORT_SYMBOL(open_exec);
 
 int kernel_read(struct file *file, unsigned long offset,

Check at runtime whether error argument of ERR_PTR and ERR_OR_0_PTR is valid. It can catch bugs which possibly lead to oops or panic earlier.

Currently there are > 600 calls of ERR_PTR with non-constant argument in Linux kernel sources.

Signed-off-by Marcin Slusarz <[email protected]>

Cc Andrew Morton <[email protected]>

---
 include/linux/err.h |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

include/linux/err.h +2 -0

index cdec8b6..51e2459 100644
@@ -3,6 +3,7 @@
 
 #include <linux/compiler.h>
 
+#include <asm/bug.h>
 #include <asm/errno.h>
 
 /*

@@ -21,6 +22,7 @@
 
 static inline void *__ERR_PTR(long error)
 {
+	WARN_ON(!IS_ERR_VALUE(error));
 	return (void *) error;
 }
 

Quoted 26 lines · Marcin Slusarz

Check at runtime whether error argument of ERR_PTR and ERR_OR_0_PTR is valid. It can catch bugs which possibly lead to oops or panic earlier.

Currently there are > 600 calls of ERR_PTR with non-constant argument in Linux kernel sources.

include/linux/err.h +2 -0

@@ -3,6 +3,7 @@
 
 #include <linux/compiler.h>
 
+#include <asm/bug.h>
 #include <asm/errno.h>
 
 /*

@@ -21,6 +22,7 @@
 
 static inline void *__ERR_PTR(long error)
 {
+	WARN_ON(!IS_ERR_VALUE(error));
 	return (void *) error;
 }
 

@@ -28,6 +30,7 @@ static inline void *__ERR_PTR(long error)
 

include/linux/err.h static inline void *__ERR_PTR(long error) · line 35

Quoted 5 lines

include/linux/err.h +1 -0

 static inline void *__ERR_OR_0_PTR(long error)
 {
+	WARN_ON(!IS_ERR_VALUE(error) && error);
 	return (void *) error;
 }

How about WARN_ON_ONCE() instead? That would warn once for each erroneous user which should be enough.

Quoted 28 lines · Johannes Weiner

Quoted 26 lines · Marcin Slusarz

Check at runtime whether error argument of ERR_PTR and ERR_OR_0_PTR is valid. It can catch bugs which possibly lead to oops or panic earlier.

Currently there are > 600 calls of ERR_PTR with non-constant argument in Linux kernel sources.

include/linux/err.h +2 -0

@@ -3,6 +3,7 @@
 
 #include <linux/compiler.h>
 
+#include <asm/bug.h>
 #include <asm/errno.h>
 
 /*

@@ -21,6 +22,7 @@
 
 static inline void *__ERR_PTR(long error)
 {
+	WARN_ON(!IS_ERR_VALUE(error));
 	return (void *) error;
 }
 

@@ -28,6 +30,7 @@ static inline void *__ERR_PTR(long error)
 

Quoted 8 lines

include/linux/err.h static inline void *__ERR_PTR(long error) · line 35

Quoted 5 lines

include/linux/err.h +1 -0

 static inline void *__ERR_OR_0_PTR(long error)
 {
+	WARN_ON(!IS_ERR_VALUE(error) && error);
 	return (void *) error;
 }

How about WARN_ON_ONCE() instead? That would warn once for each erroneous user which should be enough.

And blow up .bss ?

Hi,

Quoted 38 lines · Alexey Dobriyan

Quoted 36 lines · Johannes Weiner

Quoted 25 lines · Marcin Slusarz

Check at runtime whether error argument of ERR_PTR and ERR_OR_0_PTR is valid. It can catch bugs which possibly lead to oops or panic earlier.

Currently there are > 600 calls of ERR_PTR with non-constant argument in Linux kernel sources.

include/linux/err.h +2 -0

@@ -3,6 +3,7 @@
 
 #include <linux/compiler.h>
 
+#include <asm/bug.h>
 #include <asm/errno.h>
 
 /*

@@ -21,6 +22,7 @@
 
 static inline void *__ERR_PTR(long error)
 {
+	WARN_ON(!IS_ERR_VALUE(error));
 	return (void *) error;
 }
 

include/linux/err.h static inline void *__ERR_PTR(long error) · line 35

Quoted 6 lines

include/linux/err.h +1 -0

@@ -28,6 +30,7 @@ static inline void *__ERR_PTR(long error)
 
 static inline void *__ERR_OR_0_PTR(long error)
 {
+	WARN_ON(!IS_ERR_VALUE(error) && error);
 	return (void *) error;
 }

How about WARN_ON_ONCE() instead? That would warn once for each erroneous user which should be enough.

Quoted 1 line

And blow up .bss ?

Well, okay. Perhaps ratelimiting? I really prefer a bigger image size over a spammed dmesg.