backing-dev: no need to check return value of debugfs_create functions
Subsystems (2)
PATCH 1/1 backing-dev: no need to check return value of debugfs_create functions
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.
--- include/linux/backing-dev-defs.h | 1 - mm/backing-dev.c | 24 +++++------------------- 2 files changed, 5 insertions(+), 20 deletions(-)
include/linux/backing-dev-defs.h +0 -1
@@ -202,7 +202,6 @@ struct backing_dev_info {#ifdef CONFIG_DEBUG_FS struct dentry *debug_dir; -struct dentry *debug_stats;#endif };
mm/backing-dev.c +5 -19
@@ -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) {
Signature · 1 line
2.20.1
Quoted 5 lines · Greg Kroah-Hartman
mm/backing-dev.c +0
-0
mm/backing-dev.c static int bdi_debug_stats_show(struct seq_file *m, void *v) · line 107
Quoted 10 lines
mm/backing-dev.c +1
-4@@ -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
Quoted 4 lines
mm/backing-dev.c +0
-4-if (!bdi->debug_dir) -return -ENOMEM ; - - bdi->debug_stats = debugfs_create_file("stats", 0444, bdi->debug_dir,
mm/backing-dev.c static int bdi_debug_stats_show(struct seq_file *m, void *v) · line 110
Quoted 10 lines
mm/backing-dev.c +2
-7- 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
mm/backing-dev.c static int bdi_debug_stats_show(struct seq_file *m, void *v) · line 115
Quoted 7 lines
mm/backing-dev.c +1
-2}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
Quoted 9 lines · Sebastian Andrzej Siewior
Quoted 8 lines · Greg Kroah-Hartman
mm/backing-dev.c +0
-0@@ -102,39 +102,25 @@ static int bdi_debug_stats_show(struct seq_file *m, void *v) }DEFINE_SHOW_ATTRIBUTE (bdi_debug_stats);
Quoted 9 lines
mm/backing-dev.c static int bdi_debug_stats_show(struct seq_file *m, void *v) · line 107
Quoted 7 lines
mm/backing-dev.c +1
-4-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 :)
Quoted 11 lines
Quoted 11 lines
mm/backing-dev.c +0
-10-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 ; - }
Quoted 5 lines
mm/backing-dev.c static int bdi_debug_stats_show(struct seq_file *m, void *v) · line 110
Quoted 3 lines
mm/backing-dev.c +2
-1-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.
Quoted 9 lines
mm/backing-dev.c static int bdi_debug_stats_show(struct seq_file *m, void *v) · line 115
Quoted 7 lines
mm/backing-dev.c +1
-2}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).
Quoted 1 line
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
Quoted 11 lines · Greg Kroah-Hartman
Quoted 9 lines
mm/backing-dev.c static int bdi_debug_stats_show(struct seq_file *m, void *v) · line 115
Quoted 7 lines
}
static void
bdi_debug_unregister(struct backing_dev_info *bdi){mm/backing-dev.c +1
-2-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?
Quoted 4 lines
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.
Quoted 5 lines
Quoted 1 line
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.
Quoted 3 lines
thanks,
greg k-h
Sebastian
Quoted 11 lines · Sebastian Andrzej Siewior
Quoted 10 lines · Greg Kroah-Hartman
Quoted 9 lines
mm/backing-dev.c static int bdi_debug_stats_show(struct seq_file *m, void *v) · line 115
Quoted 7 lines
}
static void
bdi_debug_unregister(struct backing_dev_info *bdi){mm/backing-dev.c +1
-2-debugfs_remove (bdi->debug_stats); -debugfs_remove (bdi->debug_dir); +debugfs_remove_recursive (bdi->debug_dir);this won't remove it.
Quoted 4 lines
Quoted 1 line
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 :)
Quoted 7 lines
Quoted 4 lines
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-ENOMEMand then the directory creation fails with NULL.
And if that happens, your system has worse problems :)
Quoted 9 lines
Quoted 6 lines
Quoted 2 lines
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
Quoted 24 lines · Greg Kroah-Hartman
Quoted 11 lines · Sebastian Andrzej Siewior
Quoted 10 lines · Greg Kroah-Hartman
Quoted 9 lines
mm/backing-dev.c static int bdi_debug_stats_show(struct seq_file *m, void *v) · line 115
Quoted 7 lines
}
static void
bdi_debug_unregister(struct backing_dev_info *bdi){mm/backing-dev.c +1
-2-debugfs_remove (bdi->debug_stats); -debugfs_remove (bdi->debug_dir); +debugfs_remove_recursive (bdi->debug_dir);this won't remove it.
Quoted 4 lines
Quoted 1 line
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 :)
Quoted 5 lines
Quoted 4 lines
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).
Quoted 4 lines
Quoted 2 lines
debugfs_get_inode()could do-ENOMEMand 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.
Quoted 5 lines · Greg Kroah-Hartman
Quoted 2 lines · Sebastian Andrzej Siewior
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.
Quoted 9 lines
Quoted 7 lines
Quoted 4 lines
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-ENOMEMand 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.
Quoted 3 lines
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).
Quoted 3 lines
thanks,
greg k-h
Sebastian
Quoted 20 lines · Greg Kroah-Hartman
Quoted 9 lines · Sebastian Andrzej Siewior
Quoted 8 lines · Greg Kroah-Hartman
mm/backing-dev.c +0
-0@@ -102,39 +102,25 @@ static int bdi_debug_stats_show(struct seq_file *m, void *v) }DEFINE_SHOW_ATTRIBUTE (bdi_debug_stats);Quoted 9 lines
mm/backing-dev.c static int bdi_debug_stats_show(struct seq_file *m, void *v) · line 107
Quoted 7 lines
mm/backing-dev.c +1
-4-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
Quoted 1 line
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
Quoted 7 lines · 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.
Quoted 7 lines
Cc Andrew Morton <[email protected]>Cc Anders Roxell <[email protected]>Cc Arnd Bergmann <[email protected]>Cc Sebastian Andrzej Siewior <[email protected]>Cc Michal Hocko <[email protected]>Signed-off-by Greg Kroah-Hartman <[email protected]>
with "[PATCH 2/2] debugfs: return error values, not NULL"
Sebastian