kernel/params.c: print failure information instead of 'KOBJ_ADD' to user space, when sysfs_create_file() fails.

PATCH In review Chen Gang 3 participants 10 messages Jul 9, 2013 – Jul 22, 2013

Share & tools

Chen Gang

author

2013-07-09 03:38 UTC #0 lore raw

PATCH 1/1 kernel/params.c: print failure information instead of 'KOBJ_ADD' to user space, when sysfs_create_file() fails.

When sysfs_create_file() fails, recommend to print the related failure information. And it is useless to still 'KOBJ_ADD' to user space.

Signed-off-by Chen Gang <[email protected]>

---
 kernel/params.c |    8 +++++++-
 1 files changed, 7 insertions(+), 1 deletions(-)

kernel/params.c +7 -1

index 440e65d..f5299c1 100644
@@ -845,7 +845,13 @@ static void __init version_sysfs_builtin(void)
 		mk = locate_module_kobject(vattr->module_name);
 		if (mk) {
 			err = sysfs_create_file(&mk->kobj, &vattr->mattr.attr);
-			kobject_uevent(&mk->kobj, KOBJ_ADD);
+			if (err)
+				printk(KERN_WARNING
+				       "%s (%d): sysfs_create_file fail for %s, err: %d\n",
+				       __FILE__, __LINE__,
+				       vattr->module_name, err);
+			else
+				kobject_uevent(&mk->kobj, KOBJ_ADD);
 			kobject_put(&mk->kobj);
 		}
 	}

Signature · 1 line

1.7.7.6

Rusty Russell

2013-07-10 01:49 UTC #1 lore raw

Quoted 4 lines · Chen Gang

When sysfs_create_file() fails, recommend to print the related failure information. And it is useless to still 'KOBJ_ADD' to user space.
Signed-off-by Chen Gang <[email protected]>

sysfs_create_file() should not fail during boot, should it?

Cheers,
Rusty.

Quoted 25 lines

---
 kernel/params.c |    8 +++++++-
 1 files changed, 7 insertions(+), 1 deletions(-)

kernel/params.c +7 -1

index 440e65d..f5299c1 100644
@@ -845,7 +845,13 @@ static void __init version_sysfs_builtin(void)
 		mk = locate_module_kobject(vattr->module_name);
 		if (mk) {
 			err = sysfs_create_file(&mk->kobj, &vattr->mattr.attr);
-			kobject_uevent(&mk->kobj, KOBJ_ADD);
+			if (err)
+				printk(KERN_WARNING
+				       "%s (%d): sysfs_create_file fail for %s, err: %d\n",
+				       __FILE__, __LINE__,
+				       vattr->module_name, err);
+			else
+				kobject_uevent(&mk->kobj, KOBJ_ADD);
 			kobject_put(&mk->kobj);
 		}
 	}

Signature · 1 line

1.7.7.6

Chen Gang F T

2013-07-10 02:18 UTC #2 lore raw

Quoted 8 lines · Rusty Russell

Quoted 4 lines · Chen Gang
When sysfs_create_file() fails, recommend to print the related failure information. And it is useless to still 'KOBJ_ADD' to user space.
Signed-off-by Chen Gang <[email protected]>
sysfs_create_file() should not fail during boot, should it?

Hmm..., please reference locate_module_kobject() in "kernel/params.c", which is an '__init' function, and also call sysfs_create_file(), it processes the related error.

So I recommend to get the check too in version_sysfs_builtin().

Thanks.

Quoted 34 lines

Cheers,
Rusty.

Quoted 25 lines

---
 kernel/params.c |    8 +++++++-
 1 files changed, 7 insertions(+), 1 deletions(-)

kernel/params.c +7 -1

index 440e65d..f5299c1 100644
@@ -845,7 +845,13 @@ static void __init version_sysfs_builtin(void)
 		mk = locate_module_kobject(vattr->module_name);
 		if (mk) {
 			err = sysfs_create_file(&mk->kobj, &vattr->mattr.attr);
-			kobject_uevent(&mk->kobj, KOBJ_ADD);
+			if (err)
+				printk(KERN_WARNING
+				       "%s (%d): sysfs_create_file fail for %s, err: %d\n",
+				       __FILE__, __LINE__,
+				       vattr->module_name, err);
+			else
+				kobject_uevent(&mk->kobj, KOBJ_ADD);
 			kobject_put(&mk->kobj);
 		}
 	}

Signature · 1 line

1.7.7.6

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Signature · 1 line

Chen Gang

Chen Gang

author

2013-07-10 02:36 UTC #3 lore raw

