[PATCH] powerpc/pseries: Fix dn reference error in dlpar_cpu_remove_by_index

[PATCH] powerpc/pseries: Fix dn reference error in dlpar_cpu_remove_by_index

[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;
 }
 

Re: [PATCH] powerpc/pseries: Fix dn reference error in dlpar_cpu_remove_by_index

[Tyrel Datwyler]

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

Re: [PATCH] powerpc/pseries: Fix dn reference error in dlpar_cpu_remove_by_index

[Michael Bringmann]

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