[PATCH 0/4] powerpc/4xx: Adjustments for four function implementations

[PATCH 0/4] powerpc/4xx: Adjustments for four function implementations

[Markus Elfring]

Date: Thu, 16 Mar 2023 20:15:43 +0100

Some update suggestions were taken into account
from static source code analysis.

Markus Elfring (4):
  Fix exception handling in ppc4xx_pciex_port_setup_hose()
  Fix exception handling in ppc4xx_probe_pcix_bridge()
  Fix exception handling in ppc4xx_probe_pci_bridge()
  Delete unnecessary variable initialisations in four functions

 arch/powerpc/platforms/4xx/pci.c | 69 ++++++++++++++------------------
 1 file changed, 31 insertions(+), 38 deletions(-)

--
2.39.2

[PATCH 1/4] powerpc/4xx: Fix exception handling in ppc4xx_pciex_port_setup_hose()

[Markus Elfring]

Date: Thu, 16 Mar 2023 19:00:57 +0100

The label “fail” was used to jump to another pointer check despite of
the detail in the implementation of the function “ppc4xx_pciex_port_setup_hose”
that it was determined already that the corresponding variable contained
a null pointer (because of a failed function call in three cases).

1. Thus return directly after a call of the function “pcibios_alloc_controller” failed.

2. Use more appropriate labels instead.

3. Reorder jump targets at the end.

4. Delete two questionable checks.

This issue was detected by using the Coccinelle software.

Fixes: a2d2e1ec07a80946cbe812dc8c73291cad8214b2 ("[POWERPC] 4xx: PLB to PCI Express support")
Fixes: 80daac3f86d4f5aafc9d3e79addb90fa118244e2 ("[POWERPC] 4xx: Add endpoint support to 4xx PCIe driver")
Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 arch/powerpc/platforms/4xx/pci.c | 19 ++++++++++---------
 1 file changed, 10 insertions(+), 9 deletions(-)

diff --git a/arch/powerpc/platforms/4xx/pci.c b/arch/powerpc/platforms/4xx/pci.c
index ca5dd7a5842a..7336c7039b10 100644
--- a/arch/powerpc/platforms/4xx/pci.c
+++ b/arch/powerpc/platforms/4xx/pci.c
@@ -1930,7 +1930,7 @@ static void __init ppc4xx_pciex_port_setup_hose(struct ppc4xx_pciex_port *port)
     /* Allocate the host controller data structure */
     hose = pcibios_alloc_controller(port->node);
     if (!hose)
-        goto fail;
+        return;
 
     /* We stick the port number in "indirect_type" so the config space
      * ops can retrieve the port data structure easily
@@ -1962,7 +1962,7 @@ static void __init ppc4xx_pciex_port_setup_hose(struct ppc4xx_pciex_port *port)
         if (cfg_data == NULL) {
             printk(KERN_ERR "%pOF: Can't map external config space !",
                    port->node);
-            goto fail;
+            goto free_controller;
         }
         hose->cfg_data = cfg_data;
     }
@@ -1974,7 +1974,7 @@ static void __init ppc4xx_pciex_port_setup_hose(struct ppc4xx_pciex_port *port)
     if (mbase == NULL) {
         printk(KERN_ERR "%pOF: Can't map internal config space !",
                port->node);
-        goto fail;
+        goto recheck_cfg_data;
     }
     hose->cfg_addr = mbase;
 
@@ -2007,7 +2007,7 @@ static void __init ppc4xx_pciex_port_setup_hose(struct ppc4xx_pciex_port *port)
 
     /* Parse inbound mapping resources */
     if (ppc4xx_parse_dma_ranges(hose, mbase, &dma_window) != 0)
-        goto fail;
+        goto unmap_io_mbase;
 
     /* Configure outbound ranges POMs */
     ppc4xx_configure_pciex_POMs(port, hose, mbase);
@@ -2064,13 +2064,14 @@ static void __init ppc4xx_pciex_port_setup_hose(struct ppc4xx_pciex_port *port)
     }
 
     return;
- fail:
-    if (hose)
-        pcibios_free_controller(hose);
+
+unmap_io_mbase:
+    iounmap(mbase);
+recheck_cfg_data:
     if (cfg_data)
         iounmap(cfg_data);
