LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH] powerpc/pseries: Fix dn reference error in dlpar_cpu_remove_by_index
@ 2019-02-19 15:46 Michael Bringmann
  2019-02-19 20:03 ` Tyrel Datwyler
  0 siblings, 1 reply; 3+ messages in thread
From: Michael Bringmann @ 2019-02-19 15:46 UTC (permalink / raw)
  To: linuxppc-dev, linux-kernel
  Cc: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman,
	Nicholas Piggin, Rob Herring, Tyrel Datwyler, Juliet M. Kim,
	Nathan Lynch, Thomas Falcon, Michael Bringmann

powerpc/pseries: Fix dn reference error in dlpar_cpu_remove_by_index()

A reference to the device node of the CPU to be removed is released
upon successful removal of the associated CPU device.  If the call
to remove the CPU device fails, dlpar_cpu_remove_by_index() still
frees the reference and this leads to miscomparisons and/or
addressing errors later on.

This problem may be observed when trying to DLPAR 'hot-remove' a CPU
from a system that has only a single CPU.  The operation will fail
because there is no other CPU to which the kernel operations may be
migrated, but the refcount will still be decremented.

Signed-off-by: Michael Bringmann <mwb@linux.vnet.ibm.com>


diff --git a/arch/powerpc/platforms/pseries/hotplug-cpu.c b/arch/powerpc/platforms/pseries/hotplug-cpu.c
index 97feb6e..9537bb9 100644
--- a/arch/powerpc/platforms/pseries/hotplug-cpu.c
+++ b/arch/powerpc/platforms/pseries/hotplug-cpu.c
@@ -635,7 +635,8 @@ static int dlpar_cpu_remove_by_index(u32 drc_index)
 	}
 
 	rc = dlpar_cpu_remove(dn, drc_index);
-	of_node_put(dn);
+	if (!rc)
+		of_node_put(dn);
 	return rc;
 }
 


^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH] powerpc/pseries: Fix dn reference error in dlpar_cpu_remove_by_index
  2019-02-19 15:46 [PATCH] powerpc/pseries: Fix dn reference error in dlpar_cpu_remove_by_index Michael Bringmann
@ 2019-02-19 20:03 ` Tyrel Datwyler
  2019-02-20 20:18   ` Michael Bringmann
  0 siblings, 1 reply; 3+ messages in thread
From: Tyrel Datwyler @ 2019-02-19 20:03 UTC (permalink / raw)
  To: Michael Bringmann, linuxppc-dev, linux-kernel
  Cc: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman,
	Nicholas Piggin, Rob Herring, Juliet M. Kim, Nathan Lynch,
	Thomas Falcon

On 02/19/2019 07:46 AM, Michael Bringmann wrote:
> powerpc/pseries: Fix dn reference error in dlpar_cpu_remove_by_index()
> 
> A reference to the device node of the CPU to be removed is released
> upon successful removal of the associated CPU device.  If the call
> to remove the CPU device fails, dlpar_cpu_remove_by_index() still
> frees the reference and this leads to miscomparisons and/or
> addressing errors later on.
> 
> This problem may be observed when trying to DLPAR 'hot-remove' a CPU
> from a system that has only a single CPU.  The operation will fail
> because there is no other CPU to which the kernel operations may be
> migrated, but the refcount will still be decremented.
> 
> Signed-off-by: Michael Bringmann <mwb@linux.vnet.ibm.com>
> 
> 
> diff --git a/arch/powerpc/platforms/pseries/hotplug-cpu.c b/arch/powerpc/platforms/pseries/hotplug-cpu.c
> index 97feb6e..9537bb9 100644
> --- a/arch/powerpc/platforms/pseries/hotplug-cpu.c
> +++ b/arch/powerpc/platforms/pseries/hotplug-cpu.c
> @@ -635,7 +635,8 @@ static int dlpar_cpu_remove_by_index(u32 drc_index)
>  	}
> 
>  	rc = dlpar_cpu_remove(dn, drc_index);
> -	of_node_put(dn);
> +	if (!rc)
> +		of_node_put(dn);
>  	return rc;
>  }
> 

NACK!

The logic here is wrong. Here is the full function.

static int dlpar_cpu_remove_by_index(u32 drc_index)
{
        struct device_node *dn;
        int rc;

        dn = cpu_drc_index_to_dn(drc_index);
        if (!dn) {
                pr_warn("Cannot find CPU (drc index %x) to remove\n",
                        drc_index);
                return -ENODEV;
        }

        rc = dlpar_cpu_remove(dn, drc_index);
        of_node_put(dn);
        return rc;
}

The call to cpu_drc_index_to_dn() returns a device_node with the reference count
incremented. So, regardless of the success or failure of the call to
dlpar_cpu_remove() you need to release that reference.

If there is a reference counting issue it is somewhere else.

-Tyrel


^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH] powerpc/pseries: Fix dn reference error in dlpar_cpu_remove_by_index
  2019-02-19 20:03 ` Tyrel Datwyler