Quoted 7 lines · Chen Gang F T

Quoted 6 lines · Rusty Russell
Quoted 4 lines · Chen Gang
When sysfs_create_file() fails, recommend to print the related failure information. And it is useless to still 'KOBJ_ADD' to user space.
Signed-off-by Chen Gang <[email protected]>

Quoted 9 lines

Quoted 2 lines
sysfs_create_file() should not fail during boot, should it?
Hmm..., please reference locate_module_kobject() in "kernel/params.c", which is an '__init' function, and also call sysfs_create_file(), it processes the related error.
So I recommend to get the check too in version_sysfs_builtin().

Oh, also for locate_module_kobject(), if !CONFIG_MODULES, when error occurs, it still print the information about "Adding module".

Hmm..., do we need call kobject_get() before kobject_put() in failure processing block ?

740 mk = kzalloc(sizeof(struct module_kobject), GFP_KERNEL);
741 BUG_ON(!mk);
742

743                 mk->mod = THIS_MODULE;
744                 mk->kobj.kset = module_kset;
745                 err = kobject_init_and_add(&mk->kobj, &module_ktype, NULL,

746 "%s", name);
747 #ifdef CONFIG_MODULES
748 if (!err)
749 err = sysfs_create_file(&mk->kobj, &module_uevent.attr);
750 #endif
751 if (err) {
752 kobject_put(&mk->kobj);
753 pr_crit("Adding module '%s' to sysfs failed (%d), the system may be unstable.\n",
754 name, err);
755 return NULL;
756 }
757
758 /* So that we hold reference in both cases. */
759 kobject_get(&mk->kobj);
760 }
761
762 return mk;
763 }

Quoted 38 lines

Thanks.

Quoted 34 lines

Cheers,
Rusty.

Quoted 25 lines

---
 kernel/params.c |    8 +++++++-
 1 files changed, 7 insertions(+), 1 deletions(-)

kernel/params.c +7 -1

index 440e65d..f5299c1 100644
@@ -845,7 +845,13 @@ static void __init version_sysfs_builtin(void)
 		mk = locate_module_kobject(vattr->module_name);
 		if (mk) {
 			err = sysfs_create_file(&mk->kobj, &vattr->mattr.attr);
-			kobject_uevent(&mk->kobj, KOBJ_ADD);
+			if (err)
+				printk(KERN_WARNING
+				       "%s (%d): sysfs_create_file fail for %s, err: %d\n",
+				       __FILE__, __LINE__,
+				       vattr->module_name, err);
+			else
+				kobject_uevent(&mk->kobj, KOBJ_ADD);
 			kobject_put(&mk->kobj);
 		}
 	}

Signature · 1 line

1.7.7.6

Signature · 1 line

Chen Gang

Chen Gang F T

2013-07-10 03:02 UTC #4 lore raw

Quoted 18 lines · Chen Gang

Quoted 7 lines · Chen Gang F T
Quoted 6 lines · Rusty Russell
Quoted 4 lines · Chen Gang
When sysfs_create_file() fails, recommend to print the related failure information. And it is useless to still 'KOBJ_ADD' to user space.
Signed-off-by Chen Gang <[email protected]>
Quoted 9 lines
Quoted 2 lines
sysfs_create_file() should not fail during boot, should it?
Hmm..., please reference locate_module_kobject() in "kernel/params.c", which is an '__init' function, and also call sysfs_create_file(), it processes the related error.
So I recommend to get the check too in version_sysfs_builtin().

Quoted 8 lines

Oh, also for locate_module_kobject(), if !CONFIG_MODULES, when error occurs, it still print the information about "Adding module".
Hmm..., do we need call kobject_get() before kobject_put() in failure processing block ?

Oh, sorry for what I said for kobject_get/put() items above, it is incorrect.

What about the diff below for kobject_get() ?

-------------------------------diff begin-------------------------------

kernel/params.c +3 -3

index 440e65d..ef8d720 100644
@@ -754,11 +754,11 @@ static struct module_kobject * __init locate_module_kobject(const char *name)
 				name, err);
 			return NULL;
 		}
-
-		/* So that we hold reference in both cases. */
-		kobject_get(&mk->kobj);
 	}
 
+	/* So that we hold reference in both cases. */
+	kobject_get(&mk->kobj);
+
 	return mk;
 }

-------------------------------diff end---------------------------------

And it also need add additional kobject_put(), if we really need process the failure in version_sysfs_builtin().

Thanks.

Quoted 67 lines

