Messages in this thread | | | Date | Thu, 24 Jan 2019 11:26:52 +0900 | From | Masami Hiramatsu <> | Subject | Re: [PATCH 2/2] debugfs: return error values, not NULL |
| |
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>
| |