[PATCH 1/1] nfsd: incorrect check for debugfs returns

[PATCH 1/1] nfsd: incorrect check for debugfs returns

[Chengyu Song]

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.

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

Re: [PATCH 1/1] nfsd: incorrect check for debugfs returns

[Jeff Layton]

On Mon, 23 Mar 2015 22:58:05 -0400
Chengyu Song <csong84@gatech.edu> 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.
> 
> 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;
>  }

No objection to taking this patch in the near term if it helps, but we
had discussed over the summer just removing the NFS fault injection
framework.

Bruce, any objections to making that happen for v4.1?

-- 
Jeff Layton <jeff.layton@primarydata.com>

Re: [PATCH 1/1] nfsd: incorrect check for debugfs returns

[J. Bruce Fields]

On Tue, Mar 24, 2015 at 06:44:20AM -0400, Jeff Layton wrote:
> On Mon, 23 Mar 2015 22:58:05 -0400
> Chengyu Song <csong84@gatech.edu> 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.
> > 
> > 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;
> >  }
> 
> No objection to taking this patch in the near term if it helps, but we
> had discussed over the summer just removing the NFS fault injection
> framework.
> 
> Bruce, any objections to making that happen for v4.1?

I was prepared to, but I think Redhat QA people told me that they do use
it--which means other people may too, so I'm sort of reluctant to tear
it out even if it's imperfect.

--b.

Re: [PATCH 1/1] nfsd: incorrect check for debugfs returns

[J. Bruce Fields]

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

Re: [PATCH 1/1] nfsd: incorrect check for debugfs returns

[Jeff Layton]

On Wed, 25 Mar 2015 11:08:56 -0400
"J. Bruce Fields" <bfields@fieldses.org> wrote:

> On Tue, Mar 24, 2015 at 06:44:20AM -0400, Jeff Layton wrote:
> > On Mon, 23 Mar 2015 22:58:05 -0400
> > Chengyu Song <csong84@gatech.edu> 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.
> > > 
> > > 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;
> > >  }
> > 
> > No objection to taking this patch in the near term if it helps, but we
> > had discussed over the summer just removing the NFS fault injection
> > framework.
> > 
> > Bruce, any objections to making that happen for v4.1?
> 
> I was prepared to, but I think Redhat QA people told me that they do use
> it--which means other people may too, so I'm sort of reluctant to tear
> it out even if it's imperfect.
> 
> --b.

Fair enough then. I'll leave it be...

-- 
Jeff Layton <jeff.layton@primarydata.com>

Re: [PATCH 1/1] nfsd: incorrect check for debugfs returns

[Chengyu Song]

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

Re: [PATCH 1/1] nfsd: incorrect check for debugfs returns

[J. Bruce Fields]

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 <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

Re: [PATCH 1/1] nfsd: incorrect check for debugfs returns

[Chengyu Song]

Cool. Sent.

> On Mar 25, 2015, at 4:09 PM, J. Bruce Fields <bfields@fieldses.org> wrote:
> 
> 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 <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