On Wed, Mar 25, 2015 at 12:41:29PM -0400, Chengyu Song wrote:
> There may be a simpler solution, declare NFSD_FAULT_INJECTION has dependency
> on DEBUG_FS, or automatically select DEBUG_FS.
Oh, I forgot about that--you're right, NFSD_FAULT_INJECTION isn't useful
without DEBUG_FS anyway, so, sure, let's do that.
--b.
> 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 <bfie...@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 <cson...@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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
