Oops in rpc_clnt_debugfs_register() from debugfs change

Oops in rpc_clnt_debugfs_register() from debugfs change

[David Howells]

I've bisected an oops that occurs in rpc_clnt_debugfs_register() trying to
dereference a pointer with -EACCES in it.  This is the causing commit, though
I suspect the bug is in sunrpc expecting to see NULL rather than an error.

ff9fb72bc07705c00795ca48631f7fffe24d2c6b is the first bad commit
commit ff9fb72bc07705c00795ca48631f7fffe24d2c6b
Author: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Date:   Wed Jan 23 11:28:14 2019 +0100

    debugfs: return error values, not NULL
    
    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.
    ...

The attached oops occurs during boot from the gssproxy process in
rpc_clnt_debugfs_register().  The code at this point is:

   0xffffffff8195cbdd <+450>:   mov    0x50(%rax),%rcx   <--- oopsing
   0xffffffff8195cbe1 <+454>:   mov    $0xffffffff821cc8ba,%rdx
   0xffffffff8195cbe8 <+461>:   mov    $0x18,%esi
   0xffffffff8195cbed <+466>:   lea    -0x30(%rbp),%rdi
   0xffffffff8195cbf1 <+470>:   callq  0xffffffff819db773 <snprintf>

RAX is -EACCES.

Looking in the source:

	len = snprintf(name, sizeof(name), "../../rpc_xprt/%s",
			xprt->debugfs->d_name.name);

I think xprt->debugfs is the value in RAX.

	(gdb) p &((struct dentry *)0)->d_name.name
	$5 = (const unsigned char **) 0x50 <irq_stack_union+80>

which matches the offset on the oopsing MOV instruction.

This is with linus/master (aa0c38cf39de73bf7360a3da8f1707601261e518).

David
---
BUG: unable to handle kernel NULL pointer dereference at 0000000000000043
#PF error: [normal kernel read fault]
...
RIP: 0010:rpc_clnt_debugfs_register+0x1c2/0x27e
RSP: 0018:ffff8880be407b58 EFLAGS: 00010286
RAX: fffffffffffffff3 RBX: ffff8880d56c0e00 RCX: ab0ed8627cda32e2
RDX: 000000000000000f RSI: ffffffff82462ac0 RDI: ffff8880ce487018
RBP: ffff8880be407b88 R08: 0000000000000000 R09: 0000000000000000
R10: ffffffff826ccfac R11: 0000000000000000 R12: 0000000000000000
R13: ffffffff8241b540 R14: ffff8880ce6a1000 R15: ffff8880d56c0e00
FS:  00007f195717bc80(0000) GS:ffff8880c6d80000(0000) knlGS:0000000000000000
CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 0000000000000043 CR3: 00000000be40a004 CR4: 00000000001606e0
Call Trace:
 rpc_client_register+0x43/0x271
 rpc_new_client+0x215/0x291
 rpc_create_xprt+0xa5/0x167
 ? rpc_xprt_debugfs_register+0x8b/0xce
 ? page_mapping+0x5e/0x84
 rpc_create+0x143/0x15f
 ? __mutex_lock+0x8f/0x7b0
 ? set_gssp_clnt+0x13b/0x194
 ? mntput_no_expire+0xc0/0x39e
 gssp_rpc_create+0x76/0xdb
 set_gssp_clnt+0x147/0x194
 ? _kstrtoull+0x3b/0x8a
 write_gssp+0x90/0xcc
 proc_reg_write+0x3b/0x59
 ? proc_reg_poll+0x52/0x52
 __vfs_write+0x31/0x15b
 ? rcu_read_lock_sched_held+0x5d/0x63
 ? rcu_sync_lockdep_assert+0x28/0x4e
 ? __sb_start_write+0xb6/0x151
 ? vfs_write+0xca/0x182
 vfs_write+0xdb/0x182
 ksys_write+0x60/0xb1
 do_syscall_64+0x7d/0x1a0
 entry_SYSCALL_64_after_hwframe+0x49/0xbe
