[PATCH 1/2] debugfs: fix debugfs_rename parameter checking

[PATCH 1/2] debugfs: fix debugfs_rename parameter checking

[Greg Kroah-Hartman]

debugfs_rename() needs to check that the dentries passed into it really
are valid, as sometimes they are not (i.e. if the return value of
another debugfs call is passed into this one.)  So fix this up by
properly checking if the two parent directories are errors (they are
allowed to be NULL), and if the dentry to rename is not NULL or an
error.

Cc: stable <stable@vger.kernel.org>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 fs/debugfs/inode.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/fs/debugfs/inode.c b/fs/debugfs/inode.c
index 13b01351dd1c..41ef452c1fcf 100644
--- a/fs/debugfs/inode.c
+++ b/fs/debugfs/inode.c
@@ -787,6 +787,13 @@ struct dentry *debugfs_rename(struct dentry *old_dir, struct dentry *old_dentry,
 	struct dentry *dentry = NULL, *trap;
 	struct name_snapshot old_name;
 
+	if (IS_ERR(old_dir))
+		return old_dir;
+	if (IS_ERR(new_dir))
+		return new_dir;
+	if (IS_ERR_OR_NULL(old_dentry))
+		return old_dentry;
+
 	trap = lock_rename(new_dir, old_dir);
 	/* Source or destination directories don't exist? */
 	if (d_really_is_negative(old_dir) || d_really_is_negative(new_dir))
-- 
2.20.1

[PATCH 2/2] debugfs: return error values, not NULL

[Greg Kroah-Hartman]

When an error happens, debugfs should return an error pointer value, not
NULL.  This will prevent the totally theoretical error where a debugfs
call fails due to lack of memory, returning NULL, and that dentry value
is then passed to another debugfs call, which would end up succeeding,
creating a file at the root of the debugfs tree, but would then be
impossible to remove (because you can not remove the directory NULL).

So, to make everyone happy, always return errors, this makes the users
of debugfs much simpler (they do not have to ever check the return
value), and everyone can rest easy.

Reported-by: Masami Hiramatsu <mhiramat@kernel.org>
Reported-by: Ulf Hansson <ulf.hansson@linaro.org>
Reported-by: Gary R Hook <ghook@amd.com>
Reported-by: Heiko Carstens <heiko.carstens@de.ibm.com>
Cc: stable <stable@vger.kernel.org>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 fs/debugfs/inode.c | 39 ++++++++++++++++++++++-----------------
 1 file changed, 22 insertions(+), 17 deletions(-)

diff --git a/fs/debugfs/inode.c b/fs/debugfs/inode.c
index 41ef452c1fcf..b16f8035b1af 100644
--- a/fs/debugfs/inode.c
+++ b/fs/debugfs/inode.c
@@ -254,8 +254,8 @@ MODULE_ALIAS_FS("debugfs");
  * @parent: a pointer to the parent dentry of the file.
  *
  * This function will return a pointer to a dentry if it succeeds.  If the file
- * doesn't exist or an error occurs, %NULL will be returned.  The returned
- * dentry must be passed to dput() when it is no longer needed.
+ * doesn't exist or an error occurs, %ERR_PTR(-ERROR) will be returned.  The
+ * returned dentry must be passed to dput() when it is no longer needed.
  *
  * If debugfs is not enabled in the kernel, the value -%ENODEV will be
  * returned.
@@ -265,17 +265,17 @@ struct dentry *debugfs_lookup(const char *name, struct dentry *parent)
 	struct dentry *dentry;
 
 	if (IS_ERR(parent))
-		return NULL;
+		return parent;
 
 	if (!parent)
 		parent = debugfs_mount->mnt_root;
 
 	dentry = lookup_one_len_unlocked(name, parent, strlen(name));
 	if (IS_ERR(dentry))
-		return NULL;
+		return dentry;
 	if (!d_really_is_positive(dentry)) {
 		dput(dentry);
-		return NULL;
+		return ERR_PTR(-EINVAL);
 	}
 	return dentry;
 }
@@ -324,7 +324,7 @@ static struct dentry *failed_creating(struct dentry *dentry)
 	inode_unlock(d_inode(dentry->d_parent));
 	dput(dentry);
 	simple_release_fs(&debugfs_mount, &debugfs_mount_count);
-	return NULL;
+	return ERR_PTR(-ENOMEM);
 }
 
 static struct dentry *end_creating(struct dentry *dentry)
@@ -347,7 +347,7 @@ static struct dentry *__debugfs_create_file(const char *name, umode_t mode,
 	dentry = start_creating(name, parent);
 
 	if (IS_ERR(dentry))
-		return NULL;
+		return dentry;
 
 	inode = debugfs_get_inode(dentry->d_sb);
 	if (unlikely(!inode))
@@ -386,7 +386,8 @@ static struct dentry *__debugfs_create_file(const char *name, umode_t mode,
  * This function will return a pointer to a dentry if it succeeds.  This
  * pointer must be passed to the debugfs_remove() function when the file is
  * to be removed (no automatic cleanup happens if your module is unloaded,
- * you are responsible here.)  If an error occurs, %NULL will be returned.
+ * you are responsible here.)  If an error occurs, %ERR_PTR(-ERROR) will be
+ * returned.
  *
  * If debugfs is not enabled in the kernel, the value -%ENODEV will be
  * returned.
@@ -464,7 +465,8 @@ EXPORT_SYMBOL_GPL(debugfs_create_file_unsafe);
  * This function will return a pointer to a dentry if it succeeds.  This
  * pointer must be passed to the debugfs_remove() function when the file is
  * to be removed (no automatic cleanup happens if your module is unloaded,
- * you are responsible here.)  If an error occurs, %NULL will be returned.
+ * you are responsible here.)  If an error occurs, %ERR_PTR(-ERROR) will be
+ * returned.
  *
  * If debugfs is not enabled in the kernel, the value -%ENODEV will be
  * returned.
@@ -495,7 +497,8 @@ EXPORT_SYMBOL_GPL(debugfs_create_file_size);
  * This function will return a pointer to a dentry if it succeeds.  This
  * pointer must be passed to the debugfs_remove() function when the file is
  * to be removed (no automatic cleanup happens if your module is unloaded,
- * you are responsible here.)  If an error occurs, %NULL will be returned.
+ * you are responsible here.)  If an error occurs, %ERR_PTR(-ERROR) will be
+ * returned.
  *
  * If debugfs is not enabled in the kernel, the value -%ENODEV will be
  * returned.
@@ -506,7 +509,7 @@ struct dentry *debugfs_create_dir(const char *name, struct dentry *parent)
 	struct inode *inode;
 
 	if (IS_ERR(dentry))
-		return NULL;
+		return dentry;
 
 	inode = debugfs_get_inode(dentry->d_sb);
 	if (unlikely(!inode))
@@ -545,7 +548,7 @@ struct dentry *debugfs_create_automount(const char *name,
 	struct inode *inode;
 
 	if (IS_ERR(dentry))
-		return NULL;
+		return dentry;
 
 	inode = debugfs_get_inode(dentry->d_sb);
 	if (unlikely(!inode))
@@ -581,8 +584,8 @@ EXPORT_SYMBOL(debugfs_create_automount);
  * This function will return a pointer to a dentry if it succeeds.  This
  * pointer must be passed to the debugfs_remove() function when the symbolic
  * link is to be removed (no automatic cleanup happens if your module is
- * unloaded, you are responsible here.)  If an error occurs, %NULL will be
- * returned.
+ * unloaded, you are responsible here.)  If an error occurs, %ERR_PTR(-ERROR)
+ * will be returned.
  *
  * If debugfs is not enabled in the kernel, the value -%ENODEV will be
  * returned.
