On Sun 04 Aug 09:25 PDT 2019, Nishka Dasgupta wrote:
> Each iteration of for_each_available_child_of_node puts the previous
> node, but in the case of a goto from the middle of the loop, there is no
> put, thus causing a memory leak. Hence make the gotos within the loop
> first go to a new label where an of_node_put() puts the last used node,
> before falling through to the original label.
> Issue found with Coccinelle.
>
Good catch, thanks for the patch!
> Signed-off-by: Nishka Dasgupta <nishkadg.linux@xxxxxxxxx>
> ---
> drivers/soc/qcom/smp2p.c | 10 ++++++----
> 1 file changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/soc/qcom/smp2p.c b/drivers/soc/qcom/smp2p.c
> index c7300d54e444..d223e914487d 100644
> --- a/drivers/soc/qcom/smp2p.c
> +++ b/drivers/soc/qcom/smp2p.c
> @@ -501,7 +501,7 @@ static int qcom_smp2p_probe(struct platform_device *pdev)
> entry = devm_kzalloc(&pdev->dev, sizeof(*entry), GFP_KERNEL);
> if (!entry) {
> ret = -ENOMEM;
> - goto unwind_interfaces;
> + goto release_child;
> }
>
> entry->smp2p = smp2p;
> @@ -509,18 +509,18 @@ static int qcom_smp2p_probe(struct platform_device *pdev)
>
> ret = of_property_read_string(node, "qcom,entry-name", &entry->name);
> if (ret < 0)
> - goto unwind_interfaces;
> + goto release_child;
>
> if (of_property_read_bool(node, "interrupt-controller")) {
> ret = qcom_smp2p_inbound_entry(smp2p, entry, node);
> if (ret < 0)
> - goto unwind_interfaces;
> + goto release_child;
>
> list_add(&entry->node, &smp2p->inbound);
> } else {
> ret = qcom_smp2p_outbound_entry(smp2p, entry, node);
> if (ret < 0)
> - goto unwind_interfaces;
> + goto release_child;
>
> list_add(&entry->node, &smp2p->outbound);
> }
> @@ -541,6 +541,8 @@ static int qcom_smp2p_probe(struct platform_device *pdev)
>
> return 0;
>
> +release_child:
> + of_node_put(node);
Following the loop over the children we request the irq and if that
falls we'll jump to unwind_interfaces. So while it would work fine to
jump to release_child within the loop and then unwind_interfaces after
the loop, it doesn't follow the idiomatic way of using the error path to
"unroll" things that has been setup up until a particular point in the
function.
So I would rather see that you of_node_put() in the loop and then jump
to unwind_interfaces as is done today.
Regards,
Bjorn
> unwind_interfaces:
> list_for_each_entry(entry, &smp2p->inbound, node)
> irq_domain_remove(entry->domain);
> --
> 2.19.1
>
