Messages in this thread | | | Subject | Re: [PATCH 1/1] nfsd: incorrect check for debugfs returns | From | Chengyu Song <> | Date | Wed, 25 Mar 2015 12:41:29 -0400 |
| |
There may be a simpler solution, declare NFSD_FAULT_INJECTION has dependency on DEBUG_FS, or automatically select DEBUG_FS. I don't think current debugfs implementation will return any error ptr once it's configured.
I choose to check the return instead, because I was worried the debugfs interface may change in the future.
Does this sounds like a solution? If so, I can submit a patch for Kconfig.
Best, Chengyu
> On Mar 25, 2015, at 11:17 AM, J. Bruce Fields <bfields@fieldses.org> wrote: > > On Mon, Mar 23, 2015 at 10:58:05PM -0400, Chengyu Song wrote: >> debugfs_create_dir and debugfs_create_file may return -ENODEV when debugfs >> is not configured, so the return value should be checked against ERROR_VALUE >> as well, otherwise the later dereference of the dentry pointer would crash >> the kernel. > > Thanks for spotting this. But it looks like this will cause nfsd > startup to fail when debugfs isn't configured. I'd rather we didn't, it > just isn't that important. > > So I'd rather just make nfsd_fault_inject_init() a void return--just do > a dprintk as a warning in the "fail" case, and otherwise let normal > startup continue (and check that doesn't lead to other unsafe > dereferences of debug_dir). Could you try that? > > --b. > > >> >> Signed-off-by: Chengyu Song <csong84@gatech.edu> >> --- >> fs/nfsd/fault_inject.c | 12 ++++++++---- >> 1 file changed, 8 insertions(+), 4 deletions(-) >> >> diff --git a/fs/nfsd/fault_inject.c b/fs/nfsd/fault_inject.c >> index c16bf5a..621d065 100644 >> --- a/fs/nfsd/fault_inject.c >> +++ b/fs/nfsd/fault_inject.c >> @@ -132,19 +132,23 @@ int nfsd_fault_inject_init(void) >> unsigned int i; >> struct nfsd_fault_inject_op *op; >> umode_t mode = S_IFREG | S_IRUSR | S_IWUSR; >> + struct dentry *dent; >> >> - debug_dir = debugfs_create_dir("nfsd", NULL); >> - if (!debug_dir) >> + dent = debugfs_create_dir("nfsd", NULL); >> + if (IS_ERR_OR_NULL(dent)) >> goto fail; >> + debug_dir = dent; >> >> for (i = 0; i < NUM_INJECT_OPS; i++) { >> op = &inject_ops[i]; >> - if (!debugfs_create_file(op->file, mode, debug_dir, op, &fops_nfsd)) >> + dent = debugfs_create_file(op->file, mode, debug_dir, op, &fops_nfsd); >> + if (IS_ERR_OR_NULL(dent)) >> goto fail; >> + >> } >> return 0; >> >> fail: >> nfsd_fault_inject_cleanup(); >> - return -ENOMEM; >> + return dent ? PTR_ERR(dent) : -ENOMEM; >> } >> -- >> 2.1.0
| |