linux-kernel - Re: [PATCH] powerpc/pci: Remove useless null check before call 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: <c4613523-de98-b824-175a-89fd66931bd6@linux.ibm.com>
Date:   Fri, 22 Apr 2022 13:05:51 -0700
From:   Tyrel Datwyler <tyreld@...ux.ibm.com>
To:     Haowen Bai <baihaowen@...zu.com>,
        Michael Ellerman <mpe@...erman.id.au>,
        Benjamin Herrenschmidt <benh@...nel.crashing.org>,
        Paul Mackerras <paulus@...ba.org>
Cc:     linuxppc-dev@...ts.ozlabs.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] powerpc/pci: Remove useless null check before call
 of_node_put()

On 4/20/22 19:52, Haowen Bai wrote:
> No need to add null check before call of_node_put(), since the
> implementation of of_node_put() has done it.
> 
> Signed-off-by: Haowen Bai <baihaowen@...zu.com>
> ---
>  arch/powerpc/kernel/pci_dn.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/powerpc/kernel/pci_dn.c b/arch/powerpc/kernel/pci_dn.c
> index 61571ae23953..ba3bbc9bec2d 100644
> --- a/arch/powerpc/kernel/pci_dn.c
> +++ b/arch/powerpc/kernel/pci_dn.c
> @@ -357,8 +357,8 @@ void pci_remove_device_node_info(struct device_node *dn)
> 
>  	/* Drop the parent pci_dn's ref to our backing dt node */
>  	parent = of_get_parent(dn);
> -	if (parent)
> -		of_node_put(parent);
> +
> +	of_node_put(parent);

This whole block of code looks useless, or suspect. Examining the rest of the
code for this function this is the only place that parent is referenced. The
of_get_parent() call returns the parent with its refcount incremented, and then
we turn around and call of_node_put() which drops that reference we just took.
The comment doesn't do what it says it does. If we really need to drop a
previous reference to the parent device node this code block would need to call
of_node_put() twice on parent to accomplish that.

A closer examination is required to determine if what the comment says we need
to do is required. If it is then the code as it exists today is leaking that
reference AFAICS.

-Tyrel

> 
>  	/*
>  	 * At this point we *might* still have a pci_dev that was