| Message ID | 20190122152151.16139-8-gregkh@linuxfoundation.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | backing-dev: no need to check return value of debugfs_create functions | expand |
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
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
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
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
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.
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
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
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
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) {
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(-)