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

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

[Marcin Slusarz]

Signed-off-by: Marcin Slusarz <marcin.slusarz@gmail.com>
Cc: Andrew Morton <akpm@osdl.org>
---
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(-)

diff --git a/include/linux/err.h b/include/linux/err.h
--- a/include/linux/err.h
+++ b/include/linux/err.h
@@ -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;
 }
 
+#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;
-- 

Re: [PATCH] let ERR_PTR BUILD_BUG_ON when we know its argument is not a valid errno

[Andrew Morton]

On Sun, 11 May 2008 22:12:14 +0200
Marcin Slusarz <marcin.slusarz@gmail.com> wrote:

> Signed-off-by: Marcin Slusarz <marcin.slusarz@gmail.com>
> Cc: Andrew Morton <akpm@osdl.org>
> ---
> 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(-)
> 
> diff --git a/include/linux/err.h b/include/linux/err.h
> --- a/include/linux/err.h
> +++ b/include/linux/err.h
> @@ -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;
>  }
>  
> +#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?)

Thanks.

Re: [PATCH] let ERR_PTR BUILD_BUG_ON when we know its argument is not a valid errno

[Marcin Slusarz]

On Mon, May 12, 2008 at 04:38:30PM -0700, Andrew Morton wrote:
> On Sun, 11 May 2008 22:12:14 +0200
> Marcin Slusarz <marcin.slusarz@gmail.com> wrote:
> 
> > Signed-off-by: Marcin Slusarz <marcin.slusarz@gmail.com>
> > Cc: Andrew Morton <akpm@osdl.org>
> > ---
> > 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(-)
> > 
> > diff --git a/include/linux/err.h b/include/linux/err.h
> > --- a/include/linux/err.h
> > +++ b/include/linux/err.h
> > @@ -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;
> >  }
> >  
> > +#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.

$ git grep 'ERR_PTR(-E[A-Z]*)' | wc -l
1431

> 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

I thought BUILD_BUG_ON uses __builtin_constant_p internally and it was
a big mistake (see below).

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

Well, I think it's better to find more errors at compile time,
than on rare runtime situation (error handling).
This patch found 2 errors on current sources (but one of them was harmless).

>  More lengthy than zero, anyway...
> 
> (If a compile-time check is needed then why not a runtime one also?)
I'm not sure - it would make kernel slightly bigger. I'll check that.

Today I discovered, that this patch causes funny runtime errors (/proc is mounted,
but many applications think it's not), so ignore this patch for now.
I'll prepare second version.

Marcin

[PATCH 0/6] Sanity checks for ERR_PTR argument

[Marcin Slusarz]

This little patchset adds sanity checks to ERR_PTR.

[PATCH 1/6] ERR_PTR: if errno value is known at compile time, make sure it's valid
[PATCH 2/6] ERR_PTR: add ERR_OR_0_PTR
[PATCH 3/6] vfs: open_exec cleanup
[PATCH 4/6] procfs: switch ERR_PTR to ERR_OR_0_PTR when "error" might be 0
[PATCH 5/6] vfs: fix ERR_PTR abuse in generic_readlink
[PATCH 6/6] ERR_PTR: warn when ERR_PTR parameter is not errno value

The most important are patches 1 and 6. 2nd patch adds another ERR_PTR-like helper.
The rest (3-5) fix false positives found by 6th patch.

---
 fs/exec.c           |   44 ++++++++++++++++++++++----------------------
 fs/namei.c          |   15 ++++++++-------
 fs/proc/base.c      |    4 ++--
 include/linux/err.h |   14 +++++++++++++-
 4 files changed, 45 insertions(+), 32 deletions(-)
---

[PATCH 1/6] ERR_PTR: if errno value is known at compile time, make sure it's valid

[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.)

Signed-off-by: Marcin Slusarz <marcin.slusarz@gmail.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
---
 include/linux/err.h |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/include/linux/err.h b/include/linux/err.h
index ec87f31..7b5daa6 100644
--- a/include/linux/err.h
+++ b/include/linux/err.h
@@ -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;
 }
 