-    if (mbase)
-        iounmap(mbase);
+free_controller:
+    pcibios_free_controller(hose);
 }
 
 static void __init ppc4xx_probe_pciex_bridge(struct device_node *np)
--
2.39.2

[PATCH 2/4] powerpc/4xx: Fix exception handling in ppc4xx_probe_pcix_bridge()

[Markus Elfring]

Date: Thu, 16 Mar 2023 19:09:33 +0100

The label “fail” was used to jump to another pointer check despite of
the detail in the implementation of the function “ppc4xx_probe_pcix_bridge”
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 “ioremap” failed.

2. Use a more appropriate label instead.

3. Delete two questionable checks.

4. Adjust the exception handling for another if branch a bit more.

5. Remove a return statement at the end.

This issue was detected by using the Coccinelle software.

Fixes: 5738ec6d00b7abbcd4cd342af83fd18d192b0979 ("[POWERPC] 4xx: PLB to PCI-X support")
Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 arch/powerpc/platforms/4xx/pci.c | 18 +++++++-----------
 1 file changed, 7 insertions(+), 11 deletions(-)

diff --git a/arch/powerpc/platforms/4xx/pci.c b/arch/powerpc/platforms/4xx/pci.c
index 7336c7039b10..fdf13e12ca9d 100644
--- a/arch/powerpc/platforms/4xx/pci.c
+++ b/arch/powerpc/platforms/4xx/pci.c
@@ -564,13 +564,13 @@ static void __init ppc4xx_probe_pcix_bridge(struct device_node *np)
     reg = ioremap(rsrc_reg.start, resource_size(&rsrc_reg));
     if (reg == NULL) {
         printk(KERN_ERR "%pOF: Can't map registers !", np);
-        goto fail;
+        return;
     }
 
     /* Allocate the host controller data structure */
     hose = pcibios_alloc_controller(np);
     if (!hose)
-        goto fail;
+        goto unmap_io;
 
     hose->first_busno = bus_range ? bus_range[0] : 0x0;
     hose->last_busno = bus_range ? bus_range[1] : 0xff;
@@ -595,8 +595,10 @@ static void __init ppc4xx_probe_pcix_bridge(struct device_node *np)
     pci_process_bridge_OF_ranges(hose, np, primary);
 
     /* Parse inbound mapping resources */
-    if (ppc4xx_parse_dma_ranges(hose, reg, &dma_window) != 0)
-        goto fail;
+    if (ppc4xx_parse_dma_ranges(hose, reg, &dma_window)) {
+        pcibios_free_controller(hose);
+        goto unmap_io;
+    }
 
     /* Configure outbound ranges POMs */
     ppc4xx_configure_pcix_POMs(hose, reg);
@@ -605,14 +607,8 @@ static void __init ppc4xx_probe_pcix_bridge(struct device_node *np)
     ppc4xx_configure_pcix_PIMs(hose, reg, &dma_window, big_pim, msi);
 
     /* We don't need the registers anymore */
+unmap_io:
     iounmap(reg);
-    return;
-
- fail:
-    if (hose)
-        pcibios_free_controller(hose);
-    if (reg)
-        iounmap(reg);
 }
 
 #ifdef CONFIG_PPC4xx_PCI_EXPRESS
--
2.39.2

[PATCH 3/4] powerpc/4xx: Fix exception handling in ppc4xx_probe_pci_bridge()

[Markus Elfring]

Date: Thu, 16 Mar 2023 19:12:10 +0100

The label “fail” was used to jump to another pointer check despite of
the detail in the implementation of the function “ppc4xx_probe_pci_bridge”
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 “ioremap” failed.

2. Use a more appropriate label instead.

3. Delete two questionable checks.

4. Adjust the exception handling for another if branch a bit more.

5. Remove a return statement at the end.

This issue was detected by using the Coccinelle software.

Fixes: c839e0eff500af03de65e560c2e21c3831586e6e ("[POWERPC] 4xx: PLB to PCI 2.x support")
Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 arch/powerpc/platforms/4xx/pci.c | 18 +++++++-----------
 1 file changed, 7 insertions(+), 11 deletions(-)

