In dtb_setup_hpet(), of_find_compatible_node() will return a node
pointer with refcount incremented. We should use of_node_put() when it
is not used anymore.

Signed-off-by: Liang He <windhl@126.com>
---
 arch/x86/kernel/devicetree.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/arch/x86/kernel/devicetree.c b/arch/x86/kernel/devicetree.c
index 5cd51f25f446..6a386424ddf7 100644
--- a/arch/x86/kernel/devicetree.c
+++ b/arch/x86/kernel/devicetree.c
@@ -120,6 +120,9 @@ static void __init dtb_setup_hpet(void)
 	if (!dn)
 		return;
 	ret = of_address_to_resource(dn, 0, &r);
+	
+	of_node_put(dn);
+	
 	if (ret) {
 		WARN_ON(1);
 		return;
-- 
2.25.1

On Wed, Jun 15, 2022 at 9:03 AM Liang He <windhl@126.com> wrote:
>
> In dtb_setup_hpet(), of_find_compatible_node() will return a node
> pointer with refcount incremented. We should use of_node_put() when it
> is not used anymore.
>
> Signed-off-by: Liang He <windhl@126.com>
> ---
>  arch/x86/kernel/devicetree.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/arch/x86/kernel/devicetree.c b/arch/x86/kernel/devicetree.c
> index 5cd51f25f446..6a386424ddf7 100644
> --- a/arch/x86/kernel/devicetree.c
> +++ b/arch/x86/kernel/devicetree.c
> @@ -120,6 +120,9 @@ static void __init dtb_setup_hpet(void)
>         if (!dn)
>                 return;
>         ret = of_address_to_resource(dn, 0, &r);
> +
> +       of_node_put(dn);
> +

You don't want a put on success. If you are using the device, then you
want to hold a reference to it.

I would guess that if you have an error here and don't have your
timer, you're not going to finish booting anyways.

Finally, wouldn't dtb_lapic_setup() and dtb_add_ioapic() also need a
similar change? But again, if those aren't initialized, you probably
aren't getting very far.


>         if (ret) {
>                 WARN_ON(1);
>                 return;
> --
> 2.25.1
>

Hi, rob, thanks for your reply.

The of_find_xx will increase the refcounter for the local reference, so when 
the function return, we need a decrease for the destroy of the local reference.
The device will not be freed as its refcounter will be sure larger than 0 when 
the function returns.

All we need is to keep the refcounting balance, right?




At 2022-06-16 00:26:16, "Rob Herring" <robh@kernel.org> wrote:
>On Wed, Jun 15, 2022 at 9:03 AM Liang He <windhl@126.com> wrote:
>>
>> In dtb_setup_hpet(), of_find_compatible_node() will return a node
>> pointer with refcount incremented. We should use of_node_put() when it
>> is not used anymore.
>>
>> Signed-off-by: Liang He <windhl@126.com>
>> ---
>>  arch/x86/kernel/devicetree.c | 3 +++
>>  1 file changed, 3 insertions(+)
>>
>> diff --git a/arch/x86/kernel/devicetree.c b/arch/x86/kernel/devicetree.c
>> index 5cd51f25f446..6a386424ddf7 100644
>> --- a/arch/x86/kernel/devicetree.c
>> +++ b/arch/x86/kernel/devicetree.c
>> @@ -120,6 +120,9 @@ static void __init dtb_setup_hpet(void)
>>         if (!dn)
>>                 return;
>>         ret = of_address_to_resource(dn, 0, &r);
>> +
>> +       of_node_put(dn);
>> +
>
>You don't want a put on success. If you are using the device, then you
>want to hold a reference to it.
>
>I would guess that if you have an error here and don't have your
>timer, you're not going to finish booting anyways.
>
>Finally, wouldn't dtb_lapic_setup() and dtb_add_ioapic() also need a
>similar change? But again, if those aren't initialized, you probably
>aren't getting very far.
>
>
>>         if (ret) {
>>                 WARN_ON(1);
>>                 return;
>> --
>> 2.25.1
>>

On 6/15/22 08:03, Liang He wrote:
> In dtb_setup_hpet(), of_find_compatible_node() will return a node
> pointer with refcount incremented. We should use of_node_put() when it
> is not used anymore.
> 
> Signed-off-by: Liang He <windhl@126.com>

Seems like:

Fixes: ffb9fc68dff3 ("x86: dtb: Add device tree support for HPET")

and would be appropriate, right?

Also, how was this found, and what is the impact from not fixing it?
Was it causing horrible problems in production, or was it just something
that was found by inspection that's not causing any real problems in
practice?

On Wed, Jun 15, 2022 at 9:33 AM Dave Hansen <dave.hansen@intel.com> wrote:
>
> On 6/15/22 08:03, Liang He wrote:
> > In dtb_setup_hpet(), of_find_compatible_node() will return a node
> > pointer with refcount incremented. We should use of_node_put() when it
> > is not used anymore.
> >
> > Signed-off-by: Liang He <windhl@126.com>
>
> Seems like:
>
> Fixes: ffb9fc68dff3 ("x86: dtb: Add device tree support for HPET")
>
> and would be appropriate, right?
>
> Also, how was this found, and what is the impact from not fixing it?

No impact if the node is not dynamically removed. That's almost all
cases except IBM pSeries cpu, memory, and pci. Overlays could change
that, but their support in the kernel is limited.

If it really mattered, we'd probably come up with a different way to
do the refcounting as it is hard to get right. In fact, this fix is
not right. Will reply with the right context.

> Was it causing horrible problems in production, or was it just something
> that was found by inspection that's not causing any real problems in
> practice?