[PATCH] backing-dev: no need to check return value of debugfs_create functions

[PATCH] backing-dev: no need to check return value of debugfs_create functions

[Greg Kroah-Hartman]

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.

And as the return value does not matter at all, no need to save the
dentry in struct backing_dev_info, so delete it.

Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Anders Roxell <anders.roxell@linaro.org>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Cc: Michal Hocko <mhocko@suse.com>
Cc: linux-mm@kvack.org
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 include/linux/backing-dev-defs.h |  1 -
 mm/backing-dev.c                 | 24 +++++-------------------
 2 files changed, 5 insertions(+), 20 deletions(-)

diff --git a/include/linux/backing-dev-defs.h b/include/linux/backing-dev-defs.h
index c31157135598..7f64d813580b 100644
--- a/include/linux/backing-dev-defs.h
+++ b/include/linux/backing-dev-defs.h
@@ -202,7 +202,6 @@ struct backing_dev_info {
 
 #ifdef CONFIG_DEBUG_FS
 	struct dentry *debug_dir;
-	struct dentry *debug_stats;
 #endif
 };
 
diff --git a/mm/backing-dev.c b/mm/backing-dev.c
index 8a8bb8796c6c..85ef344a9c67 100644
--- a/mm/backing-dev.c
+++ b/mm/backing-dev.c
@@ -102,39 +102,25 @@ static int bdi_debug_stats_show(struct seq_file *m, void *v)
 }
 DEFINE_SHOW_ATTRIBUTE(bdi_debug_stats);
 
-static int bdi_debug_register(struct backing_dev_info *bdi, const char *name)
+static void bdi_debug_register(struct backing_dev_info *bdi, const char *name)
 {
-	if (!bdi_debug_root)
-		return -ENOMEM;
-
 	bdi->debug_dir = debugfs_create_dir(name, bdi_debug_root);
-	if (!bdi->debug_dir)
-		return -ENOMEM;
-
-	bdi->debug_stats = debugfs_create_file("stats", 0444, bdi->debug_dir,
-					       bdi, &bdi_debug_stats_fops);
-	if (!bdi->debug_stats) {
-		debugfs_remove(bdi->debug_dir);
-		bdi->debug_dir = NULL;
-		return -ENOMEM;
-	}
 
-	return 0;
+	debugfs_create_file("stats", 0444, bdi->debug_dir, bdi,
+			    &bdi_debug_stats_fops);
 }
 
 static void bdi_debug_unregister(struct backing_dev_info *bdi)
 {
-	debugfs_remove(bdi->debug_stats);
-	debugfs_remove(bdi->debug_dir);
+	debugfs_remove_recursive(bdi->debug_dir);
 }
 #else
 static inline void bdi_debug_init(void)
 {
 }
