powerpc/pseries: Fix dn reference error in dlpar_cpu_remove_by_index
diff mbox series

Message ID cc20c5cc-cf8e-d125-cac9-5d1b938a1d86@linux.vnet.ibm.com
State Rejected
Headers show
Series
  • powerpc/pseries: Fix dn reference error in dlpar_cpu_remove_by_index
Related show

Checks

Context Check Description
snowpatch_ozlabs/checkpatch success total: 0 errors, 0 warnings, 0 checks, 9 lines checked
snowpatch_ozlabs/build-pmac32 success build succeeded & removed 0 sparse warning(s)
snowpatch_ozlabs/build-ppc64e success build succeeded & removed 0 sparse warning(s)
snowpatch_ozlabs/build-ppc64be success build succeeded & removed 0 sparse warning(s)
snowpatch_ozlabs/build-ppc64le success build succeeded & removed 0 sparse warning(s)
snowpatch_ozlabs/apply_patch success next/apply_patch Successfully applied

Commit Message

Michael Bringmann Feb. 19, 2019, 3:46 p.m. UTC
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>

Comments

Tyrel Datwyler Feb. 19, 2019, 8:03 p.m. UTC | #1
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
Michael Bringmann Feb. 20, 2019, 8:18 p.m. UTC | #2
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

Patch
diff mbox series

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