@@ -594,12 +597,12 @@ struct dentry *debugfs_create_symlink(const char *name, struct dentry *parent,
 	struct inode *inode;
 	char *link = kstrdup(target, GFP_KERNEL);
 	if (!link)
-		return NULL;
+		return ERR_PTR(-ENOMEM);
 
 	dentry = start_creating(name, parent);
 	if (IS_ERR(dentry)) {
 		kfree(link);
-		return NULL;
+		return dentry;
 	}
 
 	inode = debugfs_get_inode(dentry->d_sb);
@@ -827,7 +830,9 @@ struct dentry *debugfs_rename(struct dentry *old_dir, struct dentry *old_dentry,
 	if (dentry && !IS_ERR(dentry))
 		dput(dentry);
 	unlock_rename(new_dir, old_dir);
-	return NULL;
+	if (IS_ERR(dentry))
+		return dentry;
+	return ERR_PTR(-EINVAL);
 }
 EXPORT_SYMBOL_GPL(debugfs_rename);
 
-- 
2.20.1

Re: [PATCH 2/2] debugfs: return error values, not NULL

[Greg Kroah-Hartman]

On Wed, Jan 23, 2019 at 11:28:14AM +0100, Greg Kroah-Hartman wrote:
> When an error happens, debugfs should return an error pointer value, not
> NULL.  This will prevent the totally theoretical error where a debugfs
> call fails due to lack of memory, returning NULL, and that dentry value
> is then passed to another debugfs call, which would end up succeeding,
> creating a file at the root of the debugfs tree, but would then be
> impossible to remove (because you can not remove the directory NULL).
> 
> So, to make everyone happy, always return errors, this makes the users
> of debugfs much simpler (they do not have to ever check the return
> value), and everyone can rest easy.
> 
> Reported-by: Masami Hiramatsu <mhiramat@kernel.org>
> Reported-by: Ulf Hansson <ulf.hansson@linaro.org>
> Reported-by: Gary R Hook <ghook@amd.com>
> Reported-by: Heiko Carstens <heiko.carstens@de.ibm.com>

Oops, also reported by:
	Sebastian Andrzej Siewior <bigeasy@linutronix.de>

I'll go fix that up when I merge this.

greg k-h

Re: [PATCH 2/2] debugfs: return error values, not NULL

[Greg Kroah-Hartman]

On Wed, Jan 23, 2019 at 11:29:30AM +0100, Greg Kroah-Hartman wrote:
> On Wed, Jan 23, 2019 at 11:28:14AM +0100, Greg Kroah-Hartman wrote:
> > When an error happens, debugfs should return an error pointer value, not
> > NULL.  This will prevent the totally theoretical error where a debugfs
> > call fails due to lack of memory, returning NULL, and that dentry value
> > is then passed to another debugfs call, which would end up succeeding,
> > creating a file at the root of the debugfs tree, but would then be
> > impossible to remove (because you can not remove the directory NULL).
> > 
> > So, to make everyone happy, always return errors, this makes the users
> > of debugfs much simpler (they do not have to ever check the return
> > value), and everyone can rest easy.
> > 
> > Reported-by: Masami Hiramatsu <mhiramat@kernel.org>
> > Reported-by: Ulf Hansson <ulf.hansson@linaro.org>
> > Reported-by: Gary R Hook <ghook@amd.com>
> > Reported-by: Heiko Carstens <heiko.carstens@de.ibm.com>
> 
> Oops, also reported by:
> 	Sebastian Andrzej Siewior <bigeasy@linutronix.de>

And also reported by:
	Michal Hocko <mhocko@kernel.org>

I should have checked my inbox better...

Re: [PATCH 2/2] debugfs: return error values, not NULL

[Michal Hocko]

On Wed 23-01-19 11:28:14, Greg KH wrote:
> When an error happens, debugfs should return an error pointer value, not
> NULL.  This will prevent the totally theoretical error where a debugfs
> call fails due to lack of memory, returning NULL, and that dentry value
> is then passed to another debugfs call, which would end up succeeding,
> creating a file at the root of the debugfs tree, but would then be
> impossible to remove (because you can not remove the directory NULL).
> 
> So, to make everyone happy, always return errors, this makes the users
> of debugfs much simpler (they do not have to ever check the return
> value), and everyone can rest easy.

How come this is safe at all? Say you are creating a directory by
debugfs_create_dir and then feed the return value to debugfs_create_files
as a parent. In case of error you are giving it an invalid pointer and
likely blow up unless I miss something.

I do agree that reporting errors is better than a simple catch all NULL
but this should have been done when introduced rather than now when most
callers simply check for NULL as a failure.

> Reported-by: Masami Hiramatsu <mhiramat@kernel.org>
> Reported-by: Ulf Hansson <ulf.hansson@linaro.org>
> Reported-by: Gary R Hook <ghook@amd.com>
> Reported-by: Heiko Carstens <heiko.carstens@de.ibm.com>
> Cc: stable <stable@vger.kernel.org>
> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> ---
>  fs/debugfs/inode.c | 39 ++++++++++++++++++++++-----------------
>  1 file changed, 22 insertions(+), 17 deletions(-)
> 
> diff --git a/fs/debugfs/inode.c b/fs/debugfs/inode.c
> index 41ef452c1fcf..b16f8035b1af 100644
> --- a/fs/debugfs/inode.c
> +++ b/fs/debugfs/inode.c
> @@ -254,8 +254,8 @@ MODULE_ALIAS_FS("debugfs");
>   * @parent: a pointer to the parent dentry of the file.
>   *
>   * This function will return a pointer to a dentry if it succeeds.  If the file
> - * doesn't exist or an error occurs, %NULL will be returned.  The returned
> - * dentry must be passed to dput() when it is no longer needed.
> + * doesn't exist or an error occurs, %ERR_PTR(-ERROR) will be returned.  The
> + * returned dentry must be passed to dput() when it is no longer needed.
>   *
>   * If debugfs is not enabled in the kernel, the value -%ENODEV will be
>   * returned.
> @@ -265,17 +265,17 @@ struct dentry *debugfs_lookup(const char *name, struct dentry *parent)
>  	struct dentry *dentry;
>  
>  	if (IS_ERR(parent))
> -		return NULL;
> +		return parent;
>  
>  	if (!parent)
>  		parent = debugfs_mount->mnt_root;
>  
>  	dentry = lookup_one_len_unlocked(name, parent, strlen(name));
>  	if (IS_ERR(dentry))
> -		return NULL;
> +		return dentry;
>  	if (!d_really_is_positive(dentry)) {
>  		dput(dentry);
> -		return NULL;
> +		return ERR_PTR(-EINVAL);
>  	}
>  	return dentry;
>  }
> @@ -324,7 +324,7 @@ static struct dentry *failed_creating(struct dentry *dentry)
>  	inode_unlock(d_inode(dentry->d_parent));
>  	dput(dentry);
>  	simple_release_fs(&debugfs_mount, &debugfs_mount_count);
> -	return NULL;
> +	return ERR_PTR(-ENOMEM);
>  }
>  
>  static struct dentry *end_creating(struct dentry *dentry)
> @@ -347,7 +347,7 @@ static struct dentry *__debugfs_create_file(const char *name, umode_t mode,
>  	dentry = start_creating(name, parent);
>  
>  	if (IS_ERR(dentry))
> -		return NULL;
> +		return dentry;
>  
>  	inode = debugfs_get_inode(dentry->d_sb);
>  	if (unlikely(!inode))
> @@ -386,7 +386,8 @@ static struct dentry *__debugfs_create_file(const char *name, umode_t mode,
>   * This function will return a pointer to a dentry if it succeeds.  This
>   * pointer must be passed to the debugfs_remove() function when the file is
>   * to be removed (no automatic cleanup happens if your module is unloaded,
> - * you are responsible here.)  If an error occurs, %NULL will be returned.
> + * you are responsible here.)  If an error occurs, %ERR_PTR(-ERROR) will be
> + * returned.
>   *
>   * If debugfs is not enabled in the kernel, the value -%ENODEV will be
>   * returned.
> @@ -464,7 +465,8 @@ EXPORT_SYMBOL_GPL(debugfs_create_file_unsafe);
>   * This function will return a pointer to a dentry if it succeeds.  This
>   * pointer must be passed to the debugfs_remove() function when the file is
>   * to be removed (no automatic cleanup happens if your module is unloaded,
> - * you are responsible here.)  If an error occurs, %NULL will be returned.
> + * you are responsible here.)  If an error occurs, %ERR_PTR(-ERROR) will be
> + * returned.
>   *
>   * If debugfs is not enabled in the kernel, the value -%ENODEV will be
>   * returned.
> @@ -495,7 +497,8 @@ EXPORT_SYMBOL_GPL(debugfs_create_file_size);
>   * This function will return a pointer to a dentry if it succeeds.  This
>   * pointer must be passed to the debugfs_remove() function when the file is
>   * to be removed (no automatic cleanup happens if your module is unloaded,
> - * you are responsible here.)  If an error occurs, %NULL will be returned.
> + * you are responsible here.)  If an error occurs, %ERR_PTR(-ERROR) will be
> + * returned.
>   *
>   * If debugfs is not enabled in the kernel, the value -%ENODEV will be
>   * returned.
> @@ -506,7 +509,7 @@ struct dentry *debugfs_create_dir(const char *name, struct dentry *parent)
>  	struct inode *inode;
>  
>  	if (IS_ERR(dentry))
> -		return NULL;
> +		return dentry;
>  
>  	inode = debugfs_get_inode(dentry->d_sb);
>  	if (unlikely(!inode))
> @@ -545,7 +548,7 @@ struct dentry *debugfs_create_automount(const char *name,
>  	struct inode *inode;
>  
>  	if (IS_ERR(dentry))
> -		return NULL;
> +		return dentry;
>  
>  	inode = debugfs_get_inode(dentry->d_sb);
>  	if (unlikely(!inode))
> @@ -581,8 +584,8 @@ EXPORT_SYMBOL(debugfs_create_automount);
>   * This function will return a pointer to a dentry if it succeeds.  This
>   * pointer must be passed to the debugfs_remove() function when the symbolic
>   * link is to be removed (no automatic cleanup happens if your module is
> - * unloaded, you are responsible here.)  If an error occurs, %NULL will be
> - * returned.
> + * unloaded, you are responsible here.)  If an error occurs, %ERR_PTR(-ERROR)
> + * will be returned.
>   *
>   * If debugfs is not enabled in the kernel, the value -%ENODEV will be
>   * returned.
> @@ -594,12 +597,12 @@ struct dentry *debugfs_create_symlink(const char *name, struct dentry *parent,
>  	struct inode *inode;
>  	char *link = kstrdup(target, GFP_KERNEL);
>  	if (!link)
> -		return NULL;
> +		return ERR_PTR(-ENOMEM);
>  
>  	dentry = start_creating(name, parent);
>  	if (IS_ERR(dentry)) {
>  		kfree(link);
> -		return NULL;
> +		return dentry;
>  	}
>  
>  	inode = debugfs_get_inode(dentry->d_sb);
> @@ -827,7 +830,9 @@ struct dentry *debugfs_rename(struct dentry *old_dir, struct dentry *old_dentry,
>  	if (dentry && !IS_ERR(dentry))
>  		dput(dentry);
>  	unlock_rename(new_dir, old_dir);
> -	return NULL;
> +	if (IS_ERR(dentry))
> +		return dentry;
> +	return ERR_PTR(-EINVAL);
>  }
>  EXPORT_SYMBOL_GPL(debugfs_rename);
>  
> -- 
> 2.20.1

-- 
Michal Hocko
SUSE Labs

Re: [PATCH 2/2] debugfs: return error values, not NULL

[Greg Kroah-Hartman]

On Wed, Jan 23, 2019 at 12:06:28PM +0100, Michal Hocko wrote:
> On Wed 23-01-19 11:28:14, Greg KH wrote:
> > When an error happens, debugfs should return an error pointer value, not
> > NULL.  This will prevent the totally theoretical error where a debugfs
> > call fails due to lack of memory, returning NULL, and that dentry value
> > is then passed to another debugfs call, which would end up succeeding,
> > creating a file at the root of the debugfs tree, but would then be
> > impossible to remove (because you can not remove the directory NULL).
> > 
> > So, to make everyone happy, always return errors, this makes the users
> > of debugfs much simpler (they do not have to ever check the return
> > value), and everyone can rest easy.
> 
> How come this is safe at all? Say you are creating a directory by
> debugfs_create_dir and then feed the return value to debugfs_create_files
> as a parent. In case of error you are giving it an invalid pointer and
> likely blow up unless I miss something.

debugfs_create_files checks for invalid parents and will just refuse to
create the file.  It's always done that.

> I do agree that reporting errors is better than a simple catch all NULL
> but this should have been done when introduced rather than now when most
> callers simply check for NULL as a failure.

I'm fixing up all the "NULL is a failure" callsites in the kernel, see
lkml for the first round of those patches.

thanks,

greg k-h

Re: [PATCH 2/2] debugfs: return error values, not NULL

[Michal Hocko]

On Wed 23-01-19 12:55:35, Greg KH wrote:
> On Wed, Jan 23, 2019 at 12:06:28PM +0100, Michal Hocko wrote:
> > On Wed 23-01-19 11:28:14, Greg KH wrote:
> > > When an error happens, debugfs should return an error pointer value, not
> > > NULL.  This will prevent the totally theoretical error where a debugfs
> > > call fails due to lack of memory, returning NULL, and that dentry value
> > > is then passed to another debugfs call, which would end up succeeding,
> > > creating a file at the root of the debugfs tree, but would then be
> > > impossible to remove (because you can not remove the directory NULL).
> > > 
> > > So, to make everyone happy, always return errors, this makes the users
> > > of debugfs much simpler (they do not have to ever check the return
> > > value), and everyone can rest easy.
> > 
> > How come this is safe at all? Say you are creating a directory by
> > debugfs_create_dir and then feed the return value to debugfs_create_files
> > as a parent. In case of error you are giving it an invalid pointer and
> > likely blow up unless I miss something.
> 
> debugfs_create_files checks for invalid parents and will just refuse to
> create the file.  It's always done that.

I must be missing something because debugfs_create_files does
	d_inode(parent)->i_private = data;
as the very first thing and that means that it dereferences an invalid
pointer right there.

> > I do agree that reporting errors is better than a simple catch all NULL
> > but this should have been done when introduced rather than now when most
> > callers simply check for NULL as a failure.
> 
> I'm fixing up all the "NULL is a failure" callsites in the kernel, see
> lkml for the first round of those patches.

You are merely removing them, which doesn't really help for this patch.
And even if it did, you have marked this patch for stable which would
obviously depend on all such patches to be applied before.

-- 
Michal Hocko
SUSE Labs

Re: [PATCH 2/2] debugfs: return error values, not NULL

[Greg Kroah-Hartman]

On Wed, Jan 23, 2019 at 01:13:50PM +0100, Michal Hocko wrote:
> On Wed 23-01-19 12:55:35, Greg KH wrote:
> > On Wed, Jan 23, 2019 at 12:06:28PM +0100, Michal Hocko wrote:
> > > On Wed 23-01-19 11:28:14, Greg KH wrote:
> > > > When an error happens, debugfs should return an error pointer value, not
> > > > NULL.  This will prevent the totally theoretical error where a debugfs
> > > > call fails due to lack of memory, returning NULL, and that dentry value
> > > > is then passed to another debugfs call, which would end up succeeding,
> > > > creating a file at the root of the debugfs tree, but would then be
> > > > impossible to remove (because you can not remove the directory NULL).
> > > > 
> > > > So, to make everyone happy, always return errors, this makes the users
> > > > of debugfs much simpler (they do not have to ever check the return
> > > > value), and everyone can rest easy.
> > > 
> > > How come this is safe at all? Say you are creating a directory by
> > > debugfs_create_dir and then feed the return value to debugfs_create_files
> > > as a parent. In case of error you are giving it an invalid pointer and
> > > likely blow up unless I miss something.
> > 
> > debugfs_create_files checks for invalid parents and will just refuse to
> > create the file.  It's always done that.
> 
> I must be missing something because debugfs_create_files does
> 	d_inode(parent)->i_private = data;
> as the very first thing and that means that it dereferences an invalid
> pointer right there.

debugfs_create_file() -> __debugfs_create_file() -> start_creating()
and that function checks if parent is an error, which it aborts on, or
if it is NULL, it sets parent to a valid value:

        /* If the parent is not specified, we create it in the root.
         * We need the root dentry to do this, which is in the super
         * block. A pointer to that is in the struct vfsmount that we
         * have around.
         */
        if (!parent)
                parent = debugfs_mount->mnt_root;

I don't see any line that looks like:
>       d_inode(parent)->i_private = data;
in Linus's tree right now, what kernel version are you referring to?

> > > I do agree that reporting errors is better than a simple catch all NULL
> > > but this should have been done when introduced rather than now when most
> > > callers simply check for NULL as a failure.
> > 
> > I'm fixing up all the "NULL is a failure" callsites in the kernel, see
> > lkml for the first round of those patches.
> 
> You are merely removing them, which doesn't really help for this patch.

It doesn't hurt either, as if you really wanted to handle errors from debugfs
properly, you have to check for IS_ERR() as well, because the filesystem can be
compiled out (and then it returns an error pointer)

> And even if it did, you have marked this patch for stable which would
> obviously depend on all such patches to be applied before.

No, all should still work just fine.

thanks,

greg k-h

Re: [PATCH 2/2] debugfs: return error values, not NULL

[Michal Hocko]

On Wed 23-01-19 13:26:26, Greg KH wrote:
> On Wed, Jan 23, 2019 at 01:13:50PM +0100, Michal Hocko wrote:
> > On Wed 23-01-19 12:55:35, Greg KH wrote:
> > > On Wed, Jan 23, 2019 at 12:06:28PM +0100, Michal Hocko wrote:
> > > > On Wed 23-01-19 11:28:14, Greg KH wrote:
> > > > > When an error happens, debugfs should return an error pointer value, not
> > > > > NULL.  This will prevent the totally theoretical error where a debugfs
> > > > > call fails due to lack of memory, returning NULL, and that dentry value
> > > > > is then passed to another debugfs call, which would end up succeeding,
> > > > > creating a file at the root of the debugfs tree, but would then be
> > > > > impossible to remove (because you can not remove the directory NULL).
> > > > > 
> > > > > So, to make everyone happy, always return errors, this makes the users
> > > > > of debugfs much simpler (they do not have to ever check the return
> > > > > value), and everyone can rest easy.
> > > > 
> > > > How come this is safe at all? Say you are creating a directory by
> > > > debugfs_create_dir and then feed the return value to debugfs_create_files
> > > > as a parent. In case of error you are giving it an invalid pointer and
> > > > likely blow up unless I miss something.
> > > 
> > > debugfs_create_files checks for invalid parents and will just refuse to
> > > create the file.  It's always done that.
> > 
> > I must be missing something because debugfs_create_files does
> > 	d_inode(parent)->i_private = data;
> > as the very first thing and that means that it dereferences an invalid
> > pointer right there.
> 
> debugfs_create_file() -> __debugfs_create_file() -> start_creating()
> and that function checks if parent is an error, which it aborts on, or
> if it is NULL, it sets parent to a valid value:
> 
>         /* If the parent is not specified, we create it in the root.
>          * We need the root dentry to do this, which is in the super
>          * block. A pointer to that is in the struct vfsmount that we
>          * have around.
>          */
>         if (!parent)
>                 parent = debugfs_mount->mnt_root;
> 
> I don't see any line that looks like:
> >       d_inode(parent)->i_private = data;
> in Linus's tree right now, what kernel version are you referring to?

Ohh, my bad. I have looked at debugfs_create_files which is a mq helper
around debugfs_create_file. But that is a good example why this patch is
dangerous anyway. blk_mq_debugfs_register simply checks for NULL and
debugfs_create_files doesn't expect ERR_PTR here. So you would have to
check each and every user to make sure you can do that.

> > > > I do agree that reporting errors is better than a simple catch all NULL
> > > > but this should have been done when introduced rather than now when most
> > > > callers simply check for NULL as a failure.
> > > 
> > > I'm fixing up all the "NULL is a failure" callsites in the kernel, see
> > > lkml for the first round of those patches.
> > 
> > You are merely removing them, which doesn't really help for this patch.
> 
> It doesn't hurt either, as if you really wanted to handle errors from debugfs
> properly, you have to check for IS_ERR() as well, because the filesystem can be
> compiled out (and then it returns an error pointer)

I would assume that this would be achieved by a direct config
dependency. E.g. BLK_DEBUG_FS. So the code doesn't even get compiled and
wouldn't ever encounter the ERR_PTR.

Yeah this whole debugfs API is broken and it would take a lot of time to
unclutter it but it definitely is not that simple as what this patch
does.
-- 
Michal Hocko
SUSE Labs

Re: [PATCH 2/2] debugfs: return error values, not NULL

[Greg Kroah-Hartman]

On Wed, Jan 23, 2019 at 01:40:24PM +0100, Michal Hocko wrote:
> On Wed 23-01-19 13:26:26, Greg KH wrote:
> > On Wed, Jan 23, 2019 at 01:13:50PM +0100, Michal Hocko wrote:
> > > On Wed 23-01-19 12:55:35, Greg KH wrote:
> > > > On Wed, Jan 23, 2019 at 12:06:28PM +0100, Michal Hocko wrote:
> > > > > On Wed 23-01-19 11:28:14, Greg KH wrote:
> > > > > > When an error happens, debugfs should return an error pointer value, not
> > > > > > NULL.  This will prevent the totally theoretical error where a debugfs
> > > > > > call fails due to lack of memory, returning NULL, and that dentry value
> > > > > > is then passed to another debugfs call, which would end up succeeding,
> > > > > > creating a file at the root of the debugfs tree, but would then be
> > > > > > impossible to remove (because you can not remove the directory NULL).
> > > > > > 
> > > > > > So, to make everyone happy, always return errors, this makes the users
> > > > > > of debugfs much simpler (they do not have to ever check the return
> > > > > > value), and everyone can rest easy.
> > > > > 
> > > > > How come this is safe at all? Say you are creating a directory by
> > > > > debugfs_create_dir and then feed the return value to debugfs_create_files
> > > > > as a parent. In case of error you are giving it an invalid pointer and
> > > > > likely blow up unless I miss something.
> > > > 
> > > > debugfs_create_files checks for invalid parents and will just refuse to
> > > > create the file.  It's always done that.
> > > 
> > > I must be missing something because debugfs_create_files does
> > > 	d_inode(parent)->i_private = data;
> > > as the very first thing and that means that it dereferences an invalid
> > > pointer right there.
> > 
> > debugfs_create_file() -> __debugfs_create_file() -> start_creating()
> > and that function checks if parent is an error, which it aborts on, or
> > if it is NULL, it sets parent to a valid value:
> > 
> >         /* If the parent is not specified, we create it in the root.
> >          * We need the root dentry to do this, which is in the super
> >          * block. A pointer to that is in the struct vfsmount that we
> >          * have around.
> >          */
> >         if (!parent)
> >                 parent = debugfs_mount->mnt_root;
> > 
> > I don't see any line that looks like:
> > >       d_inode(parent)->i_private = data;
> > in Linus's tree right now, what kernel version are you referring to?
> 
> Ohh, my bad. I have looked at debugfs_create_files which is a mq helper
> around debugfs_create_file. But that is a good example why this patch is
> dangerous anyway. blk_mq_debugfs_register simply checks for NULL and
> debugfs_create_files doesn't expect ERR_PTR here. So you would have to
> check each and every user to make sure you can do that.

Ah, I already have that patch in my "to add a proper changelog" queue,
it's below and fixes that problem.

Might as well just send the "don't do that with a dentry" portion right
now, as that's not a good thing to be doing no matter what.

thanks,

greg k-h

From 62794189261d5df4ef0d37e4b8172d1c85d0c8df Mon Sep 17 00:00:00 2001
From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Date: Fri, 4 Jan 2019 14:06:22 +0100
Subject: [PATCH 108/119] blk-mq: fix changelog

---
 block/blk-mq-debugfs.c | 149 +++++++++--------------------------------
 block/blk-mq-debugfs.h |  36 +++++-----
 2 files changed, 48 insertions(+), 137 deletions(-)

diff --git a/block/blk-mq-debugfs.c b/block/blk-mq-debugfs.c
index 90d68760af08..39ff9f82278d 100644
--- a/block/blk-mq-debugfs.c
+++ b/block/blk-mq-debugfs.c
@@ -835,35 +835,28 @@ static const struct blk_mq_debugfs_attr blk_mq_debugfs_ctx_attrs[] = {
 	{},
 };
 
-static bool debugfs_create_files(struct dentry *parent, void *data,
+static void debugfs_create_files(struct dentry *parent, void *data,
 				 const struct blk_mq_debugfs_attr *attr)
 {
+	if (IS_ERR_OR_NULL(parent))
+		return;
+
 	d_inode(parent)->i_private = data;
 
-	for (; attr->name; attr++) {
-		if (!debugfs_create_file(attr->name, attr->mode, parent,
-					 (void *)attr, &blk_mq_debugfs_fops))
-			return false;
-	}
-	return true;
+	for (; attr->name; attr++)
+		debugfs_create_file(attr->name, attr->mode, parent,
+				    (void *)attr, &blk_mq_debugfs_fops);
 }
 
-int blk_mq_debugfs_register(struct request_queue *q)
+void blk_mq_debugfs_register(struct request_queue *q)
 {
 	struct blk_mq_hw_ctx *hctx;
 	int i;
 
-	if (!blk_debugfs_root)
-		return -ENOENT;
-
 	q->debugfs_dir = debugfs_create_dir(kobject_name(q->kobj.parent),
 					    blk_debugfs_root);
-	if (!q->debugfs_dir)
-		return -ENOMEM;
 
-	if (!debugfs_create_files(q->debugfs_dir, q,
-				  blk_mq_debugfs_queue_attrs))
-		goto err;
+	debugfs_create_files(q->debugfs_dir, q, blk_mq_debugfs_queue_attrs);
 
 	/*
 	 * blk_mq_init_sched() attempted to do this already, but q->debugfs_dir
@@ -875,11 +868,10 @@ int blk_mq_debugfs_register(struct request_queue *q)
 
 	/* Similarly, blk_mq_init_hctx() couldn't do this previously. */
 	queue_for_each_hw_ctx(q, hctx, i) {
-		if (!hctx->debugfs_dir && blk_mq_debugfs_register_hctx(q, hctx))
-			goto err;
-		if (q->elevator && !hctx->sched_debugfs_dir &&
-		    blk_mq_debugfs_register_sched_hctx(q, hctx))
-			goto err;
+		if (!hctx->debugfs_dir)
+			blk_mq_debugfs_register_hctx(q, hctx);
+		if (q->elevator && !hctx->sched_debugfs_dir)
+			blk_mq_debugfs_register_sched_hctx(q, hctx);
 	}
 
 	if (q->rq_qos) {
@@ -890,12 +882,6 @@ int blk_mq_debugfs_register(struct request_queue *q)
 			rqos = rqos->next;
 		}
 	}
-
-	return 0;
-
-err:
-	blk_mq_debugfs_unregister(q);
-	return -ENOMEM;
 }
 
 void blk_mq_debugfs_unregister(struct request_queue *q)
@@ -905,52 +891,32 @@ void blk_mq_debugfs_unregister(struct request_queue *q)
 	q->debugfs_dir = NULL;
 }
 