740 mk = kzalloc(sizeof(struct module_kobject), GFP_KERNEL);
741 BUG_ON(!mk);
742
743                 mk->mod = THIS_MODULE;
744                 mk->kobj.kset = module_kset;
745                 err = kobject_init_and_add(&mk->kobj, &module_ktype, NULL,
746 "%s", name);
747 #ifdef CONFIG_MODULES
748 if (!err)
749 err = sysfs_create_file(&mk->kobj, &module_uevent.attr);
750 #endif
751 if (err) {
752 kobject_put(&mk->kobj);
753 pr_crit("Adding module '%s' to sysfs failed (%d), the system may be unstable.\n",
754 name, err);
755 return NULL;
756 }
757
758 /* So that we hold reference in both cases. */
759 kobject_get(&mk->kobj);
760 }
761
762 return mk;
763 }
Quoted 38 lines
Thanks.
Quoted 34 lines
Cheers,
Rusty.
Quoted 25 lines
---
 kernel/params.c |    8 +++++++-
 1 files changed, 7 insertions(+), 1 deletions(-)
kernel/params.c +7 -1
index 440e65d..f5299c1 100644
@@ -845,7 +845,13 @@ static void __init version_sysfs_builtin(void)
 		mk = locate_module_kobject(vattr->module_name);
 		if (mk) {
 			err = sysfs_create_file(&mk->kobj, &vattr->mattr.attr);
-			kobject_uevent(&mk->kobj, KOBJ_ADD);
+			if (err)
+				printk(KERN_WARNING
+				       "%s (%d): sysfs_create_file fail for %s, err: %d\n",
+				       __FILE__, __LINE__,
+				       vattr->module_name, err);
+			else
+				kobject_uevent(&mk->kobj, KOBJ_ADD);
 			kobject_put(&mk->kobj);
 		}
 	}
Signature · 1 line
1.7.7.6
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Signature · 1 line

Chen Gang

Chen Gang

author

2013-07-11 23:46 UTC #5 lore raw

Quoted 22 lines · Chen Gang F T

Quoted 4 lines
Quoted 4 lines
Hmm..., do we need call kobject_get() before kobject_put() in failure processing block ?
Oh, sorry for what I said for kobject_get/put() items above, it is incorrect.
What about the diff below for kobject_get() ?
-------------------------------diff begin-------------------------------
kernel/params.c +0 -2
index 440e65d..ef8d720 100644
@@ -754,11 +754,11 @@ static struct module_kobject * __init locate_module_kobject(const char *name)
 				name, err);
 			return NULL;
 		}
-
-		/* So that we hold reference in both cases. */

kernel/params.c static struct module_kobject * __init locate_module_kobject(const char *name)

Quoted 10 lines

kernel/params.c +3 -2

-		kobject_get(&mk->kobj);
 	}
 
+	/* So that we hold reference in both cases. */
+	kobject_get(&mk->kobj);
+
 	return mk;
 }
 
-------------------------------diff end---------------------------------

Sorry again, this diff is incorrect, the original implementation has no issues.

Quoted 2 lines

And it also need add additional kobject_put(), if we really need process the failure in version_sysfs_builtin().

If need process failure, we really need it, but now, we only use BUG_ON() is enough.

Thanks.

Signature · 1 line

Chen Gang

Rusty Russell

2013-07-11 02:38 UTC #6 lore raw

In reply to Chen Gang F T

Quoted 7 lines · Chen Gang F T

Quoted 6 lines · Rusty Russell
Quoted 4 lines · Chen Gang
When sysfs_create_file() fails, recommend to print the related failure information. And it is useless to still 'KOBJ_ADD' to user space.
Signed-off-by Chen Gang <[email protected]>

Quoted 8 lines

Quoted 2 lines
sysfs_create_file() should not fail during boot, should it?
Hmm..., please reference locate_module_kobject() in "kernel/params.c", which is an '__init' function, and also call sysfs_create_file(), it processes the related error.
So I recommend to get the check too in version_sysfs_builtin().

It still can't fail. sysfs_create_file() can fail due to OOM (not at boot) or name duplication (not here).

You can BUG_ON() if you want.

And feel free to fix locate_module_kobject() in a separate patch.

Cheers,
Rusty.

Chen Gang

author

2013-07-11 02:58 UTC #7 lore raw

Quoted 15 lines · Rusty Russell

Quoted 14 lines · Chen Gang F T
Quoted 14 lines · Rusty Russell
Quoted 8 lines
Quoted 8 lines
Quoted 4 lines · Chen Gang
Quoted 4 lines
When sysfs_create_file() fails, recommend to print the related failure information. And it is useless to still 'KOBJ_ADD' to user space.
Signed-off-by Chen Gang <[email protected]>
sysfs_create_file() should not fail during boot, should it?
Hmm..., please reference locate_module_kobject() in "kernel/params.c", which is an '__init' function, and also call sysfs_create_file(), it processes the related error.