-static inline int bdi_debug_register(struct backing_dev_info *bdi,
+static inline void bdi_debug_register(struct backing_dev_info *bdi,
 				      const char *name)
 {
-	return 0;
 }
 static inline void bdi_debug_unregister(struct backing_dev_info *bdi)
 {
-- 
2.20.1

Re: [PATCH] backing-dev: no need to check return value of debugfs_create functions

[Sebastian Andrzej Siewior]

On 2019-01-22 16:21:07 [+0100], Greg Kroah-Hartman wrote:
> diff --git a/mm/backing-dev.c b/mm/backing-dev.c
> index 8a8bb8796c6c..85ef344a9c67 100644
> --- a/mm/backing-dev.c
> +++ b/mm/backing-dev.c
> @@ -102,39 +102,25 @@ static int bdi_debug_stats_show(struct seq_file *m, void *v)
>  }
>  DEFINE_SHOW_ATTRIBUTE(bdi_debug_stats);
>  
> -static int bdi_debug_register(struct backing_dev_info *bdi, const char *name)
> +static void bdi_debug_register(struct backing_dev_info *bdi, const char *name)
>  {
> -	if (!bdi_debug_root)
> -		return -ENOMEM;
> -
>  	bdi->debug_dir = debugfs_create_dir(name, bdi_debug_root);

If this fails then ->debug_dir is NULL 

> -	if (!bdi->debug_dir)
> -		return -ENOMEM;
> -
> -	bdi->debug_stats = debugfs_create_file("stats", 0444, bdi->debug_dir,
> -					       bdi, &bdi_debug_stats_fops);
> -	if (!bdi->debug_stats) {
> -		debugfs_remove(bdi->debug_dir);
> -		bdi->debug_dir = NULL;
> -		return -ENOMEM;
> -	}
>  
> -	return 0;
> +	debugfs_create_file("stats", 0444, bdi->debug_dir, bdi,
> +			    &bdi_debug_stats_fops);

then this creates the stats file in the root folder and

>  }
>  
>  static void bdi_debug_unregister(struct backing_dev_info *bdi)
>  {
> -	debugfs_remove(bdi->debug_stats);
> -	debugfs_remove(bdi->debug_dir);
> +	debugfs_remove_recursive(bdi->debug_dir);

this won't remove it.

If you return for "debug_dir == NULL" then it is a nice cleanup.

Sebastian

Re: [PATCH] backing-dev: no need to check return value of debugfs_create functions

[Greg Kroah-Hartman]

On Tue, Jan 22, 2019 at 05:07:59PM +0100, Sebastian Andrzej Siewior wrote:
> On 2019-01-22 16:21:07 [+0100], Greg Kroah-Hartman wrote:
> > diff --git a/mm/backing-dev.c b/mm/backing-dev.c
> > index 8a8bb8796c6c..85ef344a9c67 100644
> > --- a/mm/backing-dev.c
> > +++ b/mm/backing-dev.c
> > @@ -102,39 +102,25 @@ static int bdi_debug_stats_show(struct seq_file *m, void *v)
> >  }
> >  DEFINE_SHOW_ATTRIBUTE(bdi_debug_stats);
> >  
> > -static int bdi_debug_register(struct backing_dev_info *bdi, const char *name)
> > +static void bdi_debug_register(struct backing_dev_info *bdi, const char *name)
> >  {
> > -	if (!bdi_debug_root)
> > -		return -ENOMEM;
> > -
> >  	bdi->debug_dir = debugfs_create_dir(name, bdi_debug_root);
> 
> If this fails then ->debug_dir is NULL 

Wonderful, who cares :)

> > -	if (!bdi->debug_dir)
> > -		return -ENOMEM;
> > -
> > -	bdi->debug_stats = debugfs_create_file("stats", 0444, bdi->debug_dir,
> > -					       bdi, &bdi_debug_stats_fops);
> > -	if (!bdi->debug_stats) {
> > -		debugfs_remove(bdi->debug_dir);
> > -		bdi->debug_dir = NULL;
> > -		return -ENOMEM;
> > -	}
> >  
> > -	return 0;
> > +	debugfs_create_file("stats", 0444, bdi->debug_dir, bdi,
> > +			    &bdi_debug_stats_fops);
> 
> then this creates the stats file in the root folder and

True.

> >  }
> >  
> >  static void bdi_debug_unregister(struct backing_dev_info *bdi)
> >  {
> > -	debugfs_remove(bdi->debug_stats);
> > -	debugfs_remove(bdi->debug_dir);
> > +	debugfs_remove_recursive(bdi->debug_dir);
> 
> this won't remove it.

Which is fine, you don't care.

But step back, how could that original call be NULL?  That only happens
if you pass it a bad parent dentry (which you didn't), or the system is
totally out of memory (in which case you don't care as everything else
is on fire).

> If you return for "debug_dir == NULL" then it is a nice cleanup.

No, that's not a valid thing to check for, you should not care as it
will not happen.  And if it does happen, it's ok, it's only debugfs, no
one can rely on it, it is only for debugging.

thanks,

greg k-h

Re: [PATCH] backing-dev: no need to check return value of debugfs_create functions

[Sebastian Andrzej Siewior]

On 2019-01-22 17:25:03 [+0100], Greg Kroah-Hartman wrote:
> > >  }
> > >  
> > >  static void bdi_debug_unregister(struct backing_dev_info *bdi)
> > >  {
> > > -	debugfs_remove(bdi->debug_stats);
> > > -	debugfs_remove(bdi->debug_dir);
> > > +	debugfs_remove_recursive(bdi->debug_dir);
> > 
> > this won't remove it.
> 
> Which is fine, you don't care.

but if you cat the stats file then it will dereference the bdi struct
which has been free(), right?

> But step back, how could that original call be NULL?  That only happens
> if you pass it a bad parent dentry (which you didn't), or the system is
> totally out of memory (in which case you don't care as everything else
> is on fire).

debugfs_get_inode() could do -ENOMEM and then the directory creation
fails with NULL.

> > If you return for "debug_dir == NULL" then it is a nice cleanup.
> 
> No, that's not a valid thing to check for, you should not care as it
> will not happen.  And if it does happen, it's ok, it's only debugfs, no
> one can rely on it, it is only for debugging.

It might happen with ENOMEM as of now. It could happen for other reasons
in future if the code changes.

> thanks,
> 
> greg k-h

Sebastian

Re: [PATCH] backing-dev: no need to check return value of debugfs_create functions

[Greg Kroah-Hartman]

On Tue, Jan 22, 2019 at 06:19:08PM +0100, Sebastian Andrzej Siewior wrote:
> On 2019-01-22 17:25:03 [+0100], Greg Kroah-Hartman wrote:
> > > >  }
> > > >  
> > > >  static void bdi_debug_unregister(struct backing_dev_info *bdi)
> > > >  {
> > > > -	debugfs_remove(bdi->debug_stats);
> > > > -	debugfs_remove(bdi->debug_dir);
> > > > +	debugfs_remove_recursive(bdi->debug_dir);
> > > 
> > > this won't remove it.
> > 
> > Which is fine, you don't care.
> 
> but if you cat the stats file then it will dereference the bdi struct
> which has been free(), right?

Maybe, I don't know, your code is long gone, it doesn't matter :)

> > But step back, how could that original call be NULL?  That only happens
> > if you pass it a bad parent dentry (which you didn't), or the system is
> > totally out of memory (in which case you don't care as everything else
> > is on fire).
> 
> debugfs_get_inode() could do -ENOMEM and then the directory creation
> fails with NULL.

And if that happens, your system has worse problems :)

> 
> > > If you return for "debug_dir == NULL" then it is a nice cleanup.
> > 
> > No, that's not a valid thing to check for, you should not care as it
> > will not happen.  And if it does happen, it's ok, it's only debugfs, no
> > one can rely on it, it is only for debugging.
> 
> It might happen with ENOMEM as of now. It could happen for other reasons
> in future if the code changes.

As it's been that way for over a decade, I think we will be fine :)
If it changes in the future, in some way that actually matters, I'll go
back and fix up all of the callers.

thanks,

greg k-h

Re: [PATCH] backing-dev: no need to check return value of debugfs_create functions

[Qian Cai]

On 1/22/19 1:33 PM, Greg Kroah-Hartman wrote:
> On Tue, Jan 22, 2019 at 06:19:08PM +0100, Sebastian Andrzej Siewior wrote:
>> On 2019-01-22 17:25:03 [+0100], Greg Kroah-Hartman wrote:
>>>>>  }
>>>>>  
>>>>>  static void bdi_debug_unregister(struct backing_dev_info *bdi)
>>>>>  {
>>>>> -	debugfs_remove(bdi->debug_stats);
>>>>> -	debugfs_remove(bdi->debug_dir);
>>>>> +	debugfs_remove_recursive(bdi->debug_dir);
>>>>
>>>> this won't remove it.
>>>
>>> Which is fine, you don't care.
>>
>> but if you cat the stats file then it will dereference the bdi struct
>> which has been free(), right?
> 
> Maybe, I don't know, your code is long gone, it doesn't matter :)
> 
>>> But step back, how could that original call be NULL?  That only happens
>>> if you pass it a bad parent dentry (which you didn't), or the system is
>>> totally out of memory (in which case you don't care as everything else
>>> is on fire).
>>
>> debugfs_get_inode() could do -ENOMEM and then the directory creation
>> fails with NULL.
> 
> And if that happens, your system has worse problems :)

Well, there are cases that people are running longevity testing on debug kernels
that including OOM and reading all files in sysfs test cases.

Admittedly, the situation right now is not all that healthy as many things are
unable to survive in a low-memory situation, i.e., kmemleak, dma-api debug etc
could just disable themselves.

That's been said, it certainly not necessary to make the situation worse by
triggering a NULL pointer dereferencing or KASAN use-after-free warnings because
of those patches.

Re: [PATCH] backing-dev: no need to check return value of debugfs_create functions

[Sebastian Andrzej Siewior]

On 2019-01-22 19:33:48 [+0100], Greg Kroah-Hartman wrote:
> On Tue, Jan 22, 2019 at 06:19:08PM +0100, Sebastian Andrzej Siewior wrote:
> > but if you cat the stats file then it will dereference the bdi struct
> > which has been free(), right?
> 
> Maybe, I don't know, your code is long gone, it doesn't matter :)

