[PATCH] mm/memblock: validate the creation of debugfs files

[PATCH] mm/memblock: validate the creation of debugfs files

[Alexander Kuleshov]

Signed-off-by: Alexander Kuleshov <kuleshovmail@gmail.com>
---
 mm/memblock.c | 24 +++++++++++++++++++++---
 1 file changed, 21 insertions(+), 3 deletions(-)

diff --git a/mm/memblock.c b/mm/memblock.c
index 87108e7..c09e911 100644
--- a/mm/memblock.c
+++ b/mm/memblock.c
@@ -1692,16 +1692,34 @@ static const struct file_operations memblock_debug_fops = {
 
 static int __init memblock_init_debugfs(void)
 {
+	struct dentry *f;
 	struct dentry *root = debugfs_create_dir("memblock", NULL);
 	if (!root)
 		return -ENXIO;
-	debugfs_create_file("memory", S_IRUGO, root, &memblock.memory, &memblock_debug_fops);
-	debugfs_create_file("reserved", S_IRUGO, root, &memblock.reserved, &memblock_debug_fops);
+
+	f = debugfs_create_file("memory", S_IRUGO, root, &memblock.memory, &memblock_debug_fops);
+	if (!f) {
+		pr_err("Failed to create memory debugfs file\n");
+		goto err_out;
+	}
+
+	f = debugfs_create_file("reserved", S_IRUGO, root, &memblock.reserved, &memblock_debug_fops);
+	if (!f) {
+		pr_err("Failed to create reserved debugfs file\n");
+		goto err_out;
+	}
+
 #ifdef CONFIG_HAVE_MEMBLOCK_PHYS_MAP
-	debugfs_create_file("physmem", S_IRUGO, root, &memblock.physmem, &memblock_debug_fops);
+	f = debugfs_create_file("physmem", S_IRUGO, root, &memblock.physmem, &memblock_debug_fops);
+	if (!f) {
+		pr_err("Failed to create physmem debugfs file\n");
+		goto err_out;
+	}
 #endif
 
 	return 0;
+err_out:
+	return -ENOMEM;
 }
 __initcall(memblock_init_debugfs);
 
-- 
2.5.0

[PATCH] misc/mei: Fix debugfs filename in error output

[Alexander Kuleshov]

Signed-off-by: Alexander Kuleshov <kuleshovmail@gmail.com>
---
 drivers/misc/mei/debugfs.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/misc/mei/debugfs.c b/drivers/misc/mei/debugfs.c
index eb86834..5e3a588 100644
--- a/drivers/misc/mei/debugfs.c
+++ b/drivers/misc/mei/debugfs.c
@@ -207,7 +207,7 @@ int mei_dbgfs_register(struct mei_device *dev, const char *name)
 	f = debugfs_create_file("active", S_IRUSR, dir,
 				dev, &mei_dbgfs_fops_active);
 	if (!f) {
-		dev_err(dev->dev, "meclients: registration failed\n");
+		dev_err(dev->dev, "active: registration failed\n");
 		goto err;
 	}
 	f = debugfs_create_file("devstate", S_IRUSR, dir,
-- 
2.5.0

Re: [PATCH] mm/memblock: validate the creation of debugfs files

[Alexander Kuleshov]

On 08-15-15, Alexander Kuleshov wrote:
> Signed-off-by: Alexander Kuleshov <kuleshovmail@gmail.com>
> ---
>  mm/memblock.c | 24 +++++++++++++++++++++---
>  1 file changed, 21 insertions(+), 3 deletions(-)
>

Sorry for noise, skip the first patch please.

Re: [PATCH] mm/memblock: validate the creation of debugfs files

[Andrew Morton]

On Sat, 15 Aug 2015 09:07:30 -0700 Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote:

> > > in the kernel/kprobes and etc.), besides this, the memblock API is used
> > > mostly at early stage, so we will have some output if something going wrong.
> > 
> > The debugfs error-handling rules are something Greg cooked up after one
> > too many beers.  I've never understood them, but maybe I continue to
> > miss the point.
> 
> The "point" is that it should be easy to use, and you don't care if the
> file fails to be created because your normal code flow / functionality
> does not care if a debugfs file fails to be created.
> 
> The only way a debugfs file will fail to be created is if you name
> something the same as a file is present, or you passed in the wrong
> options, or if you are out of memory, and in all of those cases, there's
> nothing a user can do about it.  Yes, when writing your code the first
> time, check the error if you want to figure out your logic, but after
> that, you don't care.
> 
> If debugfs is not enabled, yes, an error will be returned, but you don't
> have to care about that, because again, you don't care, and your main
> code path is just fine.
> 
> So just ignore the return value of debugfs functions, except to save off
> pointers that you need to pass back in them later.
> 
> > Yes, I agree that if memblock's debugfs_create_file() fails, we want to
> > know about it because something needs fixing.
> 
> What can be fixed?  Out of memory?  Identical file name?  Nothing a user
> can do about that.

wha?  We have thousands and thousands of assertions in the kernel and
there's nothing the user can do about any them, apart from sending us a
bug report.

If debugfs_create_file() fails then something is messed up in the
kernel.  The kernel error shouldn't just be ignored!  It should be
reported and fixed.

> > But that's true of
> > all(?) debugfs_create_file callsites, so it's a bit silly to add
> > warnings to them all.  Why not put the warning into
> > debugfs_create_file() itself?  And add a debugfs_create_file_no_warn()
> > if there are callsites which have reason to go it alone.  Or add a
> > debugfs_create_file_warn() wrapper.
> 
> No, it's really not worth it.  The goal of debugfs was to make an api
> that is easier to use than procfs which required a bunch of odd return
> error checks and you could never tell if the error was due to something
> real or if the procfs was not enabled in the kernel.
> 
> And it's for debugging files, again, nothing that should be something
> you rely on.  If you rely on debugfs files for something, well, you are
> using the wrong api (yes, I know all about the trace nightmare...)

Yeah.  That's just wrong.  debugfs is just kernel code.  If it goes
wrong we should handle that in the usual way, so it gets fixed.

Re: [PATCH] mm/memblock: validate the creation of debugfs files

[Greg Kroah-Hartman]

On Sat, Aug 15, 2015 at 12:38:30AM -0700, Andrew Morton wrote:
> On Sat, 15 Aug 2015 13:26:36 +0600 Alexander Kuleshov <kuleshovmail@gmail.com> wrote:
> 
> > Hello Andrew,
> > 
> > On 08-14-15, Andrew Morton wrote:
> > > On Sat, 15 Aug 2015 01:03:31 +0600 Alexander Kuleshov <kuleshovmail@gmail.com> wrote:
> > > 
> > > > Signed-off-by: Alexander Kuleshov <kuleshovmail@gmail.com>
> > > 
> > > There's no changelog.
> > 
> > Yes, will add it if there will be sense in the patch.
> > 
> > > 
> > > Why?  Ignoring the debugfs API return values is standard practice.
> > > 
> > 
> > Yes, but I saw many places where this practice is applicable (for example
> > in the kernel/kprobes and etc.), besides this, the memblock API is used
> > mostly at early stage, so we will have some output if something going wrong.
> 
> The debugfs error-handling rules are something Greg cooked up after one
> too many beers.  I've never understood them, but maybe I continue to
> miss the point.

The "point" is that it should be easy to use, and you don't care if the
file fails to be created because your normal code flow / functionality
does not care if a debugfs file fails to be created.

The only way a debugfs file will fail to be created is if you name
something the same as a file is present, or you passed in the wrong
options, or if you are out of memory, and in all of those cases, there's
nothing a user can do about it.  Yes, when writing your code the first
time, check the error if you want to figure out your logic, but after
that, you don't care.

If debugfs is not enabled, yes, an error will be returned, but you don't
have to care about that, because again, you don't care, and your main
code path is just fine.

So just ignore the return value of debugfs functions, except to save off
pointers that you need to pass back in them later.

> Yes, I agree that if memblock's debugfs_create_file() fails, we want to
> know about it because something needs fixing.

What can be fixed?  Out of memory?  Identical file name?  Nothing a user
can do about that.

> But that's true of
> all(?) debugfs_create_file callsites, so it's a bit silly to add
> warnings to them all.  Why not put the warning into
> debugfs_create_file() itself?  And add a debugfs_create_file_no_warn()
> if there are callsites which have reason to go it alone.  Or add a
> debugfs_create_file_warn() wrapper.

No, it's really not worth it.  The goal of debugfs was to make an api
that is easier to use than procfs which required a bunch of odd return
error checks and you could never tell if the error was due to something
real or if the procfs was not enabled in the kernel.

And it's for debugging files, again, nothing that should be something
you rely on.  If you rely on debugfs files for something, well, you are
using the wrong api (yes, I know all about the trace nightmare...)

thanks,

greg k-h

Re: [PATCH] mm/memblock: validate the creation of debugfs files

[Andrew Morton]

On Sat, 15 Aug 2015 13:48:50 +0600 Alexander Kuleshov <kuleshovmail@gmail.com> wrote:

> On 08-15-15, Andrew Morton wrote:
> > Yes, I agree that if memblock's debugfs_create_file() fails, we want to
> > know about it because something needs fixing.  But that's true of
> > all(?) debugfs_create_file callsites, so it's a bit silly to add
> > warnings to them all.  Why not put the warning into
> > debugfs_create_file() itself?
> 
> Good idea, but there are already some debugfs_create_file calls with checks and
> warning, if these checks failed. I don't know how many, but I saw it.
> Double warning is not good too.

Please ponder the sentence you deleted.  "Or add a
debugfs_create_file_warn() wrapper".

Re: [PATCH] mm/memblock: validate the creation of debugfs files

[Alexander Kuleshov]

On 08-15-15, Andrew Morton wrote:
> Yes, I agree that if memblock's debugfs_create_file() fails, we want to
> know about it because something needs fixing.  But that's true of
> all(?) debugfs_create_file callsites, so it's a bit silly to add
> warnings to them all.  Why not put the warning into
> debugfs_create_file() itself?

Good idea, but there are already some debugfs_create_file calls with checks and
warning, if these checks failed. I don't know how many, but I saw it.
Double warning is not good too.

Re: [PATCH] mm/memblock: validate the creation of debugfs files

[Andrew Morton]

On Sat, 15 Aug 2015 13:26:36 +0600 Alexander Kuleshov <kuleshovmail@gmail.com> wrote:

> Hello Andrew,
> 
> On 08-14-15, Andrew Morton wrote:
> > On Sat, 15 Aug 2015 01:03:31 +0600 Alexander Kuleshov <kuleshovmail@gmail.com> wrote:
> > 
> > > Signed-off-by: Alexander Kuleshov <kuleshovmail@gmail.com>
> > 
> > There's no changelog.
> 
> Yes, will add it if there will be sense in the patch.
> 
> > 
> > Why?  Ignoring the debugfs API return values is standard practice.
> > 
> 
> Yes, but I saw many places where this practice is applicable (for example
> in the kernel/kprobes and etc.), besides this, the memblock API is used
> mostly at early stage, so we will have some output if something going wrong.

The debugfs error-handling rules are something Greg cooked up after one
too many beers.  I've never understood them, but maybe I continue to
miss the point.

Yes, I agree that if memblock's debugfs_create_file() fails, we want to
know about it because something needs fixing.  But that's true of
all(?) debugfs_create_file callsites, so it's a bit silly to add
warnings to them all.  Why not put the warning into
debugfs_create_file() itself?  And add a debugfs_create_file_no_warn()
if there are callsites which have reason to go it alone.  Or add a
debugfs_create_file_warn() wrapper.

Re: [PATCH] mm/memblock: validate the creation of debugfs files

[Alexander Kuleshov]

Hello Andrew,

On 08-14-15, Andrew Morton wrote:
> On Sat, 15 Aug 2015 01:03:31 +0600 Alexander Kuleshov <kuleshovmail@gmail.com> wrote:
> 
> > Signed-off-by: Alexander Kuleshov <kuleshovmail@gmail.com>
> 
> There's no changelog.

Yes, will add it if there will be sense in the patch.

> 
> Why?  Ignoring the debugfs API return values is standard practice.
> 

Yes, but I saw many places where this practice is applicable (for example
in the kernel/kprobes and etc.), besides this, the memblock API is used
mostly at early stage, so we will have some output if something going wrong.

Re: [PATCH] mm/memblock: validate the creation of debugfs files

[Andrew Morton]

On Sat, 15 Aug 2015 01:03:31 +0600 Alexander Kuleshov <kuleshovmail@gmail.com> wrote:

> Signed-off-by: Alexander Kuleshov <kuleshovmail@gmail.com>

There's no changelog.

> --- a/mm/memblock.c
> +++ b/mm/memblock.c
> @@ -1692,16 +1692,34 @@ static const struct file_operations memblock_debug_fops = {
>  
>  static int __init memblock_init_debugfs(void)
>  {
> +	struct dentry *f;
>  	struct dentry *root = debugfs_create_dir("memblock", NULL);
>  	if (!root)
>  		return -ENXIO;
> -	debugfs_create_file("memory", S_IRUGO, root, &memblock.memory, &memblock_debug_fops);
> -	debugfs_create_file("reserved", S_IRUGO, root, &memblock.reserved, &memblock_debug_fops);
> +
> +	f = debugfs_create_file("memory", S_IRUGO, root, &memblock.memory, &memblock_debug_fops);
> +	if (!f) {
> +		pr_err("Failed to create memory debugfs file\n");
> +		goto err_out;

Why?  Ignoring the debugfs API return values is standard practice.

[PATCH] mm/memblock: validate the creation of debugfs files

[Alexander Kuleshov]

Signed-off-by: Alexander Kuleshov <kuleshovmail@gmail.com>
---
 mm/memblock.c | 24 +++++++++++++++++++++---
 1 file changed, 21 insertions(+), 3 deletions(-)

diff --git a/mm/memblock.c b/mm/memblock.c
index 87108e7..c09e911 100644
--- a/mm/memblock.c
+++ b/mm/memblock.c
@@ -1692,16 +1692,34 @@ static const struct file_operations memblock_debug_fops = {
 
 static int __init memblock_init_debugfs(void)
 {
+	struct dentry *f;
 	struct dentry *root = debugfs_create_dir("memblock", NULL);
 	if (!root)
 		return -ENXIO;
-	debugfs_create_file("memory", S_IRUGO, root, &memblock.memory, &memblock_debug_fops);
-	debugfs_create_file("reserved", S_IRUGO, root, &memblock.reserved, &memblock_debug_fops);
+
+	f = debugfs_create_file("memory", S_IRUGO, root, &memblock.memory, &memblock_debug_fops);
+	if (!f) {
+		pr_err("Failed to create memory debugfs file\n");
+		goto err_out;
+	}
+
+	f = debugfs_create_file("reserved", S_IRUGO, root, &memblock.reserved, &memblock_debug_fops);
+	if (!f) {
+		pr_err("Failed to create reserved debugfs file\n");
+		goto err_out;
+	}
+
 #ifdef CONFIG_HAVE_MEMBLOCK_PHYS_MAP
-	debugfs_create_file("physmem", S_IRUGO, root, &memblock.physmem, &memblock_debug_fops);
+	f = debugfs_create_file("physmem", S_IRUGO, root, &memblock.physmem, &memblock_debug_fops);
+	if (!f) {
+		pr_err("Failed to create physmem debugfs file\n");
+		goto err_out;
+	}
 #endif
 
 	return 0;
+err_out:
+	return -ENOMEM;
 }
 __initcall(memblock_init_debugfs);
 
-- 
2.5.0