* [PATCH] fail_function: no need to check return value of debugfs_create functions
@ 2019-01-22 15:21 Greg Kroah-Hartman
2019-01-23 0:11 ` Masami Hiramatsu
0 siblings, 1 reply; 5+ messages in thread
From: Greg Kroah-Hartman @ 2019-01-22 15:21 UTC (permalink / raw)
To: linux-kernel
Cc: Greg Kroah-Hartman, Masami Hiramatsu, Kees Cook, Josef Bacik,
Thomas Gleixner, Naveen N. Rao, zhong jiang
When calling debugfs functions, there is no need to ever check the
return value. The function can work or not, but the code logic should
never do something different based on this.
Cc: Masami Hiramatsu <mhiramat@kernel.org>
Cc: Kees Cook <keescook@chromium.org>
Cc: Josef Bacik <jbacik@fb.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com>
Cc: zhong jiang <zhongjiang@huawei.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
kernel/fail_function.c | 23 +++++------------------
1 file changed, 5 insertions(+), 18 deletions(-)
diff --git a/kernel/fail_function.c b/kernel/fail_function.c
index 17f75b545f66..afc779be5ebb 100644
--- a/kernel/fail_function.c
+++ b/kernel/fail_function.c
@@ -152,20 +152,13 @@ static int fei_retval_get(void *data, u64 *val)
DEFINE_DEBUGFS_ATTRIBUTE(fei_retval_ops, fei_retval_get, fei_retval_set,
"%llx\n");
-static int fei_debugfs_add_attr(struct fei_attr *attr)
+static void fei_debugfs_add_attr(struct fei_attr *attr)
{
struct dentry *dir;
dir = debugfs_create_dir(attr->kp.symbol_name, fei_debugfs_dir);
- if (!dir)
- return -ENOMEM;
-
- if (!debugfs_create_file("retval", 0600, dir, attr, &fei_retval_ops)) {
- debugfs_remove_recursive(dir);
- return -ENOMEM;
- }
- return 0;
+ debugfs_create_file("retval", 0600, dir, attr, &fei_retval_ops);
}
static void fei_debugfs_remove_attr(struct fei_attr *attr)
@@ -306,7 +299,7 @@ static ssize_t fei_write(struct file *file, const char __user *buffer,
ret = register_kprobe(&attr->kp);
if (!ret)
- ret = fei_debugfs_add_attr(attr);
+ fei_debugfs_add_attr(attr);
if (ret < 0)
fei_attr_remove(attr);
else {
@@ -337,19 +330,13 @@ static int __init fei_debugfs_init(void)
return PTR_ERR(dir);
/* injectable attribute is just a symlink of error_inject/list */
- if (!debugfs_create_symlink("injectable", dir,
- "../error_injection/list"))
- goto error;
+ debugfs_create_symlink("injectable", dir, "../error_injection/list");
- if (!debugfs_create_file("inject", 0600, dir, NULL, &fei_ops))
- goto error;
+ debugfs_create_file("inject", 0600, dir, NULL, &fei_ops);
fei_debugfs_dir = dir;
return 0;
-error:
- debugfs_remove_recursive(dir);
- return -ENOMEM;
}
late_initcall(fei_debugfs_init);
--
2.20.1
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [PATCH] fail_function: no need to check return value of debugfs_create functions
2019-01-22 15:21 [PATCH] fail_function: no need to check return value of debugfs_create functions Greg Kroah-Hartman
@ 2019-01-23 0:11 ` Masami Hiramatsu
2019-01-23 6:33 ` Greg Kroah-Hartman
0 siblings, 1 reply; 5+ messages in thread
From: Masami Hiramatsu @ 2019-01-23 0:11 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: linux-kernel, Masami Hiramatsu, Kees Cook, Josef Bacik,
Thomas Gleixner, Naveen N. Rao, zhong jiang
On Tue, 22 Jan 2019 16:21:44 +0100
Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote:
> When calling debugfs functions, there is no need to ever check the
> return value. The function can work or not, but the code logic should
> never do something different based on this.
Ah, OK. It simplifies the code. But I have a question below,
>
> Cc: Masami Hiramatsu <mhiramat@kernel.org>
> Cc: Kees Cook <keescook@chromium.org>
> Cc: Josef Bacik <jbacik@fb.com>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com>
> Cc: zhong jiang <zhongjiang@huawei.com>
> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> ---
> kernel/fail_function.c | 23 +++++------------------
> 1 file changed, 5 insertions(+), 18 deletions(-)
>
> diff --git a/kernel/fail_function.c b/kernel/fail_function.c
> index 17f75b545f66..afc779be5ebb 100644
> --- a/kernel/fail_function.c
> +++ b/kernel/fail_function.c
> @@ -152,20 +152,13 @@ static int fei_retval_get(void *data, u64 *val)
> DEFINE_DEBUGFS_ATTRIBUTE(fei_retval_ops, fei_retval_get, fei_retval_set,
> "%llx\n");
>
> -static int fei_debugfs_add_attr(struct fei_attr *attr)
> +static void fei_debugfs_add_attr(struct fei_attr *attr)
> {
> struct dentry *dir;
>
> dir = debugfs_create_dir(attr->kp.symbol_name, fei_debugfs_dir);
> - if (!dir)
> - return -ENOMEM;
> -
> - if (!debugfs_create_file("retval", 0600, dir, attr, &fei_retval_ops)) {
> - debugfs_remove_recursive(dir);
> - return -ENOMEM;
> - }
>
> - return 0;
Don't we need to check dir here? If above debugfs_create_dir() returns NULL,
it seems we will create "retval" under root directory of debugfs.
Thank you,
> + debugfs_create_file("retval", 0600, dir, attr, &fei_retval_ops);
> }
>
> static void fei_debugfs_remove_attr(struct fei_attr *attr)
> @@ -306,7 +299,7 @@ static ssize_t fei_write(struct file *file, const char __user *buffer,
>
> ret = register_kprobe(&attr->kp);
> if (!ret)
> - ret = fei_debugfs_add_attr(attr);
> + fei_debugfs_add_attr(attr);
> if (ret < 0)
> fei_attr_remove(attr);
> else {
> @@ -337,19 +330,13 @@ static int __init fei_debugfs_init(void)
> return PTR_ERR(dir);
>
> /* injectable attribute is just a symlink of error_inject/list */
> - if (!debugfs_create_symlink("injectable", dir,
> - "../error_injection/list"))
> - goto error;
> + debugfs_create_symlink("injectable", dir, "../error_injection/list");
>
> - if (!debugfs_create_file("inject", 0600, dir, NULL, &fei_ops))
> - goto error;
> + debugfs_create_file("inject", 0600, dir, NULL, &fei_ops);
>
> fei_debugfs_dir = dir;
>
> return 0;
> -error:
> - debugfs_remove_recursive(dir);
> - return -ENOMEM;
> }
>
> late_initcall(fei_debugfs_init);
> --
> 2.20.1
>
--
Masami Hiramatsu <mhiramat@kernel.org>
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] fail_function: no need to check return value of debugfs_create functions
2019-01-23 0:11 ` Masami Hiramatsu
@ 2019-01-23 6:33 ` Greg Kroah-Hartman
2019-01-23 6:34 ` Greg Kroah-Hartman
0 siblings, 1 reply; 5+ messages in thread
From: Greg Kroah-Hartman @ 2019-01-23 6:33 UTC (permalink / raw)
To: Masami Hiramatsu
Cc: linux-kernel, Kees Cook, Josef Bacik, Thomas Gleixner,
Naveen N. Rao, zhong jiang
On Wed, Jan 23, 2019 at 09:11:41AM +0900, Masami Hiramatsu wrote:
> On Tue, 22 Jan 2019 16:21:44 +0100
> Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote:
>
> > When calling debugfs functions, there is no need to ever check the
> > return value. The function can work or not, but the code logic should
> > never do something different based on this.
>
> Ah, OK. It simplifies the code. But I have a question below,
>
> >
> > Cc: Masami Hiramatsu <mhiramat@kernel.org>
> > Cc: Kees Cook <keescook@chromium.org>
> > Cc: Josef Bacik <jbacik@fb.com>
> > Cc: Thomas Gleixner <tglx@linutronix.de>
> > Cc: "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com>
> > Cc: zhong jiang <zhongjiang@huawei.com>
> > Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > ---
> > kernel/fail_function.c | 23 +++++------------------
> > 1 file changed, 5 insertions(+), 18 deletions(-)
> >
> > diff --git a/kernel/fail_function.c b/kernel/fail_function.c
> > index 17f75b545f66..afc779be5ebb 100644
> > --- a/kernel/fail_function.c
> > +++ b/kernel/fail_function.c
> > @@ -152,20 +152,13 @@ static int fei_retval_get(void *data, u64 *val)
> > DEFINE_DEBUGFS_ATTRIBUTE(fei_retval_ops, fei_retval_get, fei_retval_set,
> > "%llx\n");
> >
> > -static int fei_debugfs_add_attr(struct fei_attr *attr)
> > +static void fei_debugfs_add_attr(struct fei_attr *attr)
> > {
> > struct dentry *dir;
> >
> > dir = debugfs_create_dir(attr->kp.symbol_name, fei_debugfs_dir);
> > - if (!dir)
> > - return -ENOMEM;
> > -
> > - if (!debugfs_create_file("retval", 0600, dir, attr, &fei_retval_ops)) {
> > - debugfs_remove_recursive(dir);
> > - return -ENOMEM;
> > - }
> >
> > - return 0;
>
> Don't we need to check dir here? If above debugfs_create_dir() returns NULL,
> it seems we will create "retval" under root directory of debugfs.
If NULL is returned, your system is out of memory and worse things are
about to happen :)
thanks,
greg k-h
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] fail_function: no need to check return value of debugfs_create functions
2019-01-23 6:33 ` Greg Kroah-Hartman
@ 2019-01-23 6:34 ` Greg Kroah-Hartman
2019-01-23 7:33 ` Masami Hiramatsu
0 siblings, 1 reply; 5+ messages in thread
From: Greg Kroah-Hartman @ 2019-01-23 6:34 UTC (permalink / raw)
To: Masami Hiramatsu
Cc: linux-kernel, Kees Cook, Josef Bacik, Thomas Gleixner,
Naveen N. Rao, zhong jiang
On Wed, Jan 23, 2019 at 07:33:05AM +0100, Greg Kroah-Hartman wrote:
> On Wed, Jan 23, 2019 at 09:11:41AM +0900, Masami Hiramatsu wrote:
> > On Tue, 22 Jan 2019 16:21:44 +0100
> > Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote:
> >
> > > When calling debugfs functions, there is no need to ever check the
> > > return value. The function can work or not, but the code logic should
> > > never do something different based on this.
> >
> > Ah, OK. It simplifies the code. But I have a question below,
> >
> > >
> > > Cc: Masami Hiramatsu <mhiramat@kernel.org>
> > > Cc: Kees Cook <keescook@chromium.org>
> > > Cc: Josef Bacik <jbacik@fb.com>
> > > Cc: Thomas Gleixner <tglx@linutronix.de>
> > > Cc: "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com>
> > > Cc: zhong jiang <zhongjiang@huawei.com>
> > > Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > > ---
> > > kernel/fail_function.c | 23 +++++------------------
> > > 1 file changed, 5 insertions(+), 18 deletions(-)
> > >
> > > diff --git a/kernel/fail_function.c b/kernel/fail_function.c
> > > index 17f75b545f66..afc779be5ebb 100644
> > > --- a/kernel/fail_function.c
> > > +++ b/kernel/fail_function.c
> > > @@ -152,20 +152,13 @@ static int fei_retval_get(void *data, u64 *val)
> > > DEFINE_DEBUGFS_ATTRIBUTE(fei_retval_ops, fei_retval_get, fei_retval_set,
> > > "%llx\n");
> > >
> > > -static int fei_debugfs_add_attr(struct fei_attr *attr)
> > > +static void fei_debugfs_add_attr(struct fei_attr *attr)
> > > {
> > > struct dentry *dir;
> > >
> > > dir = debugfs_create_dir(attr->kp.symbol_name, fei_debugfs_dir);
> > > - if (!dir)
> > > - return -ENOMEM;
> > > -
> > > - if (!debugfs_create_file("retval", 0600, dir, attr, &fei_retval_ops)) {
> > > - debugfs_remove_recursive(dir);
> > > - return -ENOMEM;
> > > - }
> > >
> > > - return 0;
> >
> > Don't we need to check dir here? If above debugfs_create_dir() returns NULL,
> > it seems we will create "retval" under root directory of debugfs.
>
> If NULL is returned, your system is out of memory and worse things are
> about to happen :)
But you aren't the first to ask about this, I guess I should just return
ENOMEM and then the follow-on files will not be created. I'll go make
that change to the core of debugfs to help prevent this problem.
thanks,
greg k-h
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] fail_function: no need to check return value of debugfs_create functions
2019-01-23 6:34 ` Greg Kroah-Hartman
@ 2019-01-23 7:33 ` Masami Hiramatsu
0 siblings, 0 replies; 5+ messages in thread
From: Masami Hiramatsu @ 2019-01-23 7:33 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: linux-kernel, Kees Cook, Josef Bacik, Thomas Gleixner,
Naveen N. Rao, zhong jiang
On Wed, 23 Jan 2019 07:34:27 +0100
Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote:
> On Wed, Jan 23, 2019 at 07:33:05AM +0100, Greg Kroah-Hartman wrote:
> > On Wed, Jan 23, 2019 at 09:11:41AM +0900, Masami Hiramatsu wrote:
> > > On Tue, 22 Jan 2019 16:21:44 +0100
> > > Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote:
> > >
> > > > When calling debugfs functions, there is no need to ever check the
> > > > return value. The function can work or not, but the code logic should
> > > > never do something different based on this.
> > >
> > > Ah, OK. It simplifies the code. But I have a question below,
> > >
> > > >
> > > > Cc: Masami Hiramatsu <mhiramat@kernel.org>
> > > > Cc: Kees Cook <keescook@chromium.org>
> > > > Cc: Josef Bacik <jbacik@fb.com>
> > > > Cc: Thomas Gleixner <tglx@linutronix.de>
> > > > Cc: "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com>
> > > > Cc: zhong jiang <zhongjiang@huawei.com>
> > > > Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > > > ---
> > > > kernel/fail_function.c | 23 +++++------------------
> > > > 1 file changed, 5 insertions(+), 18 deletions(-)
> > > >
> > > > diff --git a/kernel/fail_function.c b/kernel/fail_function.c
> > > > index 17f75b545f66..afc779be5ebb 100644
> > > > --- a/kernel/fail_function.c
> > > > +++ b/kernel/fail_function.c
> > > > @@ -152,20 +152,13 @@ static int fei_retval_get(void *data, u64 *val)
> > > > DEFINE_DEBUGFS_ATTRIBUTE(fei_retval_ops, fei_retval_get, fei_retval_set,
> > > > "%llx\n");
> > > >
> > > > -static int fei_debugfs_add_attr(struct fei_attr *attr)
> > > > +static void fei_debugfs_add_attr(struct fei_attr *attr)
> > > > {
> > > > struct dentry *dir;
> > > >
> > > > dir = debugfs_create_dir(attr->kp.symbol_name, fei_debugfs_dir);
> > > > - if (!dir)
> > > > - return -ENOMEM;
> > > > -
> > > > - if (!debugfs_create_file("retval", 0600, dir, attr, &fei_retval_ops)) {
> > > > - debugfs_remove_recursive(dir);
> > > > - return -ENOMEM;
> > > > - }
> > > >
> > > > - return 0;
> > >
> > > Don't we need to check dir here? If above debugfs_create_dir() returns NULL,
> > > it seems we will create "retval" under root directory of debugfs.
> >
> > If NULL is returned, your system is out of memory and worse things are
> > about to happen :)
>
> But you aren't the first to ask about this, I guess I should just return
> ENOMEM and then the follow-on files will not be created. I'll go make
> that change to the core of debugfs to help prevent this problem.
I got it, and yes, returning -ENOMEM sounds good to me, especially, in this case.
Acked-by: Masami Hiramatsu <mhiramat@kernel.org>
Thanks!
>
> thanks,
>
> greg k-h
--
Masami Hiramatsu <mhiramat@kernel.org>
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2019-01-23 7:33 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-01-22 15:21 [PATCH] fail_function: no need to check return value of debugfs_create functions Greg Kroah-Hartman
2019-01-23 0:11 ` Masami Hiramatsu
2019-01-23 6:33 ` Greg Kroah-Hartman
2019-01-23 6:34 ` Greg Kroah-Hartman
2019-01-23 7:33 ` Masami Hiramatsu
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).