RIP: 0033:0x7f1957711d25
Code: 00 00 75 05 48 83 c4 58 c3 e8 f7 49 ff ff 0f 1f 80 00 00 00 00 f3 0f 1e fa 8b 05 26 f7 00 00 85 c0 75 12 b8 01 00 00 00 0f 05 <48> 3d 00 f0 ff ff 77 53 c3 66 90 41 54 49 89 d4 55 48 89 f5 53 89
RSP: 002b:00007ffcc0d5f288 EFLAGS: 00000246 ORIG_RAX: 0000000000000001
RAX: ffffffffffffffda RBX: 0000000000000009 RCX: 00007f1957711d25
RDX: 0000000000000001 RSI: 00007ffcc0d5f296 RDI: 0000000000000009
RBP: 0000000000000000 R08: 0000000000000031 R09: 0000000000000000
R10: 0000000000000000 R11: 0000000000000246 R12: 000055be5a699950
R13: 00007ffcc0d5f5c0 R14: 0000000000000000 R15: 0000000000000000
Modules linked in:
CR2: 0000000000000043

Re: Oops in rpc_clnt_debugfs_register() from debugfs change

[Greg Kroah-Hartman]

On Tue, Feb 12, 2019 at 02:31:14PM +0000, David Howells wrote:
> I've bisected an oops that occurs in rpc_clnt_debugfs_register() trying to
> dereference a pointer with -EACCES in it.  This is the causing commit, though
> I suspect the bug is in sunrpc expecting to see NULL rather than an error.
> 
> ff9fb72bc07705c00795ca48631f7fffe24d2c6b is the first bad commit
> commit ff9fb72bc07705c00795ca48631f7fffe24d2c6b
> Author: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Date:   Wed Jan 23 11:28:14 2019 +0100
> 
>     debugfs: return error values, not NULL
>     
>     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.
>     ...
> 
> The attached oops occurs during boot from the gssproxy process in
> rpc_clnt_debugfs_register().  The code at this point is:
> 
>    0xffffffff8195cbdd <+450>:   mov    0x50(%rax),%rcx   <--- oopsing
>    0xffffffff8195cbe1 <+454>:   mov    $0xffffffff821cc8ba,%rdx
>    0xffffffff8195cbe8 <+461>:   mov    $0x18,%esi
>    0xffffffff8195cbed <+466>:   lea    -0x30(%rbp),%rdi
>    0xffffffff8195cbf1 <+470>:   callq  0xffffffff819db773 <snprintf>
> 
> RAX is -EACCES.
> 
> Looking in the source:
> 
> 	len = snprintf(name, sizeof(name), "../../rpc_xprt/%s",
> 			xprt->debugfs->d_name.name);
> 
> I think xprt->debugfs is the value in RAX.
> 
> 	(gdb) p &((struct dentry *)0)->d_name.name
> 	$5 = (const unsigned char **) 0x50 <irq_stack_union+80>
> 
> which matches the offset on the oopsing MOV instruction.
> 
> This is with linus/master (aa0c38cf39de73bf7360a3da8f1707601261e518).

Ugh, yeah, I see the problem, sorry about that.

I wonder why the debugfs call is always failing, that's not good...

let me dig and see if I already have a patch for this...

greg k-h

Re: Oops in rpc_clnt_debugfs_register() from debugfs change

[Greg Kroah-Hartman]