may point is that you may remain with a stats file in debugfs' root
folder which you can cat and then crash.

> > > But step back, how could that original call be NULL?  That only happens
> > > if you pass it a bad parent dentry (which you didn't), or the system is
> > > totally out of memory (in which case you don't care as everything else
> > > is on fire).
> > 
> > debugfs_get_inode() could do -ENOMEM and then the directory creation
> > fails with NULL.
> 
> And if that happens, your system has worse problems :)

So we care to properly handle -ENOMEM in driver's probe function. Those
change find their way to stable kernels.
This unhandled -ENOMEM in debugfs_get_inode() will let
debugfs_create_dir() reuturn NULL. Then debugfs_create_file() will
create the stats in debugfs' root folder. This is a changed behaviour
which is not expected. And then on rmmod the stats file is still present
and will participate in use-after-free if it is read.

> As it's been that way for over a decade, I think we will be fine :)
> If it changes in the future, in some way that actually matters, I'll go
> back and fix up all of the callers.

That is okay then :).
I don't mind if the stats file does not show up due to an error on
probe. It is debugfs after all. However I don't think that it is okay
that the stats file remains in the root folder even after the module has
been removed (and access memory that does not belong to it).

> thanks,
> 
> greg k-h

Sebastian

Re: [PATCH] backing-dev: no need to check return value of debugfs_create functions

