linux-kernel - Re: [PATCH] PCI/hotplug: Remove unneeded of_node_put() in pnv

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] [day] [month] [year] [list]

Message-ID: <0622b8ea-7587-2e6b-5558-fae1847b14b9@linux.ibm.com>
Date:   Wed, 20 Oct 2021 09:57:23 -0700
From:   Tyrel Datwyler <tyreld@...ux.ibm.com>
To:     Nathan Lynch <nathanl@...ux.ibm.com>,
        Wan Jiabing <wanjiabing@...o.com>
Cc:     kael_w@...h.net, Michael Ellerman <mpe@...erman.id.au>,
        Benjamin Herrenschmidt <benh@...nel.crashing.org>,
        Paul Mackerras <paulus@...ba.org>,
        Bjorn Helgaas <bhelgaas@...gle.com>,
        linuxppc-dev@...ts.ozlabs.org, linux-pci@...r.kernel.org,
        linux-kernel@...r.kernel.org, ajd@...ux.ibm.com
Subject: Re: [PATCH] PCI/hotplug: Remove unneeded of_node_put() in pnv_php

On 10/20/21 4:39 AM, Nathan Lynch wrote:
> Wan Jiabing <wanjiabing@...o.com> writes:
>> Fix following coccicheck warning:
>> ./drivers/pci/hotplug/pnv_php.c:161:2-13: ERROR: probable double put.
>>
>> Device node iterators put the previous value of the index variable, so
>> an explicit put causes a double put.
> 
> I suppose Coccinelle doesn't take into account that this code is
> detaching and freeing the nodes.
> 
> 
>> diff --git a/drivers/pci/hotplug/pnv_php.c b/drivers/pci/hotplug/pnv_php.c
>> index f4c2e6e01be0..f3da4f95d73f 100644
>> --- a/drivers/pci/hotplug/pnv_php.c
>> +++ b/drivers/pci/hotplug/pnv_php.c
>> @@ -158,7 +158,6 @@ static void pnv_php_detach_device_nodes(struct device_node *parent)
>>  	for_each_child_of_node(parent, dn) {
>>  		pnv_php_detach_device_nodes(dn);
>>  
>> -		of_node_put(dn);
>>  		of_detach_node(dn);
>>  	}
> 
> The code might be improved by comments explaining how the bare
> of_node_put() corresponds to a "get" somewhere else in the driver, and
> how it doesn't render the ongoing traversal unsafe. It looks suspicious
> on first review, but I believe it's intentional and probably correct as
> written.
> 

This is a common usage pattern which if we put a comment about the pattern here
we need to do it every where. I suppose a better solution is to wrap this put in
a more descriptive function name like of_node_long_put() or something of the
sort the makes it obvious we are dropping a long held global scope reference.

-Tyrel