-static int blk_mq_debugfs_register_ctx(struct blk_mq_hw_ctx *hctx,
-				       struct blk_mq_ctx *ctx)
+static void blk_mq_debugfs_register_ctx(struct blk_mq_hw_ctx *hctx,
+					struct blk_mq_ctx *ctx)
 {
 	struct dentry *ctx_dir;
 	char name[20];
 
 	snprintf(name, sizeof(name), "cpu%u", ctx->cpu);
 	ctx_dir = debugfs_create_dir(name, hctx->debugfs_dir);
-	if (!ctx_dir)
-		return -ENOMEM;
 
-	if (!debugfs_create_files(ctx_dir, ctx, blk_mq_debugfs_ctx_attrs))
-		return -ENOMEM;
-
-	return 0;
+	debugfs_create_files(ctx_dir, ctx, blk_mq_debugfs_ctx_attrs);
 }
 
-int blk_mq_debugfs_register_hctx(struct request_queue *q,
-				 struct blk_mq_hw_ctx *hctx)
+void blk_mq_debugfs_register_hctx(struct request_queue *q,
+				  struct blk_mq_hw_ctx *hctx)
 {
 	struct blk_mq_ctx *ctx;
 	char name[20];
 	int i;
 
-	if (!q->debugfs_dir)
-		return -ENOENT;
-
 	snprintf(name, sizeof(name), "hctx%u", hctx->queue_num);
 	hctx->debugfs_dir = debugfs_create_dir(name, q->debugfs_dir);
-	if (!hctx->debugfs_dir)
-		return -ENOMEM;
 
-	if (!debugfs_create_files(hctx->debugfs_dir, hctx,
-				  blk_mq_debugfs_hctx_attrs))
-		goto err;
+	debugfs_create_files(hctx->debugfs_dir, hctx, blk_mq_debugfs_hctx_attrs);
 
-	hctx_for_each_ctx(hctx, ctx, i) {
-		if (blk_mq_debugfs_register_ctx(hctx, ctx))
-			goto err;
-	}
-
-	return 0;
-
-err:
-	blk_mq_debugfs_unregister_hctx(hctx);
-	return -ENOMEM;
+	hctx_for_each_ctx(hctx, ctx, i)
+		blk_mq_debugfs_register_ctx(hctx, ctx);
 }
 
 void blk_mq_debugfs_unregister_hctx(struct blk_mq_hw_ctx *hctx)