diff --git a/arch/powerpc/platforms/4xx/pci.c b/arch/powerpc/platforms/4xx/pci.c
index fdf13e12ca9d..0c0848f0c819 100644
--- a/arch/powerpc/platforms/4xx/pci.c
+++ b/arch/powerpc/platforms/4xx/pci.c
@@ -358,13 +358,13 @@ static void __init ppc4xx_probe_pci_bridge(struct device_node *np)
     reg = ioremap(rsrc_reg.start, resource_size(&rsrc_reg));
     if (reg == NULL) {
         printk(KERN_ERR "%pOF: Can't map registers !", np);
-        goto fail;
+        return;
     }
 
     /* Allocate the host controller data structure */
     hose = pcibios_alloc_controller(np);
     if (!hose)
-        goto fail;
+        goto unmap_io;
 
     hose->first_busno = bus_range ? bus_range[0] : 0x0;
     hose->last_busno = bus_range ? bus_range[1] : 0xff;
@@ -383,8 +383,10 @@ static void __init ppc4xx_probe_pci_bridge(struct device_node *np)
     pci_process_bridge_OF_ranges(hose, np, primary);
 
     /* Parse inbound mapping resources */
-    if (ppc4xx_parse_dma_ranges(hose, reg, &dma_window) != 0)
-        goto fail;
+    if (ppc4xx_parse_dma_ranges(hose, reg, &dma_window) {
+        pcibios_free_controller(hose);
+        goto unmap_io;
+    }
 
     /* Configure outbound ranges POMs */
     ppc4xx_configure_pci_PMMs(hose, reg);
@@ -393,14 +395,8 @@ static void __init ppc4xx_probe_pci_bridge(struct device_node *np)
     ppc4xx_configure_pci_PTMs(hose, reg, &dma_window);
 
     /* We don't need the registers anymore */
+unmap_io:
     iounmap(reg);
-    return;
-
- fail:
-    if (hose)
-        pcibios_free_controller(hose);
-    if (reg)
-        iounmap(reg);
 }
 
 /*
--
2.39.2

[PATCH 4/4] powerpc/4xx: Delete unnecessary variable initialisations in four functions

[Markus Elfring]

Date: Thu, 16 Mar 2023 19:56:21 +0100

Some local variables will be set to an appropriate value before usage.
Thus omit explicit initialisations at the beginning of these functions.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 arch/powerpc/platforms/4xx/pci.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/arch/powerpc/platforms/4xx/pci.c b/arch/powerpc/platforms/4xx/pci.c
index 0c0848f0c819..ebc030e12663 100644
--- a/arch/powerpc/platforms/4xx/pci.c
+++ b/arch/powerpc/platforms/4xx/pci.c
@@ -323,8 +323,8 @@ static void __init ppc4xx_probe_pci_bridge(struct device_node *np)
     struct resource rsrc_cfg;
     struct resource rsrc_reg;
     struct resource dma_window;
-    struct pci_controller *hose = NULL;
-    void __iomem *reg = NULL;
+    struct pci_controller *hose;
+    void __iomem *reg;
     const int *bus_range;
     int primary = 0;
 
@@ -523,8 +523,8 @@ static void __init ppc4xx_probe_pcix_bridge(struct device_node *np)
     struct resource rsrc_cfg;
     struct resource rsrc_reg;
     struct resource dma_window;
-    struct pci_controller *hose = NULL;
-    void __iomem *reg = NULL;
+    struct pci_controller *hose;
+    void __iomem *reg;
     const int *bus_range;
     int big_pim = 0, msi = 0, primary = 0;
 
@@ -1403,7 +1403,7 @@ static struct ppc4xx_pciex_hwops ppc_476fpe_pcie_hwops __initdata =
 static int __init ppc4xx_pciex_check_core_init(struct device_node *np)
 {
     static int core_init;
-    int count = -ENODEV;
+    int count;
 
     if (core_init++)
         return 0;
@@ -1905,10 +1905,10 @@ static void __init ppc4xx_configure_pciex_PIMs(struct ppc4xx_pciex_port *port,
 static void __init ppc4xx_pciex_port_setup_hose(struct ppc4xx_pciex_port *port)
 {
     struct resource dma_window;
-    struct pci_controller *hose = NULL;
+    struct pci_controller *hose;
     const int *bus_range;
     int primary = 0, busses;
-    void __iomem *mbase = NULL, *cfg_data = NULL;
+    void __iomem *mbase, *cfg_data = NULL;
     const u32 *pval;
     u32 val;
 
--
2.39.2

[PATCH] powerpc/pseries: Fix exception handling in pSeries_reconfig_add_node()

[Markus Elfring]

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 <elfring@users.sourceforge.net>
---
 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

Re: [PATCH] powerpc/pseries: Fix exception handling in pSeries_reconfig_add_node()

[Nathan Lynch]

Markus Elfring <Markus.Elfring@web.de> 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?

Re: powerpc/pseries: Fix exception handling in pSeries_reconfig_add_node()

[Markus Elfring]

>> 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-dfa8232b8b55@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: powerpc/pseries: Fix exception handling in pSeries_reconfig_add_node()

[Nathan Lynch]

Markus Elfring <Markus.Elfring@web.de> 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-dfa8232b8b55@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()

[Markus Elfring]

>>>> 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-dfa8232b8b55@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()

[Nathan Lynch]

Markus Elfring <Markus.Elfring@web.de> 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()

[Markus Elfring]

> 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

[PATCH v2 0/2] powerpc/pseries: Fixes for exception handling in pSeries_reconfig_add_node()

[Markus Elfring]

Date: Tue, 21 Mar 2023 11:26:32 +0100

A few update suggestions were taken into account
from static source code analysis.

Markus Elfring (2):
  Do not pass an error pointer to of_node_put()
  Fix exception handling

 arch/powerpc/platforms/pseries/reconfig.c | 26 ++++++++++++-----------
 1 file changed, 14 insertions(+), 12 deletions(-)

--
2.40.0

[PATCH v2 1/2] powerpc/pseries: Do not pass an error pointer to of_node_put() in pSeries_reconfig_add_node()

[Markus Elfring]

Date: Tue, 21 Mar 2023 10:30:23 +0100

It can be determined in the implementation of the function
“pSeries_reconfig_add_node” that an error code would occasionally
be provided by a call of a function like pseries_of_derive_parent().
This error indication was passed to an of_node_put() call according to
an attempt for exception handling so far.

Thus fix the risk for undesirable software behaviour by using
an additional label for this error case.

Link: https://lists.ozlabs.org/pipermail/linuxppc-dev/2023-March/256025.html
Link: https://lore.kernel.org/lkml/87pm9377qt.fsf@linux.ibm.com/
Reported-by: Nathan Lynch <nathanl@linux.ibm.com>
Fixes: 1da177e4c3f41524e886b7f1b8a0c1fc7321cac2 ("Linux-2.6.12-rc2")
Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
V2:
This update step was added according to another change request.

 arch/powerpc/platforms/pseries/reconfig.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/powerpc/platforms/pseries/reconfig.c b/arch/powerpc/platforms/pseries/reconfig.c
index 599bd2c78514..44f8ebc2ec0d 100644
--- a/arch/powerpc/platforms/pseries/reconfig.c
+++ b/arch/powerpc/platforms/pseries/reconfig.c
@@ -40,7 +40,7 @@ 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);
@@ -56,6 +56,7 @@ static int pSeries_reconfig_add_node(const char *path, struct property *proplist
 out_err:
     if (np) {
         of_node_put(np->parent);
+free_name:
         kfree(np->full_name);
         kfree(np);
     }
--
2.40.0

[PATCH v2 2/2] powerpc/pseries: Fix exception handling in pSeries_reconfig_add_node()

[Markus Elfring]

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 <elfring@users.sourceforge.net>
---
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 resent v2 0/2] powerpc/pseries: Fixes for exception handling in pSeries_reconfig_add_node()

[Markus Elfring]

Date: Tue, 21 Mar 2023 11:26:32 +0100

A few update suggestions were taken into account
from static source code analysis.

Markus Elfring (2):
  Do not pass an error pointer to of_node_put()
  Fix exception handling

 arch/powerpc/platforms/pseries/reconfig.c | 26 ++++++++++++-----------
 1 file changed, 14 insertions(+), 12 deletions(-)

--
2.40.0

[PATCH resent v2 1/2] powerpc/pseries: Do not pass an error pointer to of_node_put() in pSeries_reconfig_add_node()

[Markus Elfring]

Date: Tue, 21 Mar 2023 10:30:23 +0100

It can be determined in the implementation of the function
“pSeries_reconfig_add_node” that an error code would occasionally
be provided by a call of a function like pseries_of_derive_parent().
This error indication was passed to an of_node_put() call according to
an attempt for exception handling so far.

Thus fix the risk for undesirable software behaviour by using
an additional label for this error case.

Link: https://lists.ozlabs.org/pipermail/linuxppc-dev/2023-March/256025.html
Link: https://lore.kernel.org/lkml/87pm9377qt.fsf@linux.ibm.com/
Reported-by: Nathan Lynch <nathanl@linux.ibm.com>
Fixes: 1da177e4c3f41524e886b7f1b8a0c1fc7321cac2 ("Linux-2.6.12-rc2")
Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
V2:
This update step was added according to another change request.

 arch/powerpc/platforms/pseries/reconfig.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/powerpc/platforms/pseries/reconfig.c b/arch/powerpc/platforms/pseries/reconfig.c
index 599bd2c78514..44f8ebc2ec0d 100644
--- a/arch/powerpc/platforms/pseries/reconfig.c
+++ b/arch/powerpc/platforms/pseries/reconfig.c
@@ -40,7 +40,7 @@ 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);
@@ -56,6 +56,7 @@ static int pSeries_reconfig_add_node(const char *path, struct property *proplist
 out_err:
 	if (np) {
 		of_node_put(np->parent);
+free_name:
 		kfree(np->full_name);
 		kfree(np);
 	}
--
2.40.0

[PATCH resent v2 2/2] powerpc/pseries: Fix exception handling in pSeries_reconfig_add_node()

[Markus Elfring]

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 <elfring@users.sourceforge.net>
---
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 0/4] powerpc/4xx: Adjustments for four function implementations

[Markus Elfring]

Date: Sat, 25 Mar 2023 16:10:23 +0100

Some update suggestions were taken into account
from static source code analysis.

Markus Elfring (4):
  Fix exception handling in ppc4xx_pciex_port_setup_hose()
  Fix exception handling in ppc4xx_probe_pcix_bridge()
  Fix exception handling in ppc4xx_probe_pci_bridge()
  Delete unnecessary variable initialisations in four functions

 arch/powerpc/platforms/4xx/pci.c | 69 ++++++++++++++------------------
 1 file changed, 31 insertions(+), 38 deletions(-)

--
2.40.0

[PATCH v2 1/4] powerpc/4xx: Fix exception handling in ppc4xx_pciex_port_setup_hose()

[Markus Elfring]

Date: Thu, 16 Mar 2023 19:00:57 +0100

The label “fail” was used to jump to another pointer check despite of
the detail in the implementation of the function “ppc4xx_pciex_port_setup_hose”
that it was determined already that the corresponding variable contained
a null pointer (because of a failed function call in three cases).

1. Thus return directly after a call of the function “pcibios_alloc_controller” failed.

2. Use more appropriate labels instead.

3. Reorder jump targets at the end.

4. Delete two questionable checks.


This issue was detected by using the Coccinelle software.

Fixes: a2d2e1ec07a80946cbe812dc8c73291cad8214b2 ("[POWERPC] 4xx: PLB to PCI Express support")
Fixes: 80daac3f86d4f5aafc9d3e79addb90fa118244e2 ("[POWERPC] 4xx: Add endpoint support to 4xx PCIe driver")
Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 arch/powerpc/platforms/4xx/pci.c | 19 ++++++++++---------
 1 file changed, 10 insertions(+), 9 deletions(-)

diff --git a/arch/powerpc/platforms/4xx/pci.c b/arch/powerpc/platforms/4xx/pci.c
index ca5dd7a5842a..7336c7039b10 100644
--- a/arch/powerpc/platforms/4xx/pci.c
+++ b/arch/powerpc/platforms/4xx/pci.c
@@ -1930,7 +1930,7 @@ static void __init ppc4xx_pciex_port_setup_hose(struct ppc4xx_pciex_port *port)
 	/* Allocate the host controller data structure */
 	hose = pcibios_alloc_controller(port->node);
 	if (!hose)