[Greg Kroah-Hartman]

On Tue, Jan 22, 2019 at 05:25:03PM +0100, Greg Kroah-Hartman wrote:
> On Tue, Jan 22, 2019 at 05:07:59PM +0100, Sebastian Andrzej Siewior wrote:
> > On 2019-01-22 16:21:07 [+0100], Greg Kroah-Hartman wrote:
> > > diff --git a/mm/backing-dev.c b/mm/backing-dev.c
> > > index 8a8bb8796c6c..85ef344a9c67 100644
> > > --- a/mm/backing-dev.c
> > > +++ b/mm/backing-dev.c
> > > @@ -102,39 +102,25 @@ static int bdi_debug_stats_show(struct seq_file *m, void *v)
> > >  }
> > >  DEFINE_SHOW_ATTRIBUTE(bdi_debug_stats);
> > >  
> > > -static int bdi_debug_register(struct backing_dev_info *bdi, const char *name)
> > > +static void bdi_debug_register(struct backing_dev_info *bdi, const char *name)
> > >  {
> > > -	if (!bdi_debug_root)
> > > -		return -ENOMEM;
> > > -
> > >  	bdi->debug_dir = debugfs_create_dir(name, bdi_debug_root);
> > 
> > If this fails then ->debug_dir is NULL 
> 
> Wonderful, who cares :)

Ok, after sleeping on it, I'll change this function to return an error
if we are out of memory, that way you will not be creating any files in
any other location if you use the return value like this.  That should
solve this issue.

thanks,

greg k-h

Re: [PATCH] backing-dev: no need to check return value of debugfs_create functions

[Sebastian Andrzej Siewior]

On 2019-01-22 16:21:07 [+0100], Greg Kroah-Hartman wrote:
> 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.
> 
> And as the return value does not matter at all, no need to save the
> dentry in struct backing_dev_info, so delete it.
> 
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Anders Roxell <anders.roxell@linaro.org>
> Cc: Arnd Bergmann <arnd@arndb.de>
> Cc: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> Cc: Michal Hocko <mhocko@suse.com>
> Cc: linux-mm@kvack.org
> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

with "[PATCH 2/2] debugfs: return error values, not NULL"
Reviewed-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>

Sebastian