* Oops in rpc_clnt_debugfs_register() @ 2019-02-12 11:29 David Howells 2019-02-12 14:31 ` Oops in rpc_clnt_debugfs_register() from debugfs change David Howells 0 siblings, 1 reply; 9+ messages in thread From: David Howells @ 2019-02-12 11:29 UTC (permalink / raw) To: linux-nfs; +Cc: dhowells Hi, I'm getting the attached oops 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Oops in rpc_clnt_debugfs_register() from debugfs change 2019-02-12 11:29 Oops in rpc_clnt_debugfs_register() David Howells @ 2019-02-12 14:31 ` David Howells 2019-02-12 14:37 ` Greg Kroah-Hartman 0 siblings, 1 reply; 9+ messages in thread From: David Howells @ 2019-02-12 14:31 UTC (permalink / raw) To: Greg Kroah-Hartman; +Cc: dhowells, linux-nfs, linux-kernel, mhiramat, bigeasy 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: Oops in rpc_clnt_debugfs_register() from debugfs change 2019-02-12 14:31 ` Oops in rpc_clnt_debugfs_register() from debugfs change David Howells @ 2019-02-12 14:37 ` Greg Kroah-Hartman 2019-02-12 14:42 ` Greg Kroah-Hartman 2019-02-12 14:57 ` David Howells 0 siblings, 2 replies; 9+ messages in thread From: Greg Kroah-Hartman @ 2019-02-12 14:37 UTC (permalink / raw) To: David Howells; +Cc: linux-nfs, linux-kernel, mhiramat, bigeasy 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: Oops in rpc_clnt_debugfs_register() from debugfs change 2019-02-12 14:37 ` Greg Kroah-Hartman @ 2019-02-12 14:42 ` Greg Kroah-Hartman 2019-02-12 15:03 ` Greg Kroah-Hartman 2019-02-12 15:26 ` David Howells 2019-02-12 14:57 ` David Howells 1 sibling, 2 replies; 9+ messages in thread From: Greg Kroah-Hartman @ 2019-02-12 14:42 UTC (permalink / raw) To: David Howells; +Cc: linux-nfs, linux-kernel, mhiramat, bigeasy 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; } ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: Oops in rpc_clnt_debugfs_register() from debugfs change 2019-02-12 14:42 ` Greg Kroah-Hartman @ 2019-02-12 15:03 ` Greg Kroah-Hartman 2019-02-12 15:26 ` David Howells 1 sibling, 0 replies; 9+ messages in thread From: Greg Kroah-Hartman @ 2019-02-12 15:03 UTC (permalink / raw) To: David Howells; +Cc: linux-nfs, linux-kernel, mhiramat, bigeasy 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); } ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: Oops in rpc_clnt_debugfs_register() from debugfs change 2019-02-12 14:42 ` Greg Kroah-Hartman 2019-02-12 15:03 ` Greg Kroah-Hartman @ 2019-02-12 15:26 ` David Howells 1 sibling, 0 replies; 9+ messages in thread From: David Howells @ 2019-02-12 15:26 UTC (permalink / raw) To: Greg Kroah-Hartman; +Cc: dhowells, linux-nfs, linux-kernel, mhiramat, bigeasy 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> ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: Oops in rpc_clnt_debugfs_register() from debugfs change 2019-02-12 14:37 ` Greg Kroah-Hartman 2019-02-12 14:42 ` Greg Kroah-Hartman @ 2019-02-12 14:57 ` David Howells 2019-02-12 15:04 ` Greg Kroah-Hartman 1 sibling, 1 reply; 9+ messages in thread From: David Howells @ 2019-02-12 14:57 UTC (permalink / raw) To: Greg Kroah-Hartman; +Cc: dhowells, linux-nfs, linux-kernel, mhiramat, bigeasy 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: Oops in rpc_clnt_debugfs_register() from debugfs change 2019-02-12 14:57 ` David Howells @ 2019-02-12 15:04 ` Greg Kroah-Hartman 2019-02-12 15:13 ` Greg Kroah-Hartman 0 siblings, 1 reply; 9+ messages in thread From: Greg Kroah-Hartman @ 2019-02-12 15:04 UTC (permalink / raw) To: David Howells; +Cc: linux-nfs, linux-kernel, mhiramat, bigeasy 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: Oops in rpc_clnt_debugfs_register() from debugfs change 2019-02-12 15:04 ` Greg Kroah-Hartman @ 2019-02-12 15:13 ` Greg Kroah-Hartman 0 siblings, 0 replies; 9+ messages in thread From: Greg Kroah-Hartman @ 2019-02-12 15:13 UTC (permalink / raw) To: David Howells; +Cc: linux-nfs, linux-kernel, mhiramat, bigeasy 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 ^ permalink raw reply related [flat|nested] 9+ messages in thread
end of thread, other threads:[~2019-02-12 15:27 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-02-12 11:29 Oops in rpc_clnt_debugfs_register() David Howells 2019-02-12 14:31 ` Oops in rpc_clnt_debugfs_register() from debugfs change David Howells 2019-02-12 14:37 ` Greg Kroah-Hartman 2019-02-12 14:42 ` Greg Kroah-Hartman 2019-02-12 15:03 ` Greg Kroah-Hartman 2019-02-12 15:26 ` David Howells 2019-02-12 14:57 ` David Howells 2019-02-12 15:04 ` Greg Kroah-Hartman 2019-02-12 15:13 ` Greg Kroah-Hartman
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.