-		goto fail;
+		return;

 	/* We stick the port number in "indirect_type" so the config space
 	 * ops can retrieve the port data structure easily
@@ -1962,7 +1962,7 @@ static void __init ppc4xx_pciex_port_setup_hose(struct ppc4xx_pciex_port *port)
 		if (cfg_data == NULL) {
 			printk(KERN_ERR "%pOF: Can't map external config space !",
 			       port->node);
-			goto fail;
+			goto free_controller;
 		}
 		hose->cfg_data = cfg_data;
 	}
@@ -1974,7 +1974,7 @@ static void __init ppc4xx_pciex_port_setup_hose(struct ppc4xx_pciex_port *port)
 	if (mbase == NULL) {
 		printk(KERN_ERR "%pOF: Can't map internal config space !",
 		       port->node);
-		goto fail;
+		goto recheck_cfg_data;
 	}
 	hose->cfg_addr = mbase;

@@ -2007,7 +2007,7 @@ static void __init ppc4xx_pciex_port_setup_hose(struct ppc4xx_pciex_port *port)

 	/* Parse inbound mapping resources */
 	if (ppc4xx_parse_dma_ranges(hose, mbase, &dma_window) != 0)
-		goto fail;
+		goto unmap_io_mbase;

 	/* Configure outbound ranges POMs */
 	ppc4xx_configure_pciex_POMs(port, hose, mbase);