@@ -960,17 +926,13 @@ void blk_mq_debugfs_unregister_hctx(struct blk_mq_hw_ctx *hctx)
 	hctx->debugfs_dir = NULL;
 }
 
-int blk_mq_debugfs_register_hctxs(struct request_queue *q)
+void blk_mq_debugfs_register_hctxs(struct request_queue *q)
 {
 	struct blk_mq_hw_ctx *hctx;
 	int i;
 
-	queue_for_each_hw_ctx(q, hctx, i) {
-		if (blk_mq_debugfs_register_hctx(q, hctx))
-			return -ENOMEM;
-	}
-
-	return 0;
+	queue_for_each_hw_ctx(q, hctx, i)
+		blk_mq_debugfs_register_hctx(q, hctx);
 }
 
 void blk_mq_debugfs_unregister_hctxs(struct request_queue *q)
@@ -982,29 +944,13 @@ void blk_mq_debugfs_unregister_hctxs(struct request_queue *q)
 		blk_mq_debugfs_unregister_hctx(hctx);
 }
 
-int blk_mq_debugfs_register_sched(struct request_queue *q)
+void blk_mq_debugfs_register_sched(struct request_queue *q)
 {
 	struct elevator_type *e = q->elevator->type;
 
-	if (!q->debugfs_dir)
-		return -ENOENT;
-
-	if (!e->queue_debugfs_attrs)
-		return 0;
-
 	q->sched_debugfs_dir = debugfs_create_dir("sched", q->debugfs_dir);
-	if (!q->sched_debugfs_dir)
-		return -ENOMEM;
 
-	if (!debugfs_create_files(q->sched_debugfs_dir, q,
-				  e->queue_debugfs_attrs))
-		goto err;
-
-	return 0;
-
-err:
-	blk_mq_debugfs_unregister_sched(q);
-	return -ENOMEM;
+	debugfs_create_files(q->sched_debugfs_dir, q, e->queue_debugfs_attrs);
 }
 
 void blk_mq_debugfs_unregister_sched(struct request_queue *q)
@@ -1019,36 +965,19 @@ void blk_mq_debugfs_unregister_rqos(struct rq_qos *rqos)
 	rqos->debugfs_dir = NULL;
 }
 
-int blk_mq_debugfs_register_rqos(struct rq_qos *rqos)
+void blk_mq_debugfs_register_rqos(struct rq_qos *rqos)
 {
 	struct request_queue *q = rqos->q;
 	const char *dir_name = rq_qos_id_to_name(rqos->id);
 
-	if (!q->debugfs_dir)
-		return -ENOENT;
-
-	if (rqos->debugfs_dir || !rqos->ops->debugfs_attrs)
-		return 0;
-
-	if (!q->rqos_debugfs_dir) {
+	if (!q->rqos_debugfs_dir)
 		q->rqos_debugfs_dir = debugfs_create_dir("rqos",
 							 q->debugfs_dir);
-		if (!q->rqos_debugfs_dir)
-			return -ENOMEM;
-	}
 
 	rqos->debugfs_dir = debugfs_create_dir(dir_name,
 					       rqos->q->rqos_debugfs_dir);
-	if (!rqos->debugfs_dir)
-		return -ENOMEM;
 
-	if (!debugfs_create_files(rqos->debugfs_dir, rqos,
-				  rqos->ops->debugfs_attrs))
-		goto err;
-	return 0;
- err:
-	blk_mq_debugfs_unregister_rqos(rqos);
-	return -ENOMEM;
+	debugfs_create_files(rqos->debugfs_dir, rqos, rqos->ops->debugfs_attrs);
 }
 
 void blk_mq_debugfs_unregister_queue_rqos(struct request_queue *q)
@@ -1057,27 +986,15 @@ void blk_mq_debugfs_unregister_queue_rqos(struct request_queue *q)
 	q->rqos_debugfs_dir = NULL;
 }
 
