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

[Michal Hocko <mhocko@kernel.org>]

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