@@ -2064,13 +2064,14 @@ static void __init ppc4xx_pciex_port_setup_hose(struct ppc4xx_pciex_port *port)
 	}

 	return;
- fail:
-	if (hose)
-		pcibios_free_controller(hose);
+
+unmap_io_mbase:
+	iounmap(mbase);
+recheck_cfg_data:
 	if (cfg_data)
 		iounmap(cfg_data);
-	if (mbase)
-		iounmap(mbase);
+free_controller:
+	pcibios_free_controller(hose);
 }

 static void __init ppc4xx_probe_pciex_bridge(struct device_node *np)
--
2.40.0

[PATCH v2 2/4] powerpc/4xx: Fix exception handling in ppc4xx_probe_pcix_bridge()

[Markus Elfring]

Date: Thu, 16 Mar 2023 19:09:33 +0100

The label “fail” was used to jump to another pointer check despite of
the detail in the implementation of the function “ppc4xx_probe_pcix_bridge”
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 “ioremap” failed.

2. Use a more appropriate label instead.

3. Delete two questionable checks.

4. Adjust the exception handling for another if branch a bit more.

5. Remove a return statement at the end.


This issue was detected by using the Coccinelle software.

Fixes: 5738ec6d00b7abbcd4cd342af83fd18d192b0979 ("[POWERPC] 4xx: PLB to PCI-X support")
Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 arch/powerpc/platforms/4xx/pci.c | 18 +++++++-----------
 1 file changed, 7 insertions(+), 11 deletions(-)