+#define ERR_PTR(error) (BUILD_BUG_ON(__builtin_constant_p(error) && !IS_ERR_VALUE(error)), __ERR_PTR(error))
+
 static inline long PTR_ERR(const void *ptr)
 {
 	return (long) ptr;
-- 
1.5.4.5

[PATCH 2/6] ERR_PTR: add ERR_OR_0_PTR

[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.

Signed-off-by: Marcin Slusarz <marcin.slusarz@gmail.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
---
 include/linux/err.h |    7 +++++++
 1 files changed, 7 insertions(+), 0 deletions(-)

diff --git a/include/linux/err.h b/include/linux/err.h
index 7b5daa6..cdec8b6 100644
--- a/include/linux/err.h
+++ b/include/linux/err.h
@@ -26,6 +26,13 @@ static inline void *__ERR_PTR(long error)
 
 #define ERR_PTR(error) (BUILD_BUG_ON(__builtin_constant_p(error) && !IS_ERR_VALUE(error)), __ERR_PTR(error))
 
+static inline void *__ERR_OR_0_PTR(long error)
+{
+	return (void *) error;
+}
+
+#define ERR_OR_0_PTR(error) (BUILD_BUG_ON(__builtin_constant_p(error) && error && !IS_ERR_VALUE(error)), __ERR_OR_0_PTR(error))
+
 static inline long PTR_ERR(const void *ptr)
 {
 	return (long) ptr;
-- 
1.5.4.5

[PATCH 3/6] vfs: open_exec cleanup

[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.

Signed-off-by: Marcin Slusarz <marcin.slusarz@gmail.com>
Cc: Al Viro <viro@zeniv.linux.org.uk>
Cc: Andrew Morton <akpm@linux-foundation.org>
---
 fs/exec.c |   44 ++++++++++++++++++++++----------------------
 1 files changed, 22 insertions(+), 22 deletions(-)

diff --git a/fs/exec.c b/fs/exec.c
index aeaa979..ca8b512 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -656,34 +656,34 @@ struct file *open_exec(const char *name)
 	struct nameidata nd;
 	int err;
 	struct file *file;
+	struct inode *inode;
 
 	err = path_lookup_open(AT_FDCWD, name, LOOKUP_FOLLOW, &nd, FMODE_READ|FMODE_EXEC);
-	file = ERR_PTR(err);
-
-	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);
-					}
+	if (err)
+		return ERR_PTR(err);
+
+	inode = nd.path.dentry->d_inode;
+	file = ERR_PTR(-EACCES);
+	if (S_ISREG(inode->i_mode)) {
+		int err = vfs_permission(&nd, MAY_EXEC);
+		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;
 			}
+			goto out;
 		}
-		release_open_intent(&nd);
-		path_put(&nd.path);
+		else
+			file = ERR_PTR(err);
 	}
-	goto out;
+	release_open_intent(&nd);
+	path_put(&nd.path);
+out:
+	return file;
 }
 
 EXPORT_SYMBOL(open_exec);
-- 
1.5.4.5

[PATCH 4/6] procfs: switch ERR_PTR to ERR_OR_0_PTR when "error" might be 0

[Marcin Slusarz]

proc_pid_follow_link and proc_self_follow_link call ERR_PTR with 0 argument
(invalid errno) in most of situtations.
Replace ERR_PTR calls with ERR_OR_0_PTR which accepts 0 argument.

Signed-off-by: Marcin Slusarz <marcin.slusarz@gmail.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
---
 fs/proc/base.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/fs/proc/base.c b/fs/proc/base.c
index 808cbdc..ebbb8e6 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -1292,7 +1292,7 @@ static void *proc_pid_follow_link(struct dentry *dentry, struct nameidata *nd)
 	error = PROC_I(inode)->op.proc_get_link(inode, &nd->path);
 	nd->last_type = LAST_BIND;
 out:
-	return ERR_PTR(error);
+	return ERR_OR_0_PTR(error);
 }
 
 static int do_proc_readlink(struct path *path, char __user *buffer, int buflen)
@@ -2228,7 +2228,7 @@ static void *proc_self_follow_link(struct dentry *dentry, struct nameidata *nd)
 	if (!tgid)
 		return ERR_PTR(-ENOENT);
 	sprintf(tmp, "%d", task_tgid_nr_ns(current, ns));