@ 2019-02-20 20:18   ` Michael Bringmann
  0 siblings, 0 replies; 3+ messages in thread
From: Michael Bringmann @ 2019-02-20 20:18 UTC (permalink / raw)
  To: Tyrel Datwyler, linuxppc-dev, linux-kernel
  Cc: Rob Herring, Thomas Falcon, Nicholas Piggin, Paul Mackerras,
	Nathan Lynch, Juliet M. Kim

On 2/19/19 2:03 PM, Tyrel Datwyler wrote:
> On 02/19/2019 07:46 AM, Michael Bringmann wrote:
>> powerpc/pseries: Fix dn reference error in dlpar_cpu_remove_by_index()
>>
>> A reference to the device node of the CPU to be removed is released
>> upon successful removal of the associated CPU device.  If the call
>> to remove the CPU device fails, dlpar_cpu_remove_by_index() still
>> frees the reference and this leads to miscomparisons and/or
>> addressing errors later on.
>>
>> This problem may be observed when trying to DLPAR 'hot-remove' a CPU
>> from a system that has only a single CPU.  The operation will fail
>> because there is no other CPU to which the kernel operations may be
>> migrated, but the refcount will still be decremented.
>>
>> Signed-off-by: Michael Bringmann <mwb@linux.vnet.ibm.com>
>>
>>
>> diff --git a/arch/powerpc/platforms/pseries/hotplug-cpu.c b/arch/powerpc/platforms/pseries/hotplug-cpu.c
>> index 97feb6e..9537bb9 100644
>> --- a/arch/powerpc/platforms/pseries/hotplug-cpu.c
>> +++ b/arch/powerpc/platforms/pseries/hotplug-cpu.c
>> @@ -635,7 +635,8 @@ static int dlpar_cpu_remove_by_index(u32 drc_index)
>>  	}
>>
>>  	rc = dlpar_cpu_remove(dn, drc_index);
>> -	of_node_put(dn);
>> +	if (!rc)
>> +		of_node_put(dn);
>>  	return rc;
>>  }
>>
> 
> NACK!
> 
> The logic here is wrong. Here is the full function.
> 
> static int dlpar_cpu_remove_by_index(u32 drc_index)
> {
>         struct device_node *dn;
>         int rc;
> 
>         dn = cpu_drc_index_to_dn(drc_index);
>         if (!dn) {
>                 pr_warn("Cannot find CPU (drc index %x) to remove\n",
>                         drc_index);
>                 return -ENODEV;
>         }
> 
>         rc = dlpar_cpu_remove(dn, drc_index);
>         of_node_put(dn);
>         return rc;
> }
> 
> The call to cpu_drc_index_to_dn() returns a device_node with the reference count
> incremented. So, regardless of the success or failure of the call to
> dlpar_cpu_remove() you need to release that reference.
> 
> If there is a reference counting issue it is somewhere else.

Okay.  Withdrawn while we look some more.

> -Tyrel

-- 
Michael W. Bringmann
Linux Technology Center
IBM Corporation
Tie-Line  363-5196
External: (512) 286-5196
Cell:       (512) 466-0650
mwb@linux.vnet.ibm.com


^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2019-02-20 20:19 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-02-19 15:46 [PATCH] powerpc/pseries: Fix dn reference error in dlpar_cpu_remove_by_index Michael Bringmann
2019-02-19 20:03 ` Tyrel Datwyler
2019-02-20 20:18   ` Michael Bringmann

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).