diff --git a/arch/powerpc/platforms/4xx/pci.c b/arch/powerpc/platforms/4xx/pci.c
index 7336c7039b10..fdf13e12ca9d 100644
--- a/arch/powerpc/platforms/4xx/pci.c
+++ b/arch/powerpc/platforms/4xx/pci.c
@@ -564,13 +564,13 @@ static void __init ppc4xx_probe_pcix_bridge(struct device_node *np)
 	reg = ioremap(rsrc_reg.start, resource_size(&rsrc_reg));
 	if (reg == NULL) {
 		printk(KERN_ERR "%pOF: Can't map registers !", np);
-		goto fail;
+		return;
 	}

 	/* Allocate the host controller data structure */
 	hose = pcibios_alloc_controller(np);
 	if (!hose)
-		goto fail;
+		goto unmap_io;

 	hose->first_busno = bus_range ? bus_range[0] : 0x0;
 	hose->last_busno = bus_range ? bus_range[1] : 0xff;
@@ -595,8 +595,10 @@ static void __init ppc4xx_probe_pcix_bridge(struct device_node *np)
 	pci_process_bridge_OF_ranges(hose, np, primary);

 	/* Parse inbound mapping resources */
-	if (ppc4xx_parse_dma_ranges(hose, reg, &dma_window) != 0)
-		goto fail;
+	if (ppc4xx_parse_dma_ranges(hose, reg, &dma_window)) {
+		pcibios_free_controller(hose);
+		goto unmap_io;
+	}

 	/* Configure outbound ranges POMs */
 	ppc4xx_configure_pcix_POMs(hose, reg);