Quoted 6 lines

Quoted 1 line
Quoted 1 line
So I recommend to get the check too in version_sysfs_builtin().
It still can't fail. sysfs_create_file() can fail due to OOM (not at boot) or name duplication (not here).
You can BUG_ON() if you want.

OK, thanks, I will send patch v2 for it.

Quoted 1 line

And feel free to fix locate_module_kobject() in a separate patch.

OK, thanks, I will send related patch for it.

:-)

Thanks.

Signature · 1 line

Chen Gang

Chen Gang

author

2013-07-11 04:07 UTC #8 lore raw

PATCH v2 1/1 kernel/params.c: add/modify failure processing code when sysfs_create_file() fails.

In reply to Rusty Russell

When sysfs_create_file() fails, need consider about it. And process it with BUG_ON(), because sysfs_create_file() can fail due to OOM (not at boot) or name duplication (not here).

Also correct the error printing information when failure occurs.

Signed-off-by Chen Gang <[email protected]>

---
 kernel/params.c |    7 +++++--
 1 files changed, 5 insertions(+), 2 deletions(-)

kernel/params.c +5 -2

index 440e65d..3d70f90 100644
@@ -745,12 +745,14 @@ static struct module_kobject * __init locate_module_kobject(const char *name)
 		err = kobject_init_and_add(&mk->kobj, &module_ktype, NULL,
 					   "%s", name);
 #ifdef CONFIG_MODULES
-		if (!err)
+		if (!err) {
 			err = sysfs_create_file(&mk->kobj, &module_uevent.attr);
+			BUG_ON(err);
+		}
 #endif
 		if (err) {
 			kobject_put(&mk->kobj);
-			pr_crit("Adding module '%s' to sysfs failed (%d), the system may be unstable.\n",
+			pr_crit("Initializing and adding module '%s' failed (%d), the system may be unstable.\n",
 				name, err);
 			return NULL;
 		}

@@ -845,6 +847,7 @@ static void __init version_sysfs_builtin(void)
 		mk = locate_module_kobject(vattr->module_name);
 		if (mk) {
 			err = sysfs_create_file(&mk->kobj, &vattr->mattr.attr);
+			BUG_ON(err);
 			kobject_uevent(&mk->kobj, KOBJ_ADD);
 			kobject_put(&mk->kobj);
 		}

Signature · 1 line

1.7.7.6

Chen Gang

author

2013-07-22 02:42 UTC #9 lore raw

Re: [PATCH v2] kernel/params.c: add/modify failure processing code when sysfs_create_file() fails.

Hello Maintainers:

Please help check this patch, when you have time.

Thanks.

Quoted 41 lines · Chen Gang

When sysfs_create_file() fails, need consider about it. And process it with BUG_ON(), because sysfs_create_file() can fail due to OOM (not at boot) or name duplication (not here).

Also correct the error printing information when failure occurs.

Signed-off-by Chen Gang <[email protected]>

---
 kernel/params.c |    7 +++++--
 1 files changed, 5 insertions(+), 2 deletions(-)

kernel/params.c +5 -2

index 440e65d..3d70f90 100644
@@ -745,12 +745,14 @@ static struct module_kobject * __init locate_module_kobject(const char *name)
 		err = kobject_init_and_add(&mk->kobj, &module_ktype, NULL,
 					   "%s", name);
 #ifdef CONFIG_MODULES
-		if (!err)
+		if (!err) {
 			err = sysfs_create_file(&mk->kobj, &module_uevent.attr);
+			BUG_ON(err);
+		}
 #endif
 		if (err) {
 			kobject_put(&mk->kobj);
-			pr_crit("Adding module '%s' to sysfs failed (%d), the system may be unstable.\n",
+			pr_crit("Initializing and adding module '%s' failed (%d), the system may be unstable.\n",
 				name, err);
 			return NULL;
 		}

@@ -845,6 +847,7 @@ static void __init version_sysfs_builtin(void)
 		mk = locate_module_kobject(vattr->module_name);
 		if (mk) {
 			err = sysfs_create_file(&mk->kobj, &vattr->mattr.attr);
+			BUG_ON(err);
 			kobject_uevent(&mk->kobj, KOBJ_ADD);
 			kobject_put(&mk->kobj);
 		}

Signature · 1 line

Chen Gang