-int blk_mq_debugfs_register_sched_hctx(struct request_queue *q,
-				       struct blk_mq_hw_ctx *hctx)
+void blk_mq_debugfs_register_sched_hctx(struct request_queue *q,
+					struct blk_mq_hw_ctx *hctx)
 {
 	struct elevator_type *e = q->elevator->type;
 
-	if (!hctx->debugfs_dir)
-		return -ENOENT;
-
-	if (!e->hctx_debugfs_attrs)
-		return 0;
-
 	hctx->sched_debugfs_dir = debugfs_create_dir("sched",
 						     hctx->debugfs_dir);
-	if (!hctx->sched_debugfs_dir)
-		return -ENOMEM;
-
-	if (!debugfs_create_files(hctx->sched_debugfs_dir, hctx,
-				  e->hctx_debugfs_attrs))
-		return -ENOMEM;
-
-	return 0;
+	debugfs_create_files(hctx->sched_debugfs_dir, hctx,
+			     e->hctx_debugfs_attrs);
 }
 
 void blk_mq_debugfs_unregister_sched_hctx(struct blk_mq_hw_ctx *hctx)
diff --git a/block/blk-mq-debugfs.h b/block/blk-mq-debugfs.h
index 8c9012a578c1..a68aa6041a10 100644
--- a/block/blk-mq-debugfs.h
+++ b/block/blk-mq-debugfs.h
@@ -18,74 +18,68 @@ struct blk_mq_debugfs_attr {
 int __blk_mq_debugfs_rq_show(struct seq_file *m, struct request *rq);
 int blk_mq_debugfs_rq_show(struct seq_file *m, void *v);
 
-int blk_mq_debugfs_register(struct request_queue *q);
+void blk_mq_debugfs_register(struct request_queue *q);
 void blk_mq_debugfs_unregister(struct request_queue *q);
-int blk_mq_debugfs_register_hctx(struct request_queue *q,
-				 struct blk_mq_hw_ctx *hctx);
+void blk_mq_debugfs_register_hctx(struct request_queue *q,
+				  struct blk_mq_hw_ctx *hctx);
 void blk_mq_debugfs_unregister_hctx(struct blk_mq_hw_ctx *hctx);
-int blk_mq_debugfs_register_hctxs(struct request_queue *q);
+void blk_mq_debugfs_register_hctxs(struct request_queue *q);
 void blk_mq_debugfs_unregister_hctxs(struct request_queue *q);
 
-int blk_mq_debugfs_register_sched(struct request_queue *q);
+void blk_mq_debugfs_register_sched(struct request_queue *q);
 void blk_mq_debugfs_unregister_sched(struct request_queue *q);
-int blk_mq_debugfs_register_sched_hctx(struct request_queue *q,
+void blk_mq_debugfs_register_sched_hctx(struct request_queue *q,
 				       struct blk_mq_hw_ctx *hctx);
 void blk_mq_debugfs_unregister_sched_hctx(struct blk_mq_hw_ctx *hctx);
 
-int blk_mq_debugfs_register_rqos(struct rq_qos *rqos);
+void blk_mq_debugfs_register_rqos(struct rq_qos *rqos);
 void blk_mq_debugfs_unregister_rqos(struct rq_qos *rqos);
 void blk_mq_debugfs_unregister_queue_rqos(struct request_queue *q);
 #else
-static inline int blk_mq_debugfs_register(struct request_queue *q)
+static inline void blk_mq_debugfs_register(struct request_queue *q)
 {
-	return 0;
 }
 
 static inline void blk_mq_debugfs_unregister(struct request_queue *q)
 {
 }
 
-static inline int blk_mq_debugfs_register_hctx(struct request_queue *q,
-					       struct blk_mq_hw_ctx *hctx)
+static inline void blk_mq_debugfs_register_hctx(struct request_queue *q,
+						struct blk_mq_hw_ctx *hctx)
 {
-	return 0;
 }
 
 static inline void blk_mq_debugfs_unregister_hctx(struct blk_mq_hw_ctx *hctx)
 {
 }
 
-static inline int blk_mq_debugfs_register_hctxs(struct request_queue *q)
+static inline void blk_mq_debugfs_register_hctxs(struct request_queue *q)
 {
-	return 0;
 }
 
 static inline void blk_mq_debugfs_unregister_hctxs(struct request_queue *q)
 {
 }
 
-static inline int blk_mq_debugfs_register_sched(struct request_queue *q)
+static inline void blk_mq_debugfs_register_sched(struct request_queue *q)
 {
-	return 0;
 }
 
 static inline void blk_mq_debugfs_unregister_sched(struct request_queue *q)
 {
 }
 
-static inline int blk_mq_debugfs_register_sched_hctx(struct request_queue *q,
-						     struct blk_mq_hw_ctx *hctx)
+static inline void blk_mq_debugfs_register_sched_hctx(struct request_queue *q,
+						      struct blk_mq_hw_ctx *hctx)
 {
-	return 0;
 }
 
 static inline void blk_mq_debugfs_unregister_sched_hctx(struct blk_mq_hw_ctx *hctx)
 {
 }
 
-static inline int blk_mq_debugfs_register_rqos(struct rq_qos *rqos)
+static inline void blk_mq_debugfs_register_rqos(struct rq_qos *rqos)
 {
-	return 0;
 }
 
 static inline void blk_mq_debugfs_unregister_rqos(struct rq_qos *rqos)
-- 
2.20.1

Re: [PATCH 2/2] debugfs: return error values, not NULL

[Michal Hocko]

On Wed 23-01-19 14:00:57, Greg KH wrote:
> On Wed, Jan 23, 2019 at 01:40:24PM +0100, Michal Hocko wrote:
> > On Wed 23-01-19 13:26:26, Greg KH wrote:
> > > On Wed, Jan 23, 2019 at 01:13:50PM +0100, Michal Hocko wrote:
> > > > On Wed 23-01-19 12:55:35, Greg KH wrote:
> > > > > On Wed, Jan 23, 2019 at 12:06:28PM +0100, Michal Hocko wrote:
> > > > > > On Wed 23-01-19 11:28:14, Greg KH wrote:
> > > > > > > When an error happens, debugfs should return an error pointer value, not
> > > > > > > NULL.  This will prevent the totally theoretical error where a debugfs
> > > > > > > call fails due to lack of memory, returning NULL, and that dentry value
> > > > > > > is then passed to another debugfs call, which would end up succeeding,
> > > > > > > creating a file at the root of the debugfs tree, but would then be
> > > > > > > impossible to remove (because you can not remove the directory NULL).
> > > > > > > 
> > > > > > > So, to make everyone happy, always return errors, this makes the users
> > > > > > > of debugfs much simpler (they do not have to ever check the return
> > > > > > > value), and everyone can rest easy.
> > > > > > 
> > > > > > How come this is safe at all? Say you are creating a directory by
> > > > > > debugfs_create_dir and then feed the return value to debugfs_create_files
> > > > > > as a parent. In case of error you are giving it an invalid pointer and
> > > > > > likely blow up unless I miss something.
> > > > > 
> > > > > debugfs_create_files checks for invalid parents and will just refuse to
> > > > > create the file.  It's always done that.
> > > > 
> > > > I must be missing something because debugfs_create_files does
> > > > 	d_inode(parent)->i_private = data;
> > > > as the very first thing and that means that it dereferences an invalid
> > > > pointer right there.
> > > 
> > > debugfs_create_file() -> __debugfs_create_file() -> start_creating()
> > > and that function checks if parent is an error, which it aborts on, or
> > > if it is NULL, it sets parent to a valid value:
> > > 
> > >         /* If the parent is not specified, we create it in the root.
> > >          * We need the root dentry to do this, which is in the super
> > >          * block. A pointer to that is in the struct vfsmount that we
> > >          * have around.
> > >          */
> > >         if (!parent)
> > >                 parent = debugfs_mount->mnt_root;
> > > 
> > > I don't see any line that looks like:
> > > >       d_inode(parent)->i_private = data;
> > > in Linus's tree right now, what kernel version are you referring to?
> > 
> > Ohh, my bad. I have looked at debugfs_create_files which is a mq helper
> > around debugfs_create_file. But that is a good example why this patch is
> > dangerous anyway. blk_mq_debugfs_register simply checks for NULL and
> > debugfs_create_files doesn't expect ERR_PTR here. So you would have to
> > check each and every user to make sure you can do that.
> 
> Ah, I already have that patch in my "to add a proper changelog" queue,
> it's below and fixes that problem.

OK, fair enough. I am just wondering how many more gems like that are
lurking there. Do not get me wrong but a broken error handling is rarely
fixed by removing it.

And Cc: stable is completely inappropriate IMNSHO. This is just adding a
risk without a large benefit.

Moreover all these changes should be posted in a single patch thread so
that everybody can see the final outcome.
-- 
Michal Hocko
SUSE Labs

Re: [PATCH 2/2] debugfs: return error values, not NULL

[Greg Kroah-Hartman]

On Wed, Jan 23, 2019 at 02:09:17PM +0100, Michal Hocko wrote:
> On Wed 23-01-19 14:00:57, Greg KH wrote:
> > On Wed, Jan 23, 2019 at 01:40:24PM +0100, Michal Hocko wrote:
> > > On Wed 23-01-19 13:26:26, Greg KH wrote:
> > > > On Wed, Jan 23, 2019 at 01:13:50PM +0100, Michal Hocko wrote:
> > > > > On Wed 23-01-19 12:55:35, Greg KH wrote:
> > > > > > On Wed, Jan 23, 2019 at 12:06:28PM +0100, Michal Hocko wrote:
> > > > > > > On Wed 23-01-19 11:28:14, Greg KH wrote:
> > > > > > > > When an error happens, debugfs should return an error pointer value, not
> > > > > > > > NULL.  This will prevent the totally theoretical error where a debugfs
> > > > > > > > call fails due to lack of memory, returning NULL, and that dentry value
> > > > > > > > is then passed to another debugfs call, which would end up succeeding,
> > > > > > > > creating a file at the root of the debugfs tree, but would then be
> > > > > > > > impossible to remove (because you can not remove the directory NULL).
> > > > > > > > 
> > > > > > > > So, to make everyone happy, always return errors, this makes the users
> > > > > > > > of debugfs much simpler (they do not have to ever check the return
> > > > > > > > value), and everyone can rest easy.
> > > > > > > 
> > > > > > > How come this is safe at all? Say you are creating a directory by
> > > > > > > debugfs_create_dir and then feed the return value to debugfs_create_files
> > > > > > > as a parent. In case of error you are giving it an invalid pointer and
> > > > > > > likely blow up unless I miss something.
> > > > > > 
> > > > > > debugfs_create_files checks for invalid parents and will just refuse to
> > > > > > create the file.  It's always done that.
> > > > > 
> > > > > I must be missing something because debugfs_create_files does
> > > > > 	d_inode(parent)->i_private = data;
> > > > > as the very first thing and that means that it dereferences an invalid
> > > > > pointer right there.
> > > > 
> > > > debugfs_create_file() -> __debugfs_create_file() -> start_creating()
> > > > and that function checks if parent is an error, which it aborts on, or
> > > > if it is NULL, it sets parent to a valid value:
> > > > 
> > > >         /* If the parent is not specified, we create it in the root.
> > > >          * We need the root dentry to do this, which is in the super
> > > >          * block. A pointer to that is in the struct vfsmount that we
> > > >          * have around.
> > > >          */
> > > >         if (!parent)
> > > >                 parent = debugfs_mount->mnt_root;
> > > > 
> > > > I don't see any line that looks like:
> > > > >       d_inode(parent)->i_private = data;
> > > > in Linus's tree right now, what kernel version are you referring to?
> > > 
> > > Ohh, my bad. I have looked at debugfs_create_files which is a mq helper
> > > around debugfs_create_file. But that is a good example why this patch is
> > > dangerous anyway. blk_mq_debugfs_register simply checks for NULL and
> > > debugfs_create_files doesn't expect ERR_PTR here. So you would have to
> > > check each and every user to make sure you can do that.
> > 
> > Ah, I already have that patch in my "to add a proper changelog" queue,
> > it's below and fixes that problem.
> 
> OK, fair enough. I am just wondering how many more gems like that are
> lurking there. Do not get me wrong but a broken error handling is rarely
> fixed by removing it.

I think you all are the "lucky" one here :)

I did audit the whole kernel tree already, with the exception of the gpu
drivers, as they are even more insane than block drivers...

> And Cc: stable is completely inappropriate IMNSHO. This is just adding a
> risk without a large benefit.

What risk?  Again, the _ONLY_ way that NULL is returned here is if you
are out of memory when you try to create that debugfs file/directory.
Given that we usually don't even fail small kmallocs, I doubt this can
even ever happen.

> Moreover all these changes should be posted in a single patch thread so
> that everybody can see the final outcome.

No one wants to see a 200+ patch thread and be cc:ed on them all.

thanks,

greg k-h

Re: [PATCH 2/2] debugfs: return error values, not NULL

[Greg Kroah-Hartman]

On Wed, Jan 23, 2019 at 02:40:21PM +0100, Greg Kroah-Hartman wrote:
> On Wed, Jan 23, 2019 at 02:09:17PM +0100, Michal Hocko wrote:
> > On Wed 23-01-19 14:00:57, Greg KH wrote:
> > > On Wed, Jan 23, 2019 at 01:40:24PM +0100, Michal Hocko wrote:
> > > > On Wed 23-01-19 13:26:26, Greg KH wrote:
> > > > > On Wed, Jan 23, 2019 at 01:13:50PM +0100, Michal Hocko wrote:
> > > > > > On Wed 23-01-19 12:55:35, Greg KH wrote:
> > > > > > > On Wed, Jan 23, 2019 at 12:06:28PM +0100, Michal Hocko wrote:
> > > > > > > > On Wed 23-01-19 11:28:14, Greg KH wrote:
> > > > > > > > > When an error happens, debugfs should return an error pointer value, not
> > > > > > > > > NULL.  This will prevent the totally theoretical error where a debugfs
> > > > > > > > > call fails due to lack of memory, returning NULL, and that dentry value
> > > > > > > > > is then passed to another debugfs call, which would end up succeeding,
> > > > > > > > > creating a file at the root of the debugfs tree, but would then be
> > > > > > > > > impossible to remove (because you can not remove the directory NULL).
> > > > > > > > > 
> > > > > > > > > So, to make everyone happy, always return errors, this makes the users
> > > > > > > > > of debugfs much simpler (they do not have to ever check the return
> > > > > > > > > value), and everyone can rest easy.
> > > > > > > > 
> > > > > > > > How come this is safe at all? Say you are creating a directory by
> > > > > > > > debugfs_create_dir and then feed the return value to debugfs_create_files
> > > > > > > > as a parent. In case of error you are giving it an invalid pointer and
> > > > > > > > likely blow up unless I miss something.
> > > > > > > 
> > > > > > > debugfs_create_files checks for invalid parents and will just refuse to
> > > > > > > create the file.  It's always done that.
> > > > > > 
> > > > > > I must be missing something because debugfs_create_files does
> > > > > > 	d_inode(parent)->i_private = data;
> > > > > > as the very first thing and that means that it dereferences an invalid
> > > > > > pointer right there.
> > > > > 
> > > > > debugfs_create_file() -> __debugfs_create_file() -> start_creating()
> > > > > and that function checks if parent is an error, which it aborts on, or
> > > > > if it is NULL, it sets parent to a valid value:
> > > > > 
> > > > >         /* If the parent is not specified, we create it in the root.
> > > > >          * We need the root dentry to do this, which is in the super
> > > > >          * block. A pointer to that is in the struct vfsmount that we
> > > > >          * have around.
> > > > >          */
> > > > >         if (!parent)
> > > > >                 parent = debugfs_mount->mnt_root;
> > > > > 
> > > > > I don't see any line that looks like:
> > > > > >       d_inode(parent)->i_private = data;
> > > > > in Linus's tree right now, what kernel version are you referring to?
> > > > 
> > > > Ohh, my bad. I have looked at debugfs_create_files which is a mq helper
> > > > around debugfs_create_file. But that is a good example why this patch is
> > > > dangerous anyway. blk_mq_debugfs_register simply checks for NULL and
> > > > debugfs_create_files doesn't expect ERR_PTR here. So you would have to
> > > > check each and every user to make sure you can do that.
> > > 
> > > Ah, I already have that patch in my "to add a proper changelog" queue,
> > > it's below and fixes that problem.
> > 
> > OK, fair enough. I am just wondering how many more gems like that are
> > lurking there. Do not get me wrong but a broken error handling is rarely
> > fixed by removing it.
> 
> I think you all are the "lucky" one here :)
> 
> I did audit the whole kernel tree already, with the exception of the gpu
> drivers, as they are even more insane than block drivers...
> 
> > And Cc: stable is completely inappropriate IMNSHO. This is just adding a
> > risk without a large benefit.
> 
> What risk?  Again, the _ONLY_ way that NULL is returned here is if you
> are out of memory when you try to create that debugfs file/directory.
> Given that we usually don't even fail small kmallocs, I doubt this can
> even ever happen.