@@ -605,14 +607,8 @@ static void __init ppc4xx_probe_pcix_bridge(struct device_node *np)
 	ppc4xx_configure_pcix_PIMs(hose, reg, &dma_window, big_pim, msi);

 	/* We don't need the registers anymore */
+unmap_io:
 	iounmap(reg);
-	return;
-
- fail:
-	if (hose)
-		pcibios_free_controller(hose);
-	if (reg)
-		iounmap(reg);
 }

 #ifdef CONFIG_PPC4xx_PCI_EXPRESS
--
2.40.0

[PATCH v2 3/4] powerpc/4xx: Fix exception handling in ppc4xx_probe_pci_bridge()

[Markus Elfring]

Date: Sat, 25 Mar 2023 15:56:09 +0100

The label “fail” was used to jump to another pointer check despite of
the detail in the implementation of the function “ppc4xx_probe_pci_bridge”
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 “ioremap” failed.

2. Use a more appropriate label instead.

3. Delete two questionable checks.

4. Adjust the exception handling for another if branch a bit more.

5. Remove a return statement at the end.


This issue was detected by using the Coccinelle software.

Fixes: c839e0eff500af03de65e560c2e21c3831586e6e ("[POWERPC] 4xx: PLB to PCI 2.x support")
Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
V3:
A closing parenthesis (which was overlooked somehow) was preserved
for an if statement.

 arch/powerpc/platforms/4xx/pci.c | 18 +++++++-----------
 1 file changed, 7 insertions(+), 11 deletions(-)

diff --git a/arch/powerpc/platforms/4xx/pci.c b/arch/powerpc/platforms/4xx/pci.c
index fdf13e12ca9d..46ba0a4e5b04 100644
--- a/arch/powerpc/platforms/4xx/pci.c
+++ b/arch/powerpc/platforms/4xx/pci.c
@@ -358,13 +358,13 @@ static void __init ppc4xx_probe_pci_bridge(struct device_node *np)
 	reg = ioremap(rsrc_reg.start, resource_size(&rsrc_reg));
 	if (reg == NULL) {
 		printk(KERN_ERR "%pOF: Can't map registers !", np);
-		goto fail;
+		return;
 	}

 	/* Allocate the host controller data structure */
 	hose = pcibios_alloc_controller(np);
 	if (!hose)
-		goto fail;
+		goto unmap_io;

 	hose->first_busno = bus_range ? bus_range[0] : 0x0;
 	hose->last_busno = bus_range ? bus_range[1] : 0xff;
@@ -383,8 +383,10 @@ static void __init ppc4xx_probe_pci_bridge(struct device_node *np)
 	pci_process_bridge_OF_ranges(hose, np, primary);

 	/* Parse inbound mapping resources */
-	if (ppc4xx_parse_dma_ranges(hose, reg, &dma_window) != 0)
-		goto fail;
+	if (ppc4xx_parse_dma_ranges(hose, reg, &dma_window)) {
+		pcibios_free_controller(hose);
+		goto unmap_io;
+	}

 	/* Configure outbound ranges POMs */
 	ppc4xx_configure_pci_PMMs(hose, reg);
@@ -393,14 +395,8 @@ static void __init ppc4xx_probe_pci_bridge(struct device_node *np)
 	ppc4xx_configure_pci_PTMs(hose, reg, &dma_window);

 	/* We don't need the registers anymore */
