| Message ID | cc20c5cc-cf8e-d125-cac9-5d1b938a1d86@linux.vnet.ibm.com (mailing list archive) |
|---|---|
| State | Rejected |
| Headers | show |
| Series | powerpc/pseries: Fix dn reference error in dlpar_cpu_remove_by_index | expand |
| Context | Check | Description |
|---|---|---|
| snowpatch_ozlabs/apply_patch | success | next/apply_patch Successfully applied |
| snowpatch_ozlabs/build-ppc64le | success | build succeeded & removed 0 sparse warning(s) |
| snowpatch_ozlabs/build-ppc64be | success | build succeeded & removed 0 sparse warning(s) |
| snowpatch_ozlabs/build-ppc64e | success | build succeeded & removed 0 sparse warning(s) |
| snowpatch_ozlabs/build-pmac32 | success | build succeeded & removed 0 sparse warning(s) |
| snowpatch_ozlabs/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 9 lines checked |
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
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
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; }
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>