-	return ERR_PTR(vfs_follow_link(nd,tmp));
+	return ERR_OR_0_PTR(vfs_follow_link(nd,tmp));
 }
 
 static const struct inode_operations proc_self_inode_operations = {
-- 
1.5.4.5

[PATCH 5/6] vfs: fix ERR_PTR abuse in generic_readlink

[Marcin Slusarz]

generic_readlink calls ERR_PTR for negative and positive values
(vfs_readlink returns length of "link"), but it should not
(not an errno) and does not need to.

Signed-off-by: Marcin Slusarz <marcin.slusarz@gmail.com>
Cc: Al Viro <viro@ZenIV.linux.org.uk>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Christoph Hellwig <hch@lst.de>
---
 fs/namei.c |   15 ++++++++-------
 1 files changed, 8 insertions(+), 7 deletions(-)

diff --git a/fs/namei.c b/fs/namei.c
index 32fd965..959523d 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -2853,16 +2853,17 @@ int generic_readlink(struct dentry *dentry, char __user *buffer, int buflen)
 {
 	struct nameidata nd;
 	void *cookie;
+	int res;
 
 	nd.depth = 0;
 	cookie = dentry->d_inode->i_op->follow_link(dentry, &nd);
-	if (!IS_ERR(cookie)) {
-		int res = vfs_readlink(dentry, buffer, buflen, nd_get_link(&nd));
-		if (dentry->d_inode->i_op->put_link)
-			dentry->d_inode->i_op->put_link(dentry, &nd, cookie);
-		cookie = ERR_PTR(res);
-	}
-	return PTR_ERR(cookie);
+	if (IS_ERR(cookie))
+		return PTR_ERR(cookie);
+
+	res = vfs_readlink(dentry, buffer, buflen, nd_get_link(&nd));
+	if (dentry->d_inode->i_op->put_link)
+		dentry->d_inode->i_op->put_link(dentry, &nd, cookie);
+	return res;
 }
 
 int vfs_follow_link(struct nameidata *nd, const char *link)
-- 
1.5.4.5

[PATCH 6/6] ERR_PTR: warn when ERR_PTR parameter is not errno value

[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.

Signed-off-by: Marcin Slusarz <marcin.slusarz@gmail.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
---
 include/linux/err.h |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/include/linux/err.h b/include/linux/err.h
index cdec8b6..51e2459 100644
--- a/include/linux/err.h
+++ b/include/linux/err.h
@@ -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)
 
 static inline void *__ERR_OR_0_PTR(long error)
 {
+	WARN_ON(!IS_ERR_VALUE(error) && error);
 	return (void *) error;
 }
 
-- 
1.5.4.5

Re: [PATCH 2/6] ERR_PTR: add ERR_OR_0_PTR

[Johannes Weiner]

Hi Marcin,

Marcin Slusarz <marcin.slusarz@gmail.com> writes:

> 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 <marcin.slusarz@gmail.com>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> ---
>  include/linux/err.h |    7 +++++++
>  1 files changed, 7 insertions(+), 0 deletions(-)
>
> diff --git a/include/linux/err.h b/include/linux/err.h
> index 7b5daa6..cdec8b6 100644
> --- a/include/linux/err.h
> +++ b/include/linux/err.h
> @@ -26,6 +26,13 @@ static inline void *__ERR_PTR(long error)
>  
>  #define ERR_PTR(error) (BUILD_BUG_ON(__builtin_constant_p(error) && !IS_ERR_VALUE(error)), __ERR_PTR(error))
>  
> +static inline void *__ERR_OR_0_PTR(long error)
> +{
> +	return (void *) error;
> +}
> +
> +#define ERR_OR_0_PTR(error) (BUILD_BUG_ON(__builtin_constant_p(error) && error && !IS_ERR_VALUE(error)), __ERR_OR_0_PTR(error))

ERR_OR_0_PTR could use __ERR_PTR() too.

And please break those lines ;)

	Hannes

Re: [PATCH 6/6] ERR_PTR: warn when ERR_PTR parameter is not errno value

[Johannes Weiner]

Hi Marcin,

Marcin Slusarz <marcin.slusarz@gmail.com> writes:

> 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 <marcin.slusarz@gmail.com>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> ---
>  include/linux/err.h |    3 +++
>  1 files changed, 3 insertions(+), 0 deletions(-)
>
> diff --git a/include/linux/err.h b/include/linux/err.h
> index cdec8b6..51e2459 100644
> --- a/include/linux/err.h
> +++ b/include/linux/err.h
> @@ -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)
>  
>  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.

	Hannes

Re: [PATCH 3/6] vfs: open_exec cleanup

[Christoph Hellwig]

On Mon, May 19, 2008 at 12:01:49AM +0200, Marcin Slusarz wrote:
> 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 <hch@lst.de>

Index: linux-2.6/fs/exec.c
===================================================================
--- linux-2.6.orig/fs/exec.c	2008-05-19 07:45:18.000000000 +0200
+++ linux-2.6/fs/exec.c	2008-05-19 07:53:17.000000000 +0200
@@ -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,

Re: [PATCH 2/6] ERR_PTR: add ERR_OR_0_PTR

[Christoph Hellwig]

On Mon, May 19, 2008 at 12:01:07AM +0200, Marcin Slusarz wrote:
> 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.

Re: [PATCH 2/6] ERR_PTR: add ERR_OR_0_PTR

[Al Viro]

On Mon, May 19, 2008 at 07:55:04AM +0200, Christoph Hellwig wrote:
> On Mon, May 19, 2008 at 12:01:07AM +0200, Marcin Slusarz wrote:
> > 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.