+unmap_io:
 	iounmap(reg);
-	return;
-
- fail:
-	if (hose)
-		pcibios_free_controller(hose);
-	if (reg)
-		iounmap(reg);
 }

 /*
--
2.40.0

[PATCH v2 4/4] powerpc/4xx: Delete unnecessary variable initialisations in four functions

[Markus Elfring]

Date: Sat, 25 Mar 2023 16:00:36 +0100

Some local variables will be set to an appropriate value before usage.
Thus omit explicit initialisations at the beginning of these functions.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 arch/powerpc/platforms/4xx/pci.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/arch/powerpc/platforms/4xx/pci.c b/arch/powerpc/platforms/4xx/pci.c
index 46ba0a4e5b04..6fb7f9c966a6 100644
--- a/arch/powerpc/platforms/4xx/pci.c
+++ b/arch/powerpc/platforms/4xx/pci.c
@@ -323,8 +323,8 @@ static void __init ppc4xx_probe_pci_bridge(struct device_node *np)
 	struct resource rsrc_cfg;
 	struct resource rsrc_reg;
 	struct resource dma_window;
-	struct pci_controller *hose = NULL;
-	void __iomem *reg = NULL;
+	struct pci_controller *hose;
+	void __iomem *reg;
 	const int *bus_range;
 	int primary = 0;

@@ -523,8 +523,8 @@ static void __init ppc4xx_probe_pcix_bridge(struct device_node *np)
 	struct resource rsrc_cfg;
 	struct resource rsrc_reg;
 	struct resource dma_window;
-	struct pci_controller *hose = NULL;
-	void __iomem *reg = NULL;
+	struct pci_controller *hose;
+	void __iomem *reg;
 	const int *bus_range;
 	int big_pim = 0, msi = 0, primary = 0;

@@ -1403,7 +1403,7 @@ static struct ppc4xx_pciex_hwops ppc_476fpe_pcie_hwops __initdata =
 static int __init ppc4xx_pciex_check_core_init(struct device_node *np)
 {
 	static int core_init;
-	int count = -ENODEV;
+	int count;

 	if (core_init++)
 		return 0;
@@ -1905,10 +1905,10 @@ static void __init ppc4xx_configure_pciex_PIMs(struct ppc4xx_pciex_port *port,
 static void __init ppc4xx_pciex_port_setup_hose(struct ppc4xx_pciex_port *port)
 {
 	struct resource dma_window;
-	struct pci_controller *hose = NULL;
+	struct pci_controller *hose;
 	const int *bus_range;
 	int primary = 0, busses;
-	void __iomem *mbase = NULL, *cfg_data = NULL;
+	void __iomem *mbase, *cfg_data = NULL;
 	const u32 *pval;
 	u32 val;

--
2.40.0

Re: [PATCH resent v2 0/2] powerpc/pseries: Fixes for exception handling in pSeries_reconfig_add_node()

[Markus Elfring]

> Date: Tue, 21 Mar 2023 11:26:32 +0100
>
> A few update suggestions were taken into account
> from static source code analysis.
>
> Markus Elfring (2):
>   Do not pass an error pointer to of_node_put()
>   Fix exception handling
>
>  arch/powerpc/platforms/pseries/reconfig.c | 26 ++++++++++++-----------
>  1 file changed, 14 insertions(+), 12 deletions(-)

Is this patch series still in review queues?

Regards,
Markus

Re: [PATCH v2 0/4] powerpc/4xx: Adjustments for four function implementations

[Markus Elfring]

> Date: Sat, 25 Mar 2023 16:10:23 +0100
>
> Some update suggestions were taken into account
> from static source code analysis.
>
> Markus Elfring (4):
>   Fix exception handling in ppc4xx_pciex_port_setup_hose()
>   Fix exception handling in ppc4xx_probe_pcix_bridge()
>   Fix exception handling in ppc4xx_probe_pci_bridge()
>   Delete unnecessary variable initialisations in four functions
>
>  arch/powerpc/platforms/4xx/pci.c | 69 ++++++++++++++------------------
>  1 file changed, 31 insertions(+), 38 deletions(-)

Is this patch series still in review queues?

Regards,
Markus