| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-Id: <20190124112652.f31fc4b8dd9892f0d2f84790@kernel.org>
Date: Thu, 24 Jan 2019 11:26:52 +0900
From: Masami Hiramatsu <mhiramat@...nel.org>
To: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Cc: linux-kernel@...r.kernel.org,
Masami Hiramatsu <mhiramat@...nel.org>,
Ulf Hansson <ulf.hansson@...aro.org>,
Gary R Hook <ghook@....com>,
Heiko Carstens <heiko.carstens@...ibm.com>
Subject: Re: [PATCH 2/2] debugfs: return error values, not NULL
On Wed, 23 Jan 2019 11:28:14 +0100
Greg Kroah-Hartman <gregkh@...uxfoundation.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@...nel.org>
Thank you,
>
> Reported-by: Masami Hiramatsu <mhiramat@...nel.org>
> Reported-by: Ulf Hansson <ulf.hansson@...aro.org>
> Reported-by: Gary R Hook <ghook@....com>
> Reported-by: Heiko Carstens <heiko.carstens@...ibm.com>
> Cc: stable <stable@...r.kernel.org>
> Signed-off-by: Greg Kroah-Hartman <gregkh@...uxfoundation.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@...nel.org>
Powered by blists - more mailing lists