Um... Could somebody explain WTF is wrong with declaring that ERR_PTR(0)
is NULL?  Sure, if we run into a target where converting non-constant
integer with value zero to void * does not result in null pointer, we'll
need to adjust ERR_PTR().  So.  Fscking.  What?
	a) it's not a lot of adjustment, to start with
	b) any such target will require much more work on porting anyway;
this part will be trivial.

Re: [PATCH 1/6] ERR_PTR: if errno value is known at compile time, make sure it's valid

[Alexey Dobriyan]

On Sun, May 18, 2008 at 11:56:53PM +0200, Marcin Slusarz wrote:
> 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.)

> --- a/include/linux/err.h
> +++ b/include/linux/err.h
> @@ -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;
>  }
>  
> +#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 <adobriyan@gmail.com>
---

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

--- a/net/sunrpc/xprtrdma/svc_rdma_transport.c
+++ b/net/sunrpc/xprtrdma/svc_rdma_transport.c
@@ -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);

Re: [PATCH 6/6] ERR_PTR: warn when ERR_PTR parameter is not errno value

[Alexey Dobriyan]

On Mon, May 19, 2008 at 01:13:20AM +0200, Johannes Weiner wrote:
> Marcin Slusarz <marcin.slusarz@gmail.com> writes:
> 
> > 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.

> > --- a/include/linux/err.h
> > +++ b/include/linux/err.h
> > @@ -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)
> >  
> >  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 ?

Re: [PATCH 6/6] ERR_PTR: warn when ERR_PTR parameter is not errno value

[Johannes Weiner]

Hi,

Alexey Dobriyan <adobriyan@gmail.com> writes:

> On Mon, May 19, 2008 at 01:13:20AM +0200, Johannes Weiner wrote:
>> Marcin Slusarz <marcin.slusarz@gmail.com> writes:
>> 
>> > 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.
>
>> > --- a/include/linux/err.h
>> > +++ b/include/linux/err.h
>> > @@ -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)
>> >  
>> >  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 ?

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

	Hannes

Re: [PATCH 3/6] vfs: open_exec cleanup

[Marcin Slusarz]

On Mon, May 19, 2008 at 07:53:34AM +0200, Christoph Hellwig wrote:
> On Mon, May 19, 2008 at 12:01:49AM +0200, Marcin Slusarz wrote:
> > 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.

looks much better, thanks
Reviewed-by: Marcin Slusarz <marcin.slusarz@gmail.com>
 
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> 
> Index: linux-2.6/fs/exec.c
> ===================================================================
> --- linux-2.6.orig/fs/exec.c	2008-05-19 07:45:18.000000000 +0200
> +++ linux-2.6/fs/exec.c	2008-05-19 07:53:17.000000000 +0200
> @@ -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,
> 

Re: [PATCH 1/6] ERR_PTR: if errno value is known at compile time, make sure it's valid

[Marcin Slusarz]

On Mon, May 19, 2008 at 10:38:52AM +0400, Alexey Dobriyan wrote:
> On Sun, May 18, 2008 at 11:56:53PM +0200, Marcin Slusarz wrote:
> > 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.)
> 
> > --- a/include/linux/err.h
> > +++ b/include/linux/err.h
> > @@ -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;
> >  }
> >  
> > +#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).
Thanks, added to new version.

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

http://lkml.org/lkml/2008/5/11/159

> 
> [PATCH] xprtrdma: fix ERR_PTR(E typo
> 
> Signed-off-by: Alexey Dobriyan <adobriyan@gmail.com>
> ---
> 
>  net/sunrpc/xprtrdma/svc_rdma_transport.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> --- a/net/sunrpc/xprtrdma/svc_rdma_transport.c
> +++ b/net/sunrpc/xprtrdma/svc_rdma_transport.c
> @@ -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);
> 

Re: [PATCH 6/6] ERR_PTR: warn when ERR_PTR parameter is not errno value

[Marcin Slusarz]

On Mon, May 19, 2008 at 02:11:21PM +0200, Johannes Weiner wrote:
> Hi,
> 
> Alexey Dobriyan <adobriyan@gmail.com> writes:
> 
> > On Mon, May 19, 2008 at 01:13:20AM +0200, Johannes Weiner wrote:
> >> Marcin Slusarz <marcin.slusarz@gmail.com> writes:
> >> 
> >> > 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.
> >
> >> > --- a/include/linux/err.h
> >> > +++ b/include/linux/err.h
> >> > @@ -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)
> >> >  
> >> >  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 ?
> 
> Well, okay.  Perhaps ratelimiting?  I really prefer a bigger image size
> over a spammed dmesg.

Well, when I add 0 as a valid argument for ERR_PTR, this check won't add too many
false positives, so I think simple WARN_ON is safe.

Marcin