linux-kernel - Re: [PATCH] of/address: Add error logging for of_match

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: <20250818164907.GA1437284-robh@kernel.org>
Date: Mon, 18 Aug 2025 11:49:07 -0500
From: Rob Herring <robh@...nel.org>
To: Zhenhua Huang <quic_zhenhuah@...cinc.com>
Cc: saravanak@...gle.com, devicetree@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] of/address: Add error logging for of_match_bus() in
 address translation path

On Mon, Aug 11, 2025 at 05:53:42PM +0800, Zhenhua Huang wrote:
> The change introduced in
> commit 045b14ca5c36 ("of: WARN on deprecated #address-cells/#size-cells handling")
> triggers a warning on the direct ancestor node when translating properties
> like "iommu-addresses"/"reg". However, it fails to issue a warning if the
> ancestor’s ancestor is missing the required cells.
> For instance, if node_c lacks the necessary properties, no warning will be
> generated. Potential issues will be trigger further.

The point of the WARN is to only to check the immediate ancestor.

> node_c {
> 		//NO WARN
> 	node_b {
> 		//WARN on missing of "address-cells" and "size-cells"
> 		node_a {
> 			xxx = <memory_reion>  //contains "iommu-addresses"
> 		}
> 	}
> }

Whether a warning is appropriate here depends on whether there's 
'ranges' properties or not. If your schemas are complete, then they 
should warn on missing 'ranges'. If ranges is present, then we should 
get warnings if #address-cells or #size-cells is missing.

> Since of_match_bus() is now expected to succeed in traslation path,

now expected? Nothing changed in that aspect.

> routine __of_translate_address. Print an error message would help in
> identifying cases where it fails, making such issues easier to diagnose.

For errors in the DT (as opposed to errors using the API), it would be 
better if we can check this at build time rather than run-time. And 
generally I think we should already, but there could be some corner case 
that we don't.

> 
> Signed-off-by: Zhenhua Huang <quic_zhenhuah@...cinc.com>
> ---
>  drivers/of/address.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/of/address.c b/drivers/of/address.c
> index f0f8f0dd191c..cd33ab64ccf3 100644
> --- a/drivers/of/address.c
> +++ b/drivers/of/address.c
> @@ -515,8 +515,10 @@ static u64 __of_translate_address(struct device_node *node,
>  	if (parent == NULL)
>  		return OF_BAD_ADDR;
>  	bus = of_match_bus(parent);
> -	if (!bus)
> +	if (!bus) {
> +		pr_err("of_match_bus failed for device node(%pOF)\n", parent);
>  		return OF_BAD_ADDR;
> +	}
>  
>  	/* Count address cells & copy address locally */
>  	bus->count_cells(dev, &na, &ns);
> @@ -560,8 +562,10 @@ static u64 __of_translate_address(struct device_node *node,
>  
>  		/* Get new parent bus and counts */
>  		pbus = of_match_bus(parent);
> -		if (!pbus)
> +		if (!pbus) {
> +			pr_err("of_match_bus failed for device node(%pOF)\n", parent);
>  			return OF_BAD_ADDR;

If there's no case we expect of_match_bus() failing is correct 
operation, then the error msg should be in the of_match_bus() function 
rather than duplicated here. I'm not sure if there is any such case.

Rob