On Tue, Feb 12, 2019 at 03:37:20PM +0100, Greg Kroah-Hartman wrote:
> On Tue, Feb 12, 2019 at 02:31:14PM +0000, David Howells wrote:
> > I've bisected an oops that occurs in rpc_clnt_debugfs_register() trying to
> > dereference a pointer with -EACCES in it.  This is the causing commit, though
> > I suspect the bug is in sunrpc expecting to see NULL rather than an error.
> > 
> > ff9fb72bc07705c00795ca48631f7fffe24d2c6b is the first bad commit
> > commit ff9fb72bc07705c00795ca48631f7fffe24d2c6b
> > Author: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > Date:   Wed Jan 23 11:28:14 2019 +0100
> > 
> >     debugfs: return error values, not NULL
> >     
> >     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.
> >     ...
> > 
> > The attached oops occurs during boot from the gssproxy process in
> > rpc_clnt_debugfs_register().  The code at this point is:
> > 
> >    0xffffffff8195cbdd <+450>:   mov    0x50(%rax),%rcx   <--- oopsing
> >    0xffffffff8195cbe1 <+454>:   mov    $0xffffffff821cc8ba,%rdx
> >    0xffffffff8195cbe8 <+461>:   mov    $0x18,%esi
> >    0xffffffff8195cbed <+466>:   lea    -0x30(%rbp),%rdi
> >    0xffffffff8195cbf1 <+470>:   callq  0xffffffff819db773 <snprintf>
> > 
> > RAX is -EACCES.
> > 
> > Looking in the source:
> > 
> > 	len = snprintf(name, sizeof(name), "../../rpc_xprt/%s",
> > 			xprt->debugfs->d_name.name);
> > 
> > I think xprt->debugfs is the value in RAX.
> > 
> > 	(gdb) p &((struct dentry *)0)->d_name.name
> > 	$5 = (const unsigned char **) 0x50 <irq_stack_union+80>
> > 
> > which matches the offset on the oopsing MOV instruction.
> > 
> > This is with linus/master (aa0c38cf39de73bf7360a3da8f1707601261e518).
> 
> Ugh, yeah, I see the problem, sorry about that.
> 
> I wonder why the debugfs call is always failing, that's not good...
> 
> let me dig and see if I already have a patch for this...

I have a much larger cleanup patch for this code, but this single line
change should solve the issue for now.  Can you test it to verify?

thanks,

greg k-h

------------------

diff --git a/net/sunrpc/debugfs.c b/net/sunrpc/debugfs.c
index 45a033329cd4..19bb356230ed 100644
--- a/net/sunrpc/debugfs.c
+++ b/net/sunrpc/debugfs.c
@@ -146,7 +146,7 @@ rpc_clnt_debugfs_register(struct rpc_clnt *clnt)
 	rcu_read_lock();
 	xprt = rcu_dereference(clnt->cl_xprt);
 	/* no "debugfs" dentry? Don't bother with the symlink. */
