Re: [PATCH] powerpc/pseries: Fix dn reference error in dlpar_cpu_remove_by_index
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
>>
>>
>> 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
m...@linux.vnet.ibm.com
Re: [PATCH] powerpc/pseries: Fix dn reference error in dlpar_cpu_remove_by_index
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
>
>
> 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
[PATCH] powerpc/pseries: Fix dn reference error in dlpar_cpu_remove_by_index
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 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; }