In thinking about it some more, I'll not cc: stable on this, to give
people a chance to get synced up for 5.1 for all of this.

thanks,

greg k-h

Re: [PATCH 2/2] debugfs: return error values, not NULL

[Michal Hocko]

On Wed 23-01-19 14:40:21, Greg KH wrote:
> On Wed, Jan 23, 2019 at 02:09:17PM +0100, Michal Hocko wrote:
> > On Wed 23-01-19 14:00:57, Greg KH wrote:
> > > On Wed, Jan 23, 2019 at 01:40:24PM +0100, Michal Hocko wrote:
> > > > On Wed 23-01-19 13:26:26, Greg KH wrote:
> > > > > On Wed, Jan 23, 2019 at 01:13:50PM +0100, Michal Hocko wrote:
> > > > > > On Wed 23-01-19 12:55:35, Greg KH wrote:
> > > > > > > On Wed, Jan 23, 2019 at 12:06:28PM +0100, Michal Hocko wrote:
> > > > > > > > On Wed 23-01-19 11:28:14, Greg KH wrote:
> > > > > > > > > When an error happens, debugfs should return an error pointer value, not
> > > > > > > > > NULL.  This will prevent the totally theoretical error where a debugfs
> > > > > > > > > call fails due to lack of memory, returning NULL, and that dentry value
> > > > > > > > > is then passed to another debugfs call, which would end up succeeding,
> > > > > > > > > creating a file at the root of the debugfs tree, but would then be
> > > > > > > > > impossible to remove (because you can not remove the directory NULL).
> > > > > > > > > 
> > > > > > > > > So, to make everyone happy, always return errors, this makes the users
> > > > > > > > > of debugfs much simpler (they do not have to ever check the return
> > > > > > > > > value), and everyone can rest easy.
> > > > > > > > 
> > > > > > > > How come this is safe at all? Say you are creating a directory by
> > > > > > > > debugfs_create_dir and then feed the return value to debugfs_create_files
> > > > > > > > as a parent. In case of error you are giving it an invalid pointer and
> > > > > > > > likely blow up unless I miss something.
> > > > > > > 
> > > > > > > debugfs_create_files checks for invalid parents and will just refuse to
> > > > > > > create the file.  It's always done that.
> > > > > > 
> > > > > > I must be missing something because debugfs_create_files does
> > > > > > 	d_inode(parent)->i_private = data;
> > > > > > as the very first thing and that means that it dereferences an invalid
> > > > > > pointer right there.
> > > > > 
> > > > > debugfs_create_file() -> __debugfs_create_file() -> start_creating()
> > > > > and that function checks if parent is an error, which it aborts on, or
> > > > > if it is NULL, it sets parent to a valid value:
> > > > > 
> > > > >         /* If the parent is not specified, we create it in the root.
> > > > >          * We need the root dentry to do this, which is in the super
> > > > >          * block. A pointer to that is in the struct vfsmount that we
> > > > >          * have around.
> > > > >          */
> > > > >         if (!parent)
> > > > >                 parent = debugfs_mount->mnt_root;
> > > > > 
> > > > > I don't see any line that looks like:
> > > > > >       d_inode(parent)->i_private = data;
> > > > > in Linus's tree right now, what kernel version are you referring to?
> > > > 
> > > > Ohh, my bad. I have looked at debugfs_create_files which is a mq helper
> > > > around debugfs_create_file. But that is a good example why this patch is
> > > > dangerous anyway. blk_mq_debugfs_register simply checks for NULL and
> > > > debugfs_create_files doesn't expect ERR_PTR here. So you would have to
> > > > check each and every user to make sure you can do that.
> > > 
> > > Ah, I already have that patch in my "to add a proper changelog" queue,
> > > it's below and fixes that problem.
> > 
> > OK, fair enough. I am just wondering how many more gems like that are
> > lurking there. Do not get me wrong but a broken error handling is rarely
> > fixed by removing it.
> 
> I think you all are the "lucky" one here :)