-	if (!xprt->debugfs) {
+	if (IS_ERR_OR_NULL(xprt->debugfs)) {
 		rcu_read_unlock();
 		return;
 	}

Re: Oops in rpc_clnt_debugfs_register() from debugfs change

[David Howells]

Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote:

> -	if (!xprt->debugfs) {
> +	if (IS_ERR_OR_NULL(xprt->debugfs)) {

That works, though I don't much like the idea of there being an error there.

Looking in rpc_xprt_debugfs_register() there are two now-dodgy looking checks
on the result of debugfs calls.

David

Re: Oops in rpc_clnt_debugfs_register() from debugfs change

[Greg Kroah-Hartman]

On Tue, Feb 12, 2019 at 03:42:14PM +0100, Greg Kroah-Hartman wrote:
> On Tue, Feb 12, 2019 at 03:37:20PM +0100, Greg Kroah-Hartman wrote:
> > On Tue, Feb 12, 2019 at 02:31:14PM +0000, David Howells wrote:
> > > I've bisected an oops that occurs in rpc_clnt_debugfs_register() trying to
> > > dereference a pointer with -EACCES in it.  This is the causing commit, though
> > > I suspect the bug is in sunrpc expecting to see NULL rather than an error.
> > > 
> > > ff9fb72bc07705c00795ca48631f7fffe24d2c6b is the first bad commit
> > > commit ff9fb72bc07705c00795ca48631f7fffe24d2c6b
> > > Author: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > > Date:   Wed Jan 23 11:28:14 2019 +0100
> > > 
> > >     debugfs: return error values, not NULL
> > >     
> > >     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.
> > >     ...
> > > 
> > > The attached oops occurs during boot from the gssproxy process in
> > > rpc_clnt_debugfs_register().  The code at this point is:
> > > 
> > >    0xffffffff8195cbdd <+450>:   mov    0x50(%rax),%rcx   <--- oopsing
> > >    0xffffffff8195cbe1 <+454>:   mov    $0xffffffff821cc8ba,%rdx
> > >    0xffffffff8195cbe8 <+461>:   mov    $0x18,%esi
> > >    0xffffffff8195cbed <+466>:   lea    -0x30(%rbp),%rdi
> > >    0xffffffff8195cbf1 <+470>:   callq  0xffffffff819db773 <snprintf>
> > > 
> > > RAX is -EACCES.
> > > 
> > > Looking in the source:
> > > 
> > > 	len = snprintf(name, sizeof(name), "../../rpc_xprt/%s",
> > > 			xprt->debugfs->d_name.name);
> > > 
> > > I think xprt->debugfs is the value in RAX.
> > > 
> > > 	(gdb) p &((struct dentry *)0)->d_name.name
> > > 	$5 = (const unsigned char **) 0x50 <irq_stack_union+80>
> > > 
> > > which matches the offset on the oopsing MOV instruction.
> > > 
> > > This is with linus/master (aa0c38cf39de73bf7360a3da8f1707601261e518).
> > 
> > Ugh, yeah, I see the problem, sorry about that.
> > 
> > I wonder why the debugfs call is always failing, that's not good...
> > 
> > let me dig and see if I already have a patch for this...
> 
> I have a much larger cleanup patch for this code, but this single line
> change should solve the issue for now.  Can you test it to verify?
> 
> thanks,
> 
> greg k-h
> 
> ------------------
> 
> diff --git a/net/sunrpc/debugfs.c b/net/sunrpc/debugfs.c
> index 45a033329cd4..19bb356230ed 100644
> --- a/net/sunrpc/debugfs.c
> +++ b/net/sunrpc/debugfs.c
> @@ -146,7 +146,7 @@ rpc_clnt_debugfs_register(struct rpc_clnt *clnt)
>  	rcu_read_lock();
>  	xprt = rcu_dereference(clnt->cl_xprt);
>  	/* no "debugfs" dentry? Don't bother with the symlink. */
> -	if (!xprt->debugfs) {
> +	if (IS_ERR_OR_NULL(xprt->debugfs)) {
>  		rcu_read_unlock();
>  		return;
>  	}


And, if you want my larger fix that I will be sending to netdev one of
these days, here's that one.  It includes the above patch as part of it.

thanks,

greg k-h

---------------

commit 8d885c486153d1731c14a6a435774a4e9ccd1ebc
Author: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Date:   Fri Jan 4 13:40:56 2019 +0100

    sunrpc: fix changelog

diff --git a/net/sunrpc/debugfs.c b/net/sunrpc/debugfs.c
index 45a033329cd4..ca63f6ed873f 100644
--- a/net/sunrpc/debugfs.c
+++ b/net/sunrpc/debugfs.c
@@ -135,18 +135,15 @@ rpc_clnt_debugfs_register(struct rpc_clnt *clnt)
 
 	/* make the per-client dir */
 	clnt->cl_debugfs = debugfs_create_dir(name, rpc_clnt_dir);
-	if (!clnt->cl_debugfs)
-		return;
 
 	/* make tasks file */
-	if (!debugfs_create_file("tasks", S_IFREG | 0400, clnt->cl_debugfs,
-				 clnt, &tasks_fops))
-		goto out_err;
+	debugfs_create_file("tasks", S_IFREG | 0400, clnt->cl_debugfs, clnt,
+			    &tasks_fops);
 
 	rcu_read_lock();
 	xprt = rcu_dereference(clnt->cl_xprt);
 	/* no "debugfs" dentry? Don't bother with the symlink. */
-	if (!xprt->debugfs) {
+	if (IS_ERR_OR_NULL(xprt->debugfs)) {
 		rcu_read_unlock();
 		return;
 	}
@@ -157,8 +154,7 @@ rpc_clnt_debugfs_register(struct rpc_clnt *clnt)
 	if (len >= sizeof(name))
 		goto out_err;
 
-	if (!debugfs_create_symlink("xprt", clnt->cl_debugfs, name))
-		goto out_err;
+	debugfs_create_symlink("xprt", clnt->cl_debugfs, name);
 
 	return;
 out_err:
@@ -237,15 +233,10 @@ rpc_xprt_debugfs_register(struct rpc_xprt *xprt)
 
 	/* make the per-client dir */
 	xprt->debugfs = debugfs_create_dir(name, rpc_xprt_dir);
-	if (!xprt->debugfs)
-		return;
 
 	/* make tasks file */
-	if (!debugfs_create_file("info", S_IFREG | 0400, xprt->debugfs,
-				 xprt, &xprt_info_fops)) {
-		debugfs_remove_recursive(xprt->debugfs);
-		xprt->debugfs = NULL;
-	}
+	debugfs_create_file("info", S_IFREG | 0400, xprt->debugfs, xprt,
+			    &xprt_info_fops);
 
 	atomic_set(&xprt->inject_disconnect, rpc_inject_disconnect);
 }
@@ -308,22 +299,6 @@ static const struct file_operations fault_disconnect_fops = {
 	.release	= fault_release,
 };
 
-static struct dentry *
-inject_fault_dir(struct dentry *topdir)
-{
-	struct dentry *faultdir;
-
-	faultdir = debugfs_create_dir("inject_fault", topdir);
-	if (!faultdir)
-		return NULL;
-
-	if (!debugfs_create_file("disconnect", S_IFREG | 0400, faultdir,
-				 NULL, &fault_disconnect_fops))
-		return NULL;
-
-	return faultdir;
-}
-
 void __exit
 sunrpc_debugfs_exit(void)
 {
@@ -338,25 +313,13 @@ void __init
 sunrpc_debugfs_init(void)
 {
 	topdir = debugfs_create_dir("sunrpc", NULL);
-	if (!topdir)
-		return;
-
-	rpc_fault_dir = inject_fault_dir(topdir);
-	if (!rpc_fault_dir)
-		goto out_remove;
 
 	rpc_clnt_dir = debugfs_create_dir("rpc_clnt", topdir);
-	if (!rpc_clnt_dir)
-		goto out_remove;
 
 	rpc_xprt_dir = debugfs_create_dir("rpc_xprt", topdir);
-	if (!rpc_xprt_dir)
-		goto out_remove;
 
-	return;
-out_remove:
-	debugfs_remove_recursive(topdir);
-	topdir = NULL;
-	rpc_fault_dir = NULL;
-	rpc_clnt_dir = NULL;
+	rpc_fault_dir = debugfs_create_dir("inject_fault", topdir);
+
+	debugfs_create_file("disconnect", S_IFREG | 0400, rpc_fault_dir, NULL,
+			    &fault_disconnect_fops);
 }

Re: Oops in rpc_clnt_debugfs_register() from debugfs change

[Greg Kroah-Hartman]

On Tue, Feb 12, 2019 at 02:57:34PM +0000, David Howells wrote:
> Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote:
> 
> > -	if (!xprt->debugfs) {
> > +	if (IS_ERR_OR_NULL(xprt->debugfs)) {
> 
> That works, though I don't much like the idea of there being an error there.
> 
> Looking in rpc_xprt_debugfs_register() there are two now-dodgy looking checks
> on the result of debugfs calls.

now-dodgy checks are fine.  Well, they shouldn't matter, I've sent a
patch that just gets rid of those checks.

Ideally no one should need to check of debugfs is ok or not, the fact
that these functions keep getting called is a bit odd, I can look into
that some more, it shouldn't be needed...

thanks,

greg k-h

Re: Oops in rpc_clnt_debugfs_register() from debugfs change

[Greg Kroah-Hartman]

On Tue, Feb 12, 2019 at 04:04:59PM +0100, Greg Kroah-Hartman wrote:
> On Tue, Feb 12, 2019 at 02:57:34PM +0000, David Howells wrote:
> > Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote:
> > 
> > > -	if (!xprt->debugfs) {
> > > +	if (IS_ERR_OR_NULL(xprt->debugfs)) {
> > 
> > That works, though I don't much like the idea of there being an error there.
> > 
> > Looking in rpc_xprt_debugfs_register() there are two now-dodgy looking checks
> > on the result of debugfs calls.
> 
> now-dodgy checks are fine.  Well, they shouldn't matter, I've sent a
> patch that just gets rid of those checks.
> 
> Ideally no one should need to check of debugfs is ok or not, the fact
> that these functions keep getting called is a bit odd, I can look into
> that some more, it shouldn't be needed...

And here's a "final" version of this, that removes all of the "dodgy"
checks, with the exception of the "is this actually a dentry" check that
my first patch had, which is still required.

Overall it makes the code smaller and simpler, but for 5.0-final, I
think my original patch should be all that is needed.

thanks,

greg k-h

-------------------------

From e6114e66bb7921b1e83e5ca0083893afa7816b45 Mon Sep 17 00:00:00 2001
From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Date: Fri, 4 Jan 2019 13:40:56 +0100
Subject: [PATCH] sunrpc: no need to check return value of debugfs_create functions

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.

Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

---
 net/sunrpc/debugfs.c | 68 ++++++++------------------------------------
 1 file changed, 12 insertions(+), 56 deletions(-)

diff --git a/net/sunrpc/debugfs.c b/net/sunrpc/debugfs.c
index 45a033329cd4..c7ad5772f5d9 100644
--- a/net/sunrpc/debugfs.c
+++ b/net/sunrpc/debugfs.c
@@ -11,7 +11,6 @@
 #include "netns.h"
 
 static struct dentry *topdir;
-static struct dentry *rpc_fault_dir;
 static struct dentry *rpc_clnt_dir;
 static struct dentry *rpc_xprt_dir;
 
@@ -125,28 +124,21 @@ rpc_clnt_debugfs_register(struct rpc_clnt *clnt)
 	char name[24]; /* enough for "../../rpc_xprt/ + 8 hex digits + NULL */
 	struct rpc_xprt *xprt;
 
-	/* Already registered? */
-	if (clnt->cl_debugfs || !rpc_clnt_dir)
-		return;
-
 	len = snprintf(name, sizeof(name), "%x", clnt->cl_clid);
 	if (len >= sizeof(name))
 		return;
 
 	/* make the per-client dir */
 	clnt->cl_debugfs = debugfs_create_dir(name, rpc_clnt_dir);
-	if (!clnt->cl_debugfs)
-		return;
 
 	/* make tasks file */
-	if (!debugfs_create_file("tasks", S_IFREG | 0400, clnt->cl_debugfs,
-				 clnt, &tasks_fops))
-		goto out_err;
+	debugfs_create_file("tasks", S_IFREG | 0400, clnt->cl_debugfs, clnt,
+			    &tasks_fops);
 
 	rcu_read_lock();
 	xprt = rcu_dereference(clnt->cl_xprt);
 	/* no "debugfs" dentry? Don't bother with the symlink. */
-	if (!xprt->debugfs) {
+	if (IS_ERR_OR_NULL(xprt->debugfs)) {
 		rcu_read_unlock();
 		return;
 	}
@@ -157,8 +149,7 @@ rpc_clnt_debugfs_register(struct rpc_clnt *clnt)
 	if (len >= sizeof(name))
 		goto out_err;
 
-	if (!debugfs_create_symlink("xprt", clnt->cl_debugfs, name))
-		goto out_err;
+	debugfs_create_symlink("xprt", clnt->cl_debugfs, name);
 
 	return;
 out_err:
@@ -226,9 +217,6 @@ rpc_xprt_debugfs_register(struct rpc_xprt *xprt)
 	static atomic_t	cur_id;
 	char		name[9]; /* 8 hex digits + NULL term */
 
-	if (!rpc_xprt_dir)
-		return;
-
 	id = (unsigned int)atomic_inc_return(&cur_id);
 
 	len = snprintf(name, sizeof(name), "%x", id);
@@ -237,15 +225,10 @@ rpc_xprt_debugfs_register(struct rpc_xprt *xprt)
 
 	/* make the per-client dir */
 	xprt->debugfs = debugfs_create_dir(name, rpc_xprt_dir);
-	if (!xprt->debugfs)
-		return;
 
 	/* make tasks file */
-	if (!debugfs_create_file("info", S_IFREG | 0400, xprt->debugfs,
-				 xprt, &xprt_info_fops)) {
-		debugfs_remove_recursive(xprt->debugfs);
-		xprt->debugfs = NULL;
-	}
+	debugfs_create_file("info", S_IFREG | 0400, xprt->debugfs, xprt,
+			    &xprt_info_fops);
 
 	atomic_set(&xprt->inject_disconnect, rpc_inject_disconnect);
 }
@@ -308,28 +291,11 @@ static const struct file_operations fault_disconnect_fops = {
 	.release	= fault_release,
 };
 
-static struct dentry *
-inject_fault_dir(struct dentry *topdir)
-{
-	struct dentry *faultdir;
-
-	faultdir = debugfs_create_dir("inject_fault", topdir);
-	if (!faultdir)
-		return NULL;
-
-	if (!debugfs_create_file("disconnect", S_IFREG | 0400, faultdir,
-				 NULL, &fault_disconnect_fops))
-		return NULL;
-
-	return faultdir;
-}
-
 void __exit
 sunrpc_debugfs_exit(void)
 {
 	debugfs_remove_recursive(topdir);
 	topdir = NULL;
-	rpc_fault_dir = NULL;
 	rpc_clnt_dir = NULL;
 	rpc_xprt_dir = NULL;
 }
@@ -337,26 +303,16 @@ sunrpc_debugfs_exit(void)
 void __init
 sunrpc_debugfs_init(void)
 {
-	topdir = debugfs_create_dir("sunrpc", NULL);
-	if (!topdir)
-		return;
+	struct dentry *rpc_fault_dir;
 
-	rpc_fault_dir = inject_fault_dir(topdir);
-	if (!rpc_fault_dir)
-		goto out_remove;
+	topdir = debugfs_create_dir("sunrpc", NULL);
 
 	rpc_clnt_dir = debugfs_create_dir("rpc_clnt", topdir);
-	if (!rpc_clnt_dir)
-		goto out_remove;
 
 	rpc_xprt_dir = debugfs_create_dir("rpc_xprt", topdir);
-	if (!rpc_xprt_dir)
-		goto out_remove;
 
-	return;
-out_remove:
-	debugfs_remove_recursive(topdir);
-	topdir = NULL;
-	rpc_fault_dir = NULL;
-	rpc_clnt_dir = NULL;
+	rpc_fault_dir = debugfs_create_dir("inject_fault", topdir);
+
+	debugfs_create_file("disconnect", S_IFREG | 0400, rpc_fault_dir, NULL,
+			    &fault_disconnect_fops);
 }
-- 
2.20.1

Re: Oops in rpc_clnt_debugfs_register() from debugfs change

[David Howells]

Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote:

> And, if you want my larger fix that I will be sending to netdev one of
> these days, here's that one.  It includes the above patch as part of it.

That works.

Tested-by: David Howells <dhowells@redhat.com>