linux-kernel - Re: [PATCH v2] bus: (mvebu-mbus) Add missing of_node

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <d2637e89-db7e-e69a-f97b-ab87ed5af25a@wanadoo.fr>
Date:   Thu, 16 Jun 2022 21:08:29 +0200
From:   Christophe JAILLET <christophe.jaillet@...adoo.fr>
To:     Liang He <windhl@....com>, pali@...nel.org, lpieralisi@...nel.org
Cc:     linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] bus: (mvebu-mbus) Add missing of_node_put()

Le 16/06/2022 à 04:01, Liang He a écrit :
> In mvebu_mbus_dt_init(), of_find_matching_node_and_match() and
> of_find_node_by_phandle() will both return node pointers with
> refcount incremented. We should use of_node_put() in fail path
> or when it is not used anymore.
> 
> Signed-off-by: Liang He <windhl@....com>
> ---
>   changelog:
> 
>    v2: (1) use real name (2) add of_node_put when not used anymore
> 
>    v1: add of_node_put in fail path
> 
> 
>   drivers/bus/mvebu-mbus.c | 15 ++++++++++++++-
>   1 file changed, 14 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/bus/mvebu-mbus.c b/drivers/bus/mvebu-mbus.c
> index db612045616f..e168c8de2ae8 100644
> --- a/drivers/bus/mvebu-mbus.c
> +++ b/drivers/bus/mvebu-mbus.c
> @@ -1327,22 +1327,28 @@ int __init mvebu_mbus_dt_init(bool is_coherent)
>   
>   	prop = of_get_property(np, "controller", NULL);
>   	if (!prop) {
> +		of_node_put(np);
>   		pr_err("required 'controller' property missing\n");
>   		return -EINVAL;
>   	}
>   
>   	controller = of_find_node_by_phandle(be32_to_cpup(prop));
>   	if (!controller) {
> +		of_node_put(np);
>   		pr_err("could not find an 'mbus-controller' node\n");
>   		return -ENODEV;
>   	}
>   
>   	if (of_address_to_resource(controller, 0, &mbuswins_res)) {
> +		of_node_put(np);
> +		of_node_put(controller);
>   		pr_err("cannot get MBUS register address\n");
>   		return -EINVAL;
>   	}
>   
>   	if (of_address_to_resource(controller, 1, &sdramwins_res)) {
> +		of_node_put(np);
> +		of_node_put(controller);
>   		pr_err("cannot get SDRAM register address\n");
>   		return -EINVAL;
>   	}
> @@ -1360,6 +1366,8 @@ int __init mvebu_mbus_dt_init(bool is_coherent)
>   			pr_warn(FW_WARN "deprecated mbus-mvebu Device Tree, suspend/resume will not work\n");
>   	}
>   
> +	of_node_put(controller);	
> +
>   	mbus_state.hw_io_coherency = is_coherent;
>   
>   	/* Get optional pcie-{mem,io}-aperture properties */
> @@ -1378,6 +1386,11 @@ int __init mvebu_mbus_dt_init(bool is_coherent)
>   		return ret;

I guess that a "of_node_put(np);" is missing before this return.
This really looks like an error handling path and it has not been 
updated as all the other path, including the normal one below.

Moreover having an error handling path and some gotos is a more usual 
solution. It avoids code duplication. (but it is also a matter of taste...)

CJ

>   
>   	/* Setup statically declared windows in the DT */
> -	return mbus_dt_setup(&mbus_state, np);
> +	ret = mbus_dt_setup(&mbus_state, np);
> +
> +	of_node_put(np);
> +	
> +	return ret;
> +

Nitpick : This extra new line is not needed.

>   }
>   #endif