That was actually the first random place I've checked.

> I did audit the whole kernel tree already, with the exception of the gpu
> drivers, as they are even more insane than block drivers...
> 
> > And Cc: stable is completely inappropriate IMNSHO. This is just adding a
> > risk without a large benefit.
> 
> What risk?  Again, the _ONLY_ way that NULL is returned here is if you
> are out of memory when you try to create that debugfs file/directory.
> Given that we usually don't even fail small kmallocs, I doubt this can
> even ever happen.

This doesn't matter at all. Failures are real even when rare. Do not
forget fault injection testing which is real on older kernels as well.

Besides that I haven't heard _any_ argument why this should be
backported to stable trees in the first place.
 
> > Moreover all these changes should be posted in a single patch thread so
> > that everybody can see the final outcome.
> 
> No one wants to see a 200+ patch thread and be cc:ed on them all.

I have seen some of them and they are really dubious, some even outright
wrong so this particular one is far far away from being ready to get
mereged even to the linus tree. Not to mention older trees which would
require the same kind of tree wide recheck again.
-- 
Michal Hocko
SUSE Labs

Re: [PATCH 2/2] debugfs: return error values, not NULL

[Sebastian Andrzej Siewior]

On 2019-01-23 11:28:14 [+0100], Greg Kroah-Hartman wrote:
> When an error happens, debugfs should return an error pointer value, not
> NULL.  This will prevent the totally theoretical error where a debugfs
> call fails due to lack of memory, returning NULL, and that dentry value
> is then passed to another debugfs call, which would end up succeeding,
> creating a file at the root of the debugfs tree, but would then be
> impossible to remove (because you can not remove the directory NULL).
> 
> So, to make everyone happy, always return errors, this makes the users
> of debugfs much simpler (they do not have to ever check the return
> value), and everyone can rest easy.

Thank you.

> Reported-by: Masami Hiramatsu <mhiramat@kernel.org>
> Reported-by: Ulf Hansson <ulf.hansson@linaro.org>
> Reported-by: Gary R Hook <ghook@amd.com>
> Reported-by: Heiko Carstens <heiko.carstens@de.ibm.com>
> Cc: stable <stable@vger.kernel.org>
> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Reviewed-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>

Sebastian

Re: [PATCH 2/2] debugfs: return error values, not NULL

[Masami Hiramatsu]

On Wed, 23 Jan 2019 11:28:14 +0100
Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote:

> When an error happens, debugfs should return an error pointer value, not
> NULL.  This will prevent the totally theoretical error where a debugfs
> call fails due to lack of memory, returning NULL, and that dentry value
> is then passed to another debugfs call, which would end up succeeding,
> creating a file at the root of the debugfs tree, but would then be
> impossible to remove (because you can not remove the directory NULL).
> 
> So, to make everyone happy, always return errors, this makes the users
> of debugfs much simpler (they do not have to ever check the return
> value), and everyone can rest easy.

With Greg's return check removal patches, I'm OK for this change.

Reviewed-by: Masami Hiramatsu <mhiramat@kernel.org>

Thank you,

