linux-kernel - Re: [PATCH] tty: hvc: hvc_opal: eliminate uses of 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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <3823976c-13b6-4662-a9fd-7615cf69475a@gmail.com>
Date: Fri, 3 May 2024 13:54:41 +0200
From: Javier Carrasco <javier.carrasco.cruz@...il.com>
To: Lu Dai <dai.lu@...rdes.com>, npiggin@...il.com,
 christophe.leroy@...roup.eu, naveen.n.rao@...ux.ibm.com
Cc: mpe@...erman.id.au, gregkh@...uxfoundation.org, jirislaby@...nel.org,
 linuxppc-dev@...ts.ozlabs.org, linux-kernel@...r.kernel.org,
 linux-serial@...r.kernel.org, shuah@...nel.org
Subject: Re: [PATCH] tty: hvc: hvc_opal: eliminate uses of of_node_put()

On 5/3/24 13:43, Lu Dai wrote:
> Make use of the __free() cleanup handler to automatically free nodes
> when they get out of scope.
> 
> Removes the need for a 'goto' as an effect.
> 
> Signed-off-by: Lu Dai <dai.lu@...rdes.com>
> ---
>  drivers/tty/hvc/hvc_opal.c | 9 +++------
>  1 file changed, 3 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/tty/hvc/hvc_opal.c b/drivers/tty/hvc/hvc_opal.c
> index 095c33ad10f8..67e90fa993a3 100644
> --- a/drivers/tty/hvc/hvc_opal.c
> +++ b/drivers/tty/hvc/hvc_opal.c
> @@ -327,14 +327,14 @@ static void udbg_init_opal_common(void)
>  
>  void __init hvc_opal_init_early(void)
>  {
> -	struct device_node *stdout_node = of_node_get(of_stdout);
> +	struct device_node *stdout_node __free(device_node) = of_node_get(of_stdout);
>  	const __be32 *termno;
>  	const struct hv_ops *ops;
>  	u32 index;
>  
>  	/* If the console wasn't in /chosen, try /ibm,opal */
>  	if (!stdout_node) {
> -		struct device_node *opal, *np;

Generally, you should always initialize the variable where it is
declared. What would happen if the variable goes out of scope before it
gets initialized? Now it is not dangerous, but if new code is added and
it returns because of some error, we might run into trouble.

In this particular case you can solve this easily by putting together
your modification and the assignment right after the comment.


> +		struct device_node *opal __free(device_node), *np;
>  
>  		/* Current OPAL takeover doesn't provide the stdout
>  		 * path, so we hard wire it
> @@ -356,7 +356,6 @@ void __init hvc_opal_init_early(void)
>  				break;
>  			}
>  		}
> -		of_node_put(opal);
>  	}
>  	if (!stdout_node)
>  		return;
> @@ -382,13 +381,11 @@ void __init hvc_opal_init_early(void)
>  		hvsilib_establish(&hvc_opal_boot_priv.hvsi);
>  		pr_devel("hvc_opal: Found HVSI console\n");
>  	} else
> -		goto out;
> +		return;
>  	hvc_opal_boot_termno = index;
>  	udbg_init_opal_common();
>  	add_preferred_console("hvc", index, NULL);
>  	hvc_instantiate(index, index, ops);
> -out:
> -	of_node_put(stdout_node);
>  }
>  
>  #ifdef CONFIG_PPC_EARLY_DEBUG_OPAL_RAW


Best regards,
Javier Carrasco