[PATCH resent v2 2/2] powerpc/pseries: Fix exception handling in pSeries_reconfig_add_node()
Date: Tue, 21 Mar 2023 10:50:08 +0100
The label “out_err” was used to jump to another pointer check despite of
the detail in the implementation of the function “pSeries_reconfig_add_node”
that it was determined already that the corresponding variable contained
a null pointer (because of a failed function call in two cases).
1. Thus return directly after a call of the function “kzalloc” failed.
2. Use more appropriate labels instead.
3. Delete a redundant check.
4. Omit an explicit initialisation for the local variable “err”.
This issue was detected by using the Coccinelle software.
Fixes: 1da177e4c3f41524e886b7f1b8a0c1fc7321cac2 ("Linux-2.6.12-rc2")
Signed-off-by: Markus Elfring
---
V2:
This update step was based on a previous change.
arch/powerpc/platforms/pseries/reconfig.c | 23 ---
1 file changed, 12 insertions(+), 11 deletions(-)
diff --git a/arch/powerpc/platforms/pseries/reconfig.c
b/arch/powerpc/platforms/pseries/reconfig.c
index 44f8ebc2ec0d..14154f48ef63 100644
--- a/arch/powerpc/platforms/pseries/reconfig.c
+++ b/arch/powerpc/platforms/pseries/reconfig.c
@@ -23,15 +23,17 @@
static int pSeries_reconfig_add_node(const char *path, struct property
*proplist)
{
struct device_node *np;
- int err = -ENOMEM;
+ int err;
np = kzalloc(sizeof(*np), GFP_KERNEL);
if (!np)
- goto out_err;
+ return -ENOMEM;
np->full_name = kstrdup(kbasename(path), GFP_KERNEL);
- if (!np->full_name)
- goto out_err;
+ if (!np->full_name) {
+ err = -ENOMEM;
+ goto free_device_node;
+ }
np->properties = proplist;
of_node_set_flag(np, OF_DYNAMIC);
@@ -46,20 +48,19 @@ static int pSeries_reconfig_add_node(const char *path,
struct property *proplist
err = of_attach_node(np);
if (err) {
printk(KERN_ERR "Failed to add device node %s\n", path);
- goto out_err;
+ goto put_node;
}
of_node_put(np->parent);
return 0;
-out_err:
- if (np) {
- of_node_put(np->parent);
+put_node:
+ of_node_put(np->parent);
free_name:
- kfree(np->full_name);
- kfree(np);
- }
+ kfree(np->full_name);
+free_device_node:
+ kfree(np);
return err;
}
--
2.40.0
[PATCH v2 2/2] powerpc/pseries: Fix exception handling in pSeries_reconfig_add_node()
Date: Tue, 21 Mar 2023 10:50:08 +0100
The label “out_err” was used to jump to another pointer check despite of
the detail in the implementation of the function “pSeries_reconfig_add_node”
that it was determined already that the corresponding variable contained
a null pointer (because of a failed function call in two cases).
1. Thus return directly after a call of the function “kzalloc” failed.
2. Use more appropriate labels instead.
3. Delete a redundant check.
4. Omit an explicit initialisation for the local variable “err”.
This issue was detected by using the Coccinelle software.
Fixes: 1da177e4c3f41524e886b7f1b8a0c1fc7321cac2 ("Linux-2.6.12-rc2")
Signed-off-by: Markus Elfring
---
V2:
This update step was based on a previous change.
arch/powerpc/platforms/pseries/reconfig.c | 23 ---
1 file changed, 12 insertions(+), 11 deletions(-)
diff --git a/arch/powerpc/platforms/pseries/reconfig.c
b/arch/powerpc/platforms/pseries/reconfig.c
index 44f8ebc2ec0d..14154f48ef63 100644
--- a/arch/powerpc/platforms/pseries/reconfig.c
+++ b/arch/powerpc/platforms/pseries/reconfig.c
@@ -23,15 +23,17 @@
static int pSeries_reconfig_add_node(const char *path, struct property
*proplist)
{
struct device_node *np;
- int err = -ENOMEM;
+ int err;
np = kzalloc(sizeof(*np), GFP_KERNEL);
if (!np)
- goto out_err;
+ return -ENOMEM;
np->full_name = kstrdup(kbasename(path), GFP_KERNEL);
- if (!np->full_name)
- goto out_err;
+ if (!np->full_name) {
+ err = -ENOMEM;
+ goto free_device_node;
+ }
np->properties = proplist;
of_node_set_flag(np, OF_DYNAMIC);
@@ -46,20 +48,19 @@ static int pSeries_reconfig_add_node(const char *path,
struct property *proplist
err = of_attach_node(np);
if (err) {
printk(KERN_ERR "Failed to add device node %s\n", path);
- goto out_err;
+ goto put_node;
}
of_node_put(np->parent);
return 0;
-out_err:
- if (np) {
- of_node_put(np->parent);
+put_node:
+ of_node_put(np->parent);
free_name:
- kfree(np->full_name);
- kfree(np);
- }
+ kfree(np->full_name);
+free_device_node:
+ kfree(np);
return err;
}
--
2.40.0
Re: powerpc/pseries: Fix exception handling in pSeries_reconfig_add_node()
> It's been brought to my attention that there is in fact a crash bug > in this function's error path: How do you think about to mention any other contributors for attribution according to this issue? > np->parent can be an encoded error value, we don't want to of_node_put() that. Will the development attention grow for any more cases? > I believe the patch as written happens to fix the issue. Is it interesting how many details can still be improved (by my change suggestion) also for the discussed function implementation? > Will you please write it up as a bug fix and resubmit? Another proposal will follow. Regards, Markus
Re: powerpc/pseries: Fix exception handling in pSeries_reconfig_add_node()
Markus Elfring writes:
> The label “out_err” was used to jump to another pointer check despite of
> the detail in the implementation of the function
> “pSeries_reconfig_add_node”
> that it was determined already that the corresponding variable contained
> a null pointer (because of a failed function call in two cases).
>
> 1. Thus return directly after a call of the function “kzalloc” failed.
>
> 2. Use more appropriate labels instead.
>
> 3. Delete a redundant check.
>
> 4. Omit an explicit initialisation for the local variable “err”.
>
> This issue was detected by using the Coccinelle software.
Is there a correctness or safety issue here?
>>> I got the impression that the application of only a single label like
>>> “out_err”
>>> resulted in improvable implementation details.
>> I don't understand what you're trying to say here.
>
> What does hinder you to understand the presented change description better
> at the moment?
>
>
>> It doesn't seem to answer my question.
>
>
> I hope that my answer will trigger further helpful considerations.
I don't consider this response constructive, but I want to get this back
on track. It's been brought to my attention that there is in fact a
crash bug in this function's error path:
np->parent = pseries_of_derive_parent(path);
if (IS_ERR(np->parent)) {
err = PTR_ERR(np->parent);
goto out_err;
}
...
out_err:
if (np) {
of_node_put(np->parent);
np->parent can be an encoded error value, we don't want to of_node_put()
that.
I believe the patch as written happens to fix the issue. Will you please
write it up as a bug fix and resubmit?
Re: powerpc/pseries: Fix exception handling in pSeries_reconfig_add_node()
The label “out_err” was used to jump to another pointer check despite of the detail in the implementation of the function “pSeries_reconfig_add_node” that it was determined already that the corresponding variable contained a null pointer (because of a failed function call in two cases). 1. Thus return directly after a call of the function “kzalloc” failed. 2. Use more appropriate labels instead. 3. Delete a redundant check. 4. Omit an explicit initialisation for the local variable “err”. This issue was detected by using the Coccinelle software. >>> Is there a correctness or safety issue here? >> I got the impression that the application of only a single label like >> “out_err” >> resulted in improvable implementation details. > I don't understand what you're trying to say here. What does hinder you to understand the presented change description better at the moment? > It doesn't seem to answer my question. I hope that my answer will trigger further helpful considerations. >>> The subject uses the word "fix" but the commit message doesn't seem to >>> identify one. >> Can you find the proposed adjustments reasonable? > In the absence of a bug fix or an improvement in readability, not really, > sorry. The views are varying for “programming bugs”, aren't they? > It adds to the function more goto labels and another return, This is the suggested source code transformation. > apparently to avoid checks Can the support grow for such a programming goal? > that are sometimes redundant Can such implementation details become undesirable? > (but not incorrect) at the C source code level. Will this aspect affect further development concerns? >> Please take another look at available information sources. >> https://lore.kernel.org/cocci/f9303bdc-b1a7-be5e-56c6-dfa8232b8...@web.de/ > I wasn't cc'd on this and I'm not subscribed to any lists in the recipients > for that message, so not sure how I would take "another" look. :-) I imagine that you can benefit more from information which can be retrieved by archive interfaces also according to the mailing list of the Coccinelle software. See also: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/dev-tools/coccinelle.rst?h=v6.3-rc2#n9 Regards, Markus
Re: powerpc/pseries: Fix exception handling in pSeries_reconfig_add_node()
Markus Elfring writes: >>> The label “out_err” was used to jump to another pointer check despite of >>> the detail in the implementation of the function “pSeries_reconfig_add_node” >>> that it was determined already that the corresponding variable contained >>> a null pointer (because of a failed function call in two cases). >>> >>> 1. Thus return directly after a call of the function “kzalloc” failed. >>> >>> 2. Use more appropriate labels instead. >>> >>> 3. Delete a redundant check. >>> >>> 4. Omit an explicit initialisation for the local variable “err”. >>> >>> This issue was detected by using the Coccinelle software. >> Is there a correctness or safety issue here? > > I got the impression that the application of only a single label like > “out_err” > resulted in improvable implementation details. I don't understand what you're trying to say here. It doesn't seem to answer my question. >> The subject uses the word "fix" but the commit message doesn't seem to >> identify one. > > Can you find the proposed adjustments reasonable? In the absence of a bug fix or an improvement in readability, not really, sorry. It adds to the function more goto labels and another return, apparently to avoid checks that are sometimes redundant (but not incorrect) at the C source code level. An optimizing compiler doesn't necessarily arrange the generated code in the same way. >> Can you share how Coccinelle is being invoked and its output? > > Please take another look at available information sources. > https://lore.kernel.org/cocci/f9303bdc-b1a7-be5e-56c6-dfa8232b8...@web.de/ I wasn't cc'd on this and I'm not subscribed to any lists in the recipients for that message, so not sure how I would take "another" look. :-)
Re: powerpc/pseries: Fix exception handling in pSeries_reconfig_add_node()
>> The label “out_err” was used to jump to another pointer check despite of >> the detail in the implementation of the function “pSeries_reconfig_add_node” >> that it was determined already that the corresponding variable contained >> a null pointer (because of a failed function call in two cases). >> >> 1. Thus return directly after a call of the function “kzalloc” failed. >> >> 2. Use more appropriate labels instead. >> >> 3. Delete a redundant check. >> >> 4. Omit an explicit initialisation for the local variable “err”. >> >> This issue was detected by using the Coccinelle software. > Is there a correctness or safety issue here? I got the impression that the application of only a single label like “out_err” resulted in improvable implementation details. > The subject uses the word "fix" but the commit message doesn't seem to > identify one. Can you find the proposed adjustments reasonable? > Can you share how Coccinelle is being invoked and its output? Please take another look at available information sources. https://lore.kernel.org/cocci/f9303bdc-b1a7-be5e-56c6-dfa8232b8...@web.de/ https://sympa.inria.fr/sympa/arc/cocci/2023-03/msg00017.html Another command example: Markus_Elfring@Sonne:…/Projekte/Linux/next-patched> spatch --timeout 23 -j4 --chunksize 1 -dir . …/Projekte/Coccinelle/janitor/show_jumps_to_pointer_check.cocci > ../show_jumps_to_pointer_check-next-20230315.diff 2> ../show_jumps_to_pointer_check-errors-next-20230315.txt Regards, Markus
Re: [PATCH] powerpc/pseries: Fix exception handling in pSeries_reconfig_add_node()
Markus Elfring writes: > > The label “out_err” was used to jump to another pointer check despite of > the detail in the implementation of the function “pSeries_reconfig_add_node” > that it was determined already that the corresponding variable contained > a null pointer (because of a failed function call in two cases). > > 1. Thus return directly after a call of the function “kzalloc” failed. > > 2. Use more appropriate labels instead. > > 3. Delete a redundant check. > > 4. Omit an explicit initialisation for the local variable “err”. > > This issue was detected by using the Coccinelle software. Is there a correctness or safety issue here? The subject uses the word "fix" but the commit message doesn't seem to identify one. Can you share how Coccinelle is being invoked and its output?
[PATCH] powerpc/pseries: Fix exception handling in pSeries_reconfig_add_node()
Date: Fri, 17 Mar 2023 09:26:13 +0100
The label “out_err” was used to jump to another pointer check despite of
the detail in the implementation of the function “pSeries_reconfig_add_node”
that it was determined already that the corresponding variable contained
a null pointer (because of a failed function call in two cases).
1. Thus return directly after a call of the function “kzalloc” failed.
2. Use more appropriate labels instead.
3. Delete a redundant check.
4. Omit an explicit initialisation for the local variable “err”.
This issue was detected by using the Coccinelle software.
Fixes: 1da177e4c3f41524e886b7f1b8a0c1fc7321cac ("Linux-2.6.12-rc2")
Signed-off-by: Markus Elfring
---
arch/powerpc/platforms/pseries/reconfig.c | 26 ---
1 file changed, 14 insertions(+), 12 deletions(-)
diff --git a/arch/powerpc/platforms/pseries/reconfig.c
b/arch/powerpc/platforms/pseries/reconfig.c
index 599bd2c78514..14154f48ef63 100644
--- a/arch/powerpc/platforms/pseries/reconfig.c
+++ b/arch/powerpc/platforms/pseries/reconfig.c
@@ -23,15 +23,17 @@
static int pSeries_reconfig_add_node(const char *path, struct property
*proplist)
{
struct device_node *np;
- int err = -ENOMEM;
+ int err;
np = kzalloc(sizeof(*np), GFP_KERNEL);
if (!np)
- goto out_err;
+ return -ENOMEM;
np->full_name = kstrdup(kbasename(path), GFP_KERNEL);
- if (!np->full_name)
- goto out_err;
+ if (!np->full_name) {
+ err = -ENOMEM;
+ goto free_device_node;
+ }
np->properties = proplist;
of_node_set_flag(np, OF_DYNAMIC);
@@ -40,25 +42,25 @@ static int pSeries_reconfig_add_node(const char *path,
struct property *proplist
np->parent = pseries_of_derive_parent(path);
if (IS_ERR(np->parent)) {
err = PTR_ERR(np->parent);
- goto out_err;
+ goto free_name;
}
err = of_attach_node(np);
if (err) {
printk(KERN_ERR "Failed to add device node %s\n", path);
- goto out_err;
+ goto put_node;
}
of_node_put(np->parent);
return 0;
-out_err:
- if (np) {
- of_node_put(np->parent);
- kfree(np->full_name);
- kfree(np);
- }
+put_node:
+ of_node_put(np->parent);
+free_name:
+ kfree(np->full_name);
+free_device_node:
+ kfree(np);
return err;
}
--
2.40.0