> 
> Reported-by: Masami Hiramatsu <mhiramat@kernel.org>
> Reported-by: Ulf Hansson <ulf.hansson@linaro.org>
> Reported-by: Gary R Hook <ghook@amd.com>
> Reported-by: Heiko Carstens <heiko.carstens@de.ibm.com>
> Cc: stable <stable@vger.kernel.org>
> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> ---
>  fs/debugfs/inode.c | 39 ++++++++++++++++++++++-----------------
>  1 file changed, 22 insertions(+), 17 deletions(-)
> 
> diff --git a/fs/debugfs/inode.c b/fs/debugfs/inode.c
> index 41ef452c1fcf..b16f8035b1af 100644
> --- a/fs/debugfs/inode.c
> +++ b/fs/debugfs/inode.c
> @@ -254,8 +254,8 @@ MODULE_ALIAS_FS("debugfs");
>   * @parent: a pointer to the parent dentry of the file.
>   *
>   * This function will return a pointer to a dentry if it succeeds.  If the file
> - * doesn't exist or an error occurs, %NULL will be returned.  The returned
> - * dentry must be passed to dput() when it is no longer needed.
> + * doesn't exist or an error occurs, %ERR_PTR(-ERROR) will be returned.  The
> + * returned dentry must be passed to dput() when it is no longer needed.
>   *
>   * If debugfs is not enabled in the kernel, the value -%ENODEV will be
>   * returned.
> @@ -265,17 +265,17 @@ struct dentry *debugfs_lookup(const char *name, struct dentry *parent)
>  	struct dentry *dentry;
>  
>  	if (IS_ERR(parent))
> -		return NULL;
> +		return parent;
>  
>  	if (!parent)
>  		parent = debugfs_mount->mnt_root;
>  
>  	dentry = lookup_one_len_unlocked(name, parent, strlen(name));
>  	if (IS_ERR(dentry))
> -		return NULL;
> +		return dentry;
>  	if (!d_really_is_positive(dentry)) {
>  		dput(dentry);
> -		return NULL;
> +		return ERR_PTR(-EINVAL);
>  	}
>  	return dentry;
>  }
> @@ -324,7 +324,7 @@ static struct dentry *failed_creating(struct dentry *dentry)
>  	inode_unlock(d_inode(dentry->d_parent));
>  	dput(dentry);
>  	simple_release_fs(&debugfs_mount, &debugfs_mount_count);
> -	return NULL;
> +	return ERR_PTR(-ENOMEM);
>  }
>  
>  static struct dentry *end_creating(struct dentry *dentry)
> @@ -347,7 +347,7 @@ static struct dentry *__debugfs_create_file(const char *name, umode_t mode,
>  	dentry = start_creating(name, parent);
>  
>  	if (IS_ERR(dentry))
> -		return NULL;
> +		return dentry;
>  
>  	inode = debugfs_get_inode(dentry->d_sb);
>  	if (unlikely(!inode))
> @@ -386,7 +386,8 @@ static struct dentry *__debugfs_create_file(const char *name, umode_t mode,
>   * This function will return a pointer to a dentry if it succeeds.  This
>   * pointer must be passed to the debugfs_remove() function when the file is
>   * to be removed (no automatic cleanup happens if your module is unloaded,
> - * you are responsible here.)  If an error occurs, %NULL will be returned.
> + * you are responsible here.)  If an error occurs, %ERR_PTR(-ERROR) will be
> + * returned.
>   *
>   * If debugfs is not enabled in the kernel, the value -%ENODEV will be
>   * returned.
> @@ -464,7 +465,8 @@ EXPORT_SYMBOL_GPL(debugfs_create_file_unsafe);
>   * This function will return a pointer to a dentry if it succeeds.  This
>   * pointer must be passed to the debugfs_remove() function when the file is
>   * to be removed (no automatic cleanup happens if your module is unloaded,
> - * you are responsible here.)  If an error occurs, %NULL will be returned.
> + * you are responsible here.)  If an error occurs, %ERR_PTR(-ERROR) will be
> + * returned.
>   *
>   * If debugfs is not enabled in the kernel, the value -%ENODEV will be
>   * returned.
> @@ -495,7 +497,8 @@ EXPORT_SYMBOL_GPL(debugfs_create_file_size);
>   * This function will return a pointer to a dentry if it succeeds.  This
>   * pointer must be passed to the debugfs_remove() function when the file is
>   * to be removed (no automatic cleanup happens if your module is unloaded,
> - * you are responsible here.)  If an error occurs, %NULL will be returned.
> + * you are responsible here.)  If an error occurs, %ERR_PTR(-ERROR) will be
> + * returned.
>   *
>   * If debugfs is not enabled in the kernel, the value -%ENODEV will be
>   * returned.
> @@ -506,7 +509,7 @@ struct dentry *debugfs_create_dir(const char *name, struct dentry *parent)
>  	struct inode *inode;
>  
>  	if (IS_ERR(dentry))
> -		return NULL;
> +		return dentry;
>  
>  	inode = debugfs_get_inode(dentry->d_sb);
>  	if (unlikely(!inode))
> @@ -545,7 +548,7 @@ struct dentry *debugfs_create_automount(const char *name,
>  	struct inode *inode;
>  
>  	if (IS_ERR(dentry))
> -		return NULL;
> +		return dentry;
>  
>  	inode = debugfs_get_inode(dentry->d_sb);
>  	if (unlikely(!inode))
> @@ -581,8 +584,8 @@ EXPORT_SYMBOL(debugfs_create_automount);
>   * This function will return a pointer to a dentry if it succeeds.  This
>   * pointer must be passed to the debugfs_remove() function when the symbolic
>   * link is to be removed (no automatic cleanup happens if your module is
> - * unloaded, you are responsible here.)  If an error occurs, %NULL will be
> - * returned.
> + * unloaded, you are responsible here.)  If an error occurs, %ERR_PTR(-ERROR)
> + * will be returned.
>   *
>   * If debugfs is not enabled in the kernel, the value -%ENODEV will be
>   * returned.
> @@ -594,12 +597,12 @@ struct dentry *debugfs_create_symlink(const char *name, struct dentry *parent,
>  	struct inode *inode;
>  	char *link = kstrdup(target, GFP_KERNEL);
>  	if (!link)
> -		return NULL;
> +		return ERR_PTR(-ENOMEM);
>  
>  	dentry = start_creating(name, parent);
>  	if (IS_ERR(dentry)) {
>  		kfree(link);
> -		return NULL;
> +		return dentry;
>  	}
>  
>  	inode = debugfs_get_inode(dentry->d_sb);
> @@ -827,7 +830,9 @@ struct dentry *debugfs_rename(struct dentry *old_dir, struct dentry *old_dentry,
>  	if (dentry && !IS_ERR(dentry))
>  		dput(dentry);
>  	unlock_rename(new_dir, old_dir);
> -	return NULL;
> +	if (IS_ERR(dentry))
> +		return dentry;
> +	return ERR_PTR(-EINVAL);
>  }
>  EXPORT_SYMBOL_GPL(debugfs_rename);
>  
> -- 
> 2.20.1
> 


-- 
Masami Hiramatsu <mhiramat@kernel.org>

Re: [PATCH 2/2] debugfs: return error values, not NULL

[Masami Hiramatsu]

Hi Greg,

On Thu, 24 Jan 2019 11:26:52 +0900
Masami Hiramatsu <mhiramat@kernel.org> wrote:

> On Wed, 23 Jan 2019 11:28:14 +0100
> Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote:
> 
> > When an error happens, debugfs should return an error pointer value, not
> > NULL.  This will prevent the totally theoretical error where a debugfs
> > call fails due to lack of memory, returning NULL, and that dentry value
> > is then passed to another debugfs call, which would end up succeeding,
> > creating a file at the root of the debugfs tree, but would then be
> > impossible to remove (because you can not remove the directory NULL).
> > 
> > So, to make everyone happy, always return errors, this makes the users
> > of debugfs much simpler (they do not have to ever check the return
> > value), and everyone can rest easy.
> 
> With Greg's return check removal patches, I'm OK for this change.
> 
> Reviewed-by: Masami Hiramatsu <mhiramat@kernel.org>
> 
> Thank you,
> 
> > 
> > Reported-by: Masami Hiramatsu <mhiramat@kernel.org>
> > Reported-by: Ulf Hansson <ulf.hansson@linaro.org>
> > Reported-by: Gary R Hook <ghook@amd.com>
> > Reported-by: Heiko Carstens <heiko.carstens@de.ibm.com>
> > Cc: stable <stable@vger.kernel.org>

BTW, would you really think it should go to stable? It seems an improvement
instead of a bugfix...

Thank you,

> > Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > ---
> >  fs/debugfs/inode.c | 39 ++++++++++++++++++++++-----------------
> >  1 file changed, 22 insertions(+), 17 deletions(-)
> > 
> > diff --git a/fs/debugfs/inode.c b/fs/debugfs/inode.c
> > index 41ef452c1fcf..b16f8035b1af 100644
> > --- a/fs/debugfs/inode.c
> > +++ b/fs/debugfs/inode.c
> > @@ -254,8 +254,8 @@ MODULE_ALIAS_FS("debugfs");
> >   * @parent: a pointer to the parent dentry of the file.
> >   *
> >   * This function will return a pointer to a dentry if it succeeds.  If the file
> > - * doesn't exist or an error occurs, %NULL will be returned.  The returned
> > - * dentry must be passed to dput() when it is no longer needed.
> > + * doesn't exist or an error occurs, %ERR_PTR(-ERROR) will be returned.  The
> > + * returned dentry must be passed to dput() when it is no longer needed.
> >   *
> >   * If debugfs is not enabled in the kernel, the value -%ENODEV will be
> >   * returned.
> > @@ -265,17 +265,17 @@ struct dentry *debugfs_lookup(const char *name, struct dentry *parent)
> >  	struct dentry *dentry;
> >  
> >  	if (IS_ERR(parent))
> > -		return NULL;
> > +		return parent;
> >  
> >  	if (!parent)
> >  		parent = debugfs_mount->mnt_root;
> >  
> >  	dentry = lookup_one_len_unlocked(name, parent, strlen(name));
> >  	if (IS_ERR(dentry))
> > -		return NULL;
> > +		return dentry;
> >  	if (!d_really_is_positive(dentry)) {
> >  		dput(dentry);
> > -		return NULL;
> > +		return ERR_PTR(-EINVAL);
> >  	}
> >  	return dentry;
> >  }
> > @@ -324,7 +324,7 @@ static struct dentry *failed_creating(struct dentry *dentry)
> >  	inode_unlock(d_inode(dentry->d_parent));
> >  	dput(dentry);
> >  	simple_release_fs(&debugfs_mount, &debugfs_mount_count);
> > -	return NULL;
> > +	return ERR_PTR(-ENOMEM);
> >  }
> >  
> >  static struct dentry *end_creating(struct dentry *dentry)
> > @@ -347,7 +347,7 @@ static struct dentry *__debugfs_create_file(const char *name, umode_t mode,
> >  	dentry = start_creating(name, parent);
> >  
> >  	if (IS_ERR(dentry))
> > -		return NULL;
> > +		return dentry;
> >  
> >  	inode = debugfs_get_inode(dentry->d_sb);
> >  	if (unlikely(!inode))
> > @@ -386,7 +386,8 @@ static struct dentry *__debugfs_create_file(const char *name, umode_t mode,
> >   * This function will return a pointer to a dentry if it succeeds.  This
> >   * pointer must be passed to the debugfs_remove() function when the file is
> >   * to be removed (no automatic cleanup happens if your module is unloaded,
> > - * you are responsible here.)  If an error occurs, %NULL will be returned.
> > + * you are responsible here.)  If an error occurs, %ERR_PTR(-ERROR) will be
> > + * returned.
> >   *
> >   * If debugfs is not enabled in the kernel, the value -%ENODEV will be
> >   * returned.
> > @@ -464,7 +465,8 @@ EXPORT_SYMBOL_GPL(debugfs_create_file_unsafe);
> >   * This function will return a pointer to a dentry if it succeeds.  This
> >   * pointer must be passed to the debugfs_remove() function when the file is
> >   * to be removed (no automatic cleanup happens if your module is unloaded,
> > - * you are responsible here.)  If an error occurs, %NULL will be returned.
> > + * you are responsible here.)  If an error occurs, %ERR_PTR(-ERROR) will be
> > + * returned.
> >   *
> >   * If debugfs is not enabled in the kernel, the value -%ENODEV will be
> >   * returned.
> > @@ -495,7 +497,8 @@ EXPORT_SYMBOL_GPL(debugfs_create_file_size);
> >   * This function will return a pointer to a dentry if it succeeds.  This
> >   * pointer must be passed to the debugfs_remove() function when the file is
> >   * to be removed (no automatic cleanup happens if your module is unloaded,
> > - * you are responsible here.)  If an error occurs, %NULL will be returned.
> > + * you are responsible here.)  If an error occurs, %ERR_PTR(-ERROR) will be
> > + * returned.
> >   *
> >   * If debugfs is not enabled in the kernel, the value -%ENODEV will be
> >   * returned.
> > @@ -506,7 +509,7 @@ struct dentry *debugfs_create_dir(const char *name, struct dentry *parent)
> >  	struct inode *inode;
> >  
> >  	if (IS_ERR(dentry))
> > -		return NULL;
> > +		return dentry;
> >  
> >  	inode = debugfs_get_inode(dentry->d_sb);
> >  	if (unlikely(!inode))
> > @@ -545,7 +548,7 @@ struct dentry *debugfs_create_automount(const char *name,
> >  	struct inode *inode;
> >  
> >  	if (IS_ERR(dentry))
> > -		return NULL;
> > +		return dentry;
> >  
> >  	inode = debugfs_get_inode(dentry->d_sb);
> >  	if (unlikely(!inode))
> > @@ -581,8 +584,8 @@ EXPORT_SYMBOL(debugfs_create_automount);
> >   * This function will return a pointer to a dentry if it succeeds.  This
> >   * pointer must be passed to the debugfs_remove() function when the symbolic
> >   * link is to be removed (no automatic cleanup happens if your module is
> > - * unloaded, you are responsible here.)  If an error occurs, %NULL will be
> > - * returned.
> > + * unloaded, you are responsible here.)  If an error occurs, %ERR_PTR(-ERROR)
> > + * will be returned.
> >   *
> >   * If debugfs is not enabled in the kernel, the value -%ENODEV will be
> >   * returned.
> > @@ -594,12 +597,12 @@ struct dentry *debugfs_create_symlink(const char *name, struct dentry *parent,
> >  	struct inode *inode;
> >  	char *link = kstrdup(target, GFP_KERNEL);
> >  	if (!link)
> > -		return NULL;
> > +		return ERR_PTR(-ENOMEM);
> >  
> >  	dentry = start_creating(name, parent);
> >  	if (IS_ERR(dentry)) {
> >  		kfree(link);
> > -		return NULL;
> > +		return dentry;
> >  	}
> >  
> >  	inode = debugfs_get_inode(dentry->d_sb);
> > @@ -827,7 +830,9 @@ struct dentry *debugfs_rename(struct dentry *old_dir, struct dentry *old_dentry,
> >  	if (dentry && !IS_ERR(dentry))
> >  		dput(dentry);
> >  	unlock_rename(new_dir, old_dir);
> > -	return NULL;
> > +	if (IS_ERR(dentry))
> > +		return dentry;
> > +	return ERR_PTR(-EINVAL);
> >  }
> >  EXPORT_SYMBOL_GPL(debugfs_rename);
> >  
> > -- 
> > 2.20.1
> > 
> 
> 
> -- 
> Masami Hiramatsu <mhiramat@kernel.org>


-- 
Masami Hiramatsu <mhiramat@kernel.org>

Re: [PATCH 2/2] debugfs: return error values, not NULL

[Greg Kroah-Hartman]

On Mon, Jan 28, 2019 at 10:55:13PM +0900, Masami Hiramatsu wrote:
> Hi Greg,
> 
> On Thu, 24 Jan 2019 11:26:52 +0900
> Masami Hiramatsu <mhiramat@kernel.org> wrote:
> 
> > On Wed, 23 Jan 2019 11:28:14 +0100
> > Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote:
> > 
> > > When an error happens, debugfs should return an error pointer value, not
> > > NULL.  This will prevent the totally theoretical error where a debugfs
> > > call fails due to lack of memory, returning NULL, and that dentry value
> > > is then passed to another debugfs call, which would end up succeeding,
> > > creating a file at the root of the debugfs tree, but would then be
> > > impossible to remove (because you can not remove the directory NULL).
> > > 
> > > So, to make everyone happy, always return errors, this makes the users
> > > of debugfs much simpler (they do not have to ever check the return
> > > value), and everyone can rest easy.
> > 
> > With Greg's return check removal patches, I'm OK for this change.
> > 
> > Reviewed-by: Masami Hiramatsu <mhiramat@kernel.org>
> > 
> > Thank you,
> > 
> > > 
> > > Reported-by: Masami Hiramatsu <mhiramat@kernel.org>
> > > Reported-by: Ulf Hansson <ulf.hansson@linaro.org>
> > > Reported-by: Gary R Hook <ghook@amd.com>
> > > Reported-by: Heiko Carstens <heiko.carstens@de.ibm.com>
> > > Cc: stable <stable@vger.kernel.org>
> 
> BTW, would you really think it should go to stable? It seems an improvement
> instead of a bugfix...

See later in the thread, I decided that was not the correct thing to do
:)

thanks,

greg k-h