Skip to contentSkip to conversation

x86: mce: no need to check return value of debugfs_create functions

PATCH In review Greg Kroah-Hartman 3 participants 6 messages Jun 12, 2019 – Jun 14, 2019

Subsystems (3)
x86 EDAC-core x86 MCE infrastructure
lore
Share & tools
lore

Greg Kroah-Hartman

author
#0 lore raw

PATCH 1/1 x86: mce: 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.

Cc Thomas Gleixner <[email protected]>
Cc Ingo Molnar <[email protected]>
Cc Borislav Petkov <[email protected]>
Cc "H. Peter Anvin" <[email protected]>
Cc <[email protected]>
Cc Tony Luck <[email protected]>
Cc Vishal Verma <[email protected]>
Cc Pu Wen <[email protected]>
Signed-off-by Greg Kroah-Hartman <[email protected]>
---
 arch/x86/kernel/cpu/mce/core.c     | 16 +++++---------
 arch/x86/kernel/cpu/mce/inject.c   | 34 +++++-------------------------
 arch/x86/kernel/cpu/mce/severity.c | 14 +++---------
 3 files changed, 13 insertions(+), 51 deletions(-)
arch/x86/kernel/cpu/mce/core.c +5 -11
index 282916f3b8d8..fb055b9a486a 100644
@@ -2441,22 +2441,16 @@ static int fake_panic_set(void *data, u64 val)
 DEFINE_DEBUGFS_ATTRIBUTE(fake_panic_fops, fake_panic_get, fake_panic_set,
 			 "%llu\n");
 
-static int __init mcheck_debugfs_init(void)
+static void __init mcheck_debugfs_init(void)
 {
-	struct dentry *dmce, *ffake_panic;
+	struct dentry *dmce;
 
 	dmce = mce_get_debugfs_dir();
-	if (!dmce)
-		return -ENOMEM;
-	ffake_panic = debugfs_create_file_unsafe("fake_panic", 0444, dmce,
-						 NULL, &fake_panic_fops);
-	if (!ffake_panic)
-		return -ENOMEM;
-
-	return 0;
+	debugfs_create_file_unsafe("fake_panic", 0444, dmce, NULL,
+				   &fake_panic_fops);
 }
 #else
-static int __init mcheck_debugfs_init(void) { return -EINVAL; }
+static void __init mcheck_debugfs_init(void) { }
 #endif
 
 DEFINE_STATIC_KEY_FALSE(mcsafe_key);
arch/x86/kernel/cpu/mce/inject.c +5 -29
index 5d108f70f315..97a0d5384c08 100644
@@ -645,7 +645,6 @@ static const struct file_operations readme_fops = {
 
 static struct dfs_node {
 	char *name;
-	struct dentry *d;
 	const struct file_operations *fops;
 	umode_t perm;
 } dfs_fls[] = {

@@ -659,35 +658,16 @@ static struct dfs_node {
 	{ .name = "README",	.fops = &readme_fops, .perm = S_IRUSR | S_IRGRP | S_IROTH },
 };
 
-static int __init debugfs_init(void)
+static void __init debugfs_init(void)
 {
 	unsigned int i;
 
 	dfs_inj = debugfs_create_dir("mce-inject", NULL);
-	if (!dfs_inj)
-		return -EINVAL;
-
-	for (i = 0; i < ARRAY_SIZE(dfs_fls); i++) {
-		dfs_fls[i].d = debugfs_create_file(dfs_fls[i].name,
-						    dfs_fls[i].perm,
-						    dfs_inj,
-						    &i_mce,
-						    dfs_fls[i].fops);
-
-		if (!dfs_fls[i].d)
-			goto err_dfs_add;
-	}
-
-	return 0;
 
-err_dfs_add:
-	while (i-- > 0)
-		debugfs_remove(dfs_fls[i].d);
+	for (i = 0; i < ARRAY_SIZE(dfs_fls); i++)
+		debugfs_create_file(dfs_fls[i].name, dfs_fls[i].perm, dfs_inj,
+				    &i_mce, dfs_fls[i].fops);
 
-	debugfs_remove(dfs_inj);
-	dfs_inj = NULL;
-
-	return -ENODEV;
 }
 
 static int __init inject_init(void)

@@ -697,11 +677,7 @@ static int __init inject_init(void)
 	if (!alloc_cpumask_var(&mce_inject_cpumask, GFP_KERNEL))
 		return -ENOMEM;
 
-	err = debugfs_init();
-	if (err) {
-		free_cpumask_var(mce_inject_cpumask);
-		return err;
-	}
+	debugfs_init();
 
 	register_nmi_handler(NMI_LOCAL, mce_raise_notify, 0, "mce_notify");
 	mce_register_injector_chain(&inject_nb);
arch/x86/kernel/cpu/mce/severity.c +3 -11
index 65201e180fe0..27fd6816e270 100644
@@ -404,21 +404,13 @@ static const struct file_operations severities_coverage_fops = {
 
 static int __init severities_debugfs_init(void)
 {
-	struct dentry *dmce, *fsev;
+	struct dentry *dmce;
 
 	dmce = mce_get_debugfs_dir();
-	if (!dmce)
-		goto err_out;
-
-	fsev = debugfs_create_file("severities-coverage", 0444, dmce, NULL,
-				   &severities_coverage_fops);
-	if (!fsev)
-		goto err_out;
 
+	debugfs_create_file("severities-coverage", 0444, dmce, NULL,
+			    &severities_coverage_fops);
 	return 0;
-
-err_out:
-	return -ENOMEM;
 }
 late_initcall(severities_debugfs_init);
 #endif /* CONFIG_DEBUG_FS */
Signature · 1 line
2.22.0
maintainer
#1 lore raw
Quoted 13 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.

Cc Thomas Gleixner <[email protected]>
Cc Ingo Molnar <[email protected]>
Cc Borislav Petkov <[email protected]>
Cc "H. Peter Anvin" <[email protected]>
Cc <[email protected]>
Cc Tony Luck <[email protected]>
Cc Vishal Verma <[email protected]>
Cc Pu Wen <[email protected]>
Signed-off-by Greg Kroah-Hartman <[email protected]>
Quoted 5 lines
---
 arch/x86/kernel/cpu/mce/core.c     | 16 +++++---------
 arch/x86/kernel/cpu/mce/inject.c   | 34 +++++-------------------------
 arch/x86/kernel/cpu/mce/severity.c | 14 +++---------
 3 files changed, 13 insertions(+), 51 deletions(-)

I think I'm having a deja-vu:

https://lkml.kernel.org/r/[email protected]

Signature · 4 lines
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.
author
#2 lore raw
Quoted 20 lines · Borislav Petkov
Quoted 13 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.

Cc Thomas Gleixner <[email protected]>
Cc Ingo Molnar <[email protected]>
Cc Borislav Petkov <[email protected]>
Cc "H. Peter Anvin" <[email protected]>
Cc <[email protected]>
Cc Tony Luck <[email protected]>
Cc Vishal Verma <[email protected]>
Cc Pu Wen <[email protected]>
Signed-off-by Greg Kroah-Hartman <[email protected]>
Quoted 5 lines
---
 arch/x86/kernel/cpu/mce/core.c     | 16 +++++---------
 arch/x86/kernel/cpu/mce/inject.c   | 34 +++++-------------------------
 arch/x86/kernel/cpu/mce/severity.c | 14 +++---------
 3 files changed, 13 insertions(+), 51 deletions(-)
Quoted 3 lines

I think I'm having a deja-vu:

https://lkml.kernel.org/r/[email protected]

Ah, I thought I had sent this out before...

Anyway, I'll fix this up, but really, you will not have a debugfs file fail creation unless the system is totally out of memory...

thanks,

greg k-h

maintainer
#3 lore raw
Quoted 21 lines · Greg Kroah-Hartman
Quoted 20 lines · Borislav Petkov
Quoted 18 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.

Cc Thomas Gleixner <[email protected]>
Cc Ingo Molnar <[email protected]>
Cc Borislav Petkov <[email protected]>
Cc "H. Peter Anvin" <[email protected]>
Cc <[email protected]>
Cc Tony Luck <[email protected]>
Cc Vishal Verma <[email protected]>
Cc Pu Wen <[email protected]>
Signed-off-by Greg Kroah-Hartman <[email protected]>
---
 arch/x86/kernel/cpu/mce/core.c     | 16 +++++---------
 arch/x86/kernel/cpu/mce/inject.c   | 34 +++++-------------------------
 arch/x86/kernel/cpu/mce/severity.c | 14 +++---------
 3 files changed, 13 insertions(+), 51 deletions(-)
Quoted 8 lines
Quoted 3 lines

I think I'm having a deja-vu:

https://lkml.kernel.org/r/[email protected]

Ah, I thought I had sent this out before...

Anyway, I'll fix this up, but really, you will not have a debugfs file fail creation unless the system is totally out of memory...

Promise? :-P

I mean, I don't mind getting rid of all that error handling getting in the way of the code but we'll leave the injector in a half-init state if the allocation fails.

I guess I can take this and see what breaks. If it does, we can always revert it...

Btw, is your aim to make debugfs_create_file() return void or you're just doing some cleanups?

Signature · 4 lines
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.
author
#4 lore raw
Quoted 24 lines · Borislav Petkov
Quoted 23 lines · Greg Kroah-Hartman
Quoted 22 lines · Borislav Petkov
Quoted 13 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.

Cc Thomas Gleixner <[email protected]>
Cc Ingo Molnar <[email protected]>
Cc Borislav Petkov <[email protected]>
Cc "H. Peter Anvin" <[email protected]>
Cc <[email protected]>
Cc Tony Luck <[email protected]>
Cc Vishal Verma <[email protected]>
Cc Pu Wen <[email protected]>
Signed-off-by Greg Kroah-Hartman <[email protected]>
Quoted 5 lines
---
 arch/x86/kernel/cpu/mce/core.c     | 16 +++++---------
 arch/x86/kernel/cpu/mce/inject.c   | 34 +++++-------------------------
 arch/x86/kernel/cpu/mce/severity.c | 14 +++---------
 3 files changed, 13 insertions(+), 51 deletions(-)

I think I'm having a deja-vu:

Quoted 8 lines
Quoted 6 lines
Quoted 1 line

https://lkml.kernel.org/r/[email protected]

Ah, I thought I had sent this out before...

Anyway, I'll fix this up, but really, you will not have a debugfs file fail creation unless the system is totally out of memory...

Promise? :-P

Yes, the only way this can fail is if:
debugfs superblock can not be pinned - something really went wrong with the vfs layer file is created with same name - the caller's fault
new_inode() fails - happens if memory is exhausted

So yes, I do promise :)

Quoted 3 lines

I mean, I don't mind getting rid of all that error handling getting in the way of the code but we'll leave the injector in a half-init state if the allocation fails.

That's fine, your system is hosed then.

Quoted 2 lines

I guess I can take this and see what breaks. If it does, we can always revert it...

Yup!

Quoted 2 lines

Btw, is your aim to make debugfs_create_file() return void or you're just doing some cleanups?

Yes, that is my long-term goal here. I don't think it will be possible as there are some valid users that only want a single file and then remove the file later, but I'll deal with that when I'm done sweeping the tree. I'm already able to start removing the return value for many of the other debugfs "helper file" creation functions.

thanks,

greg k-h

merge bot
#5 lore raw

[tip:ras/core] x86/mce: Do not check return value of debugfs_create functions

In reply to Greg Kroah-Hartman
x86/mce: Do not check return value of debugfs_create functions
Commit-ID:  6e4f929ea8b2097b0052f6674de839a3c9d477e9
Gitweb:     https://git.kernel.org/tip/6e4f929ea8b2097b0052f6674de839a3c9d477e9
Author:     Greg Kroah-Hartman <[email protected]>
AuthorDate: Wed, 12 Jun 2019 17:15:31 +0200
Committer:  Borislav Petkov <[email protected]>
CommitDate: Fri, 14 Jun 2019 16:04:21 +0200

x86/mce: Do not 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.

The only way this can fail is if:

* debugfs superblock can not be pinned - something really went wrong with the vfs layer.
* file is created with same name - the caller's fault.
* new_inode() fails - happens if memory is exhausted.

so failing to clean up debugfs properly is the least of the system's sproblems in uch a situation.

[ bp: Extend commit message, remove unused err var in inject_init(). ]

Signed-off-by Greg Kroah-Hartman <[email protected]>
Signed-off-by Borislav Petkov <[email protected]>
Cc "H. Peter Anvin" <[email protected]>
Cc Ingo Molnar <[email protected]>
Cc linux-edac <[email protected]>
Cc Thomas Gleixner <[email protected]>
Cc Tony Luck <[email protected]>
Cc x86-ml <[email protected]>
Link https://lkml.kernel.org/r/[email protected] open here
---
 arch/x86/kernel/cpu/mce/core.c     | 16 +++++-----------
 arch/x86/kernel/cpu/mce/inject.c   | 37 +++++--------------------------------
 arch/x86/kernel/cpu/mce/severity.c | 14 +++-----------
 3 files changed, 13 insertions(+), 54 deletions(-)
arch/x86/kernel/cpu/mce/core.c +5 -11
index c2c93e9195ed..066562a1ea20 100644
@@ -2516,22 +2516,16 @@ static int fake_panic_set(void *data, u64 val)
 DEFINE_DEBUGFS_ATTRIBUTE(fake_panic_fops, fake_panic_get, fake_panic_set,
 			 "%llu\n");
 
-static int __init mcheck_debugfs_init(void)
+static void __init mcheck_debugfs_init(void)
 {
-	struct dentry *dmce, *ffake_panic;
+	struct dentry *dmce;
 
 	dmce = mce_get_debugfs_dir();
-	if (!dmce)
-		return -ENOMEM;
-	ffake_panic = debugfs_create_file_unsafe("fake_panic", 0444, dmce,
-						 NULL, &fake_panic_fops);
-	if (!ffake_panic)
-		return -ENOMEM;
-
-	return 0;
+	debugfs_create_file_unsafe("fake_panic", 0444, dmce, NULL,
+				   &fake_panic_fops);
 }
 #else
-static int __init mcheck_debugfs_init(void) { return -EINVAL; }
+static void __init mcheck_debugfs_init(void) { }
 #endif
 
 DEFINE_STATIC_KEY_FALSE(mcsafe_key);
arch/x86/kernel/cpu/mce/inject.c +5 -32
index 5d108f70f315..1f30117b24ba 100644
@@ -645,7 +645,6 @@ static const struct file_operations readme_fops = {
 
 static struct dfs_node {
 	char *name;
-	struct dentry *d;
 	const struct file_operations *fops;
 	umode_t perm;
 } dfs_fls[] = {

@@ -659,49 +658,23 @@ static struct dfs_node {
 	{ .name = "README",	.fops = &readme_fops, .perm = S_IRUSR | S_IRGRP | S_IROTH },
 };
 
-static int __init debugfs_init(void)
+static void __init debugfs_init(void)
 {
 	unsigned int i;
 
 	dfs_inj = debugfs_create_dir("mce-inject", NULL);
-	if (!dfs_inj)
-		return -EINVAL;
-
-	for (i = 0; i < ARRAY_SIZE(dfs_fls); i++) {
-		dfs_fls[i].d = debugfs_create_file(dfs_fls[i].name,
-						    dfs_fls[i].perm,
-						    dfs_inj,
-						    &i_mce,
-						    dfs_fls[i].fops);
-
-		if (!dfs_fls[i].d)
-			goto err_dfs_add;
-	}
-
-	return 0;
-
-err_dfs_add:
-	while (i-- > 0)
-		debugfs_remove(dfs_fls[i].d);
 
-	debugfs_remove(dfs_inj);
-	dfs_inj = NULL;
-
-	return -ENODEV;
+	for (i = 0; i < ARRAY_SIZE(dfs_fls); i++)
+		debugfs_create_file(dfs_fls[i].name, dfs_fls[i].perm, dfs_inj,
+				    &i_mce, dfs_fls[i].fops);
 }
 
 static int __init inject_init(void)
 {
-	int err;
-
 	if (!alloc_cpumask_var(&mce_inject_cpumask, GFP_KERNEL))
 		return -ENOMEM;
 
-	err = debugfs_init();
-	if (err) {
-		free_cpumask_var(mce_inject_cpumask);
-		return err;
-	}
+	debugfs_init();
 
 	register_nmi_handler(NMI_LOCAL, mce_raise_notify, 0, "mce_notify");
 	mce_register_injector_chain(&inject_nb);
arch/x86/kernel/cpu/mce/severity.c +3 -11
index 65201e180fe0..27fd6816e270 100644
@@ -404,21 +404,13 @@ static const struct file_operations severities_coverage_fops = {
 
 static int __init severities_debugfs_init(void)
 {
-	struct dentry *dmce, *fsev;
+	struct dentry *dmce;
 
 	dmce = mce_get_debugfs_dir();
-	if (!dmce)
-		goto err_out;
-
-	fsev = debugfs_create_file("severities-coverage", 0444, dmce, NULL,
-				   &severities_coverage_fops);
-	if (!fsev)
-		goto err_out;
 
+	debugfs_create_file("severities-coverage", 0444, dmce, NULL,
+			    &severities_coverage_fops);
 	return 0;
-
-err_out:
-	return -ENOMEM;
 }
 late_initcall(severities_debugfs_init);
 #endif /* CONFIG_DEBUG_FS */