[PATCH 1/2] powerpc/pseries: fix of_node_put() underflow during dlpar remove

[PATCH 1/2] powerpc/pseries: fix of_node_put() underflow during dlpar remove

[Tyrel Datwyler]

Historically device_node references were tracked using a kref embedded
as a struct field. Commit 75b57ecf9 refactored device_nodes to be
kobjects such that the device tree could by more simply exposed to
userspace using sysfs. Commit 0829f6d1f6 followed up these changes to
better control the kobject lifecycle and in particular the referecne
counting via of_node_get(), of_node_put(), and of_node_init(). A side
effect of this second commit was that it introduced an of_node_put()
call when a dynamic node is detached that removes the initial kobj
reference created by of_node_init() . Traditionally as the original
dynamic device node user the pseries code had assumed responsibilty for
releasing this final reference in its platform specific DLPAR detach code.

This patch fixes a refcount underflow introduced by commit 0829f6d1f6,
and recently exposed by the upstreaming of the recount API.

Messages like the following are no longer seen in the kernel log with this
patch following DLPAR remove operations of cpus and pci devices.

  [  269.589441] rpadlpar_io: slot PHB 72 removed
  [  270.589997] refcount_t: underflow; use-after-free.
  [  270.590019] ------------[ cut here ]------------
  [  270.590025] WARNING: CPU: 5 PID: 3335 at
     lib/refcount.c:128 refcount_sub_and_test+0xf4/0x110

Cc: stable@vger.kernel.org
Fixes: 0829f6d1f69e ("of: device_node kobject lifecycle fixes")
Signed-off-by: Tyrel Datwyler <tyreld@linux.vnet.ibm.com>
---
 arch/powerpc/platforms/pseries/dlpar.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/arch/powerpc/platforms/pseries/dlpar.c b/arch/powerpc/platforms/pseries/dlpar.c
index 193e052..bda18d8 100644
--- a/arch/powerpc/platforms/pseries/dlpar.c
+++ b/arch/powerpc/platforms/pseries/dlpar.c
@@ -288,7 +288,6 @@ int dlpar_detach_node(struct device_node *dn)
 	if (rc)
 		return rc;
 
-	of_node_put(dn); /* Must decrement the refcount */
 	return 0;
 }
 
-- 
1.8.3.1

[PATCH 2/2] powerpc/sysfs: fix reference leak of cpu device_nodes present at boot

[Tyrel Datwyler]

For cpus present at boot each logical cpu acquires a reference to the
associated device node of the core. This happens in register_cpu() which
is called by topology_init(). The result of this is that we end up with
a reference held by each thread of the core. However, these references
are never freed if the cpu core is dlpar removed.

This patch fixes the reference leaks by acquiring and releasing the
references in the cpu hotplug callbacks un/register_cpu_online().
With this patch symmetric reference counting is observed with both cpus
present at boot, and those dlpar added after boot.

Cc: stable@vger.kernel.org
fixes: f86e4718f24b ("driver/core: cpu: initialize of_node in cpu's device struture")
Signed-off-by: Tyrel Datwyler <tyreld@linux.vnet.ibm.com>
---
 arch/powerpc/kernel/sysfs.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/arch/powerpc/kernel/sysfs.c b/arch/powerpc/kernel/sysfs.c
index c1fb255..949957b 100644
--- a/arch/powerpc/kernel/sysfs.c
+++ b/arch/powerpc/kernel/sysfs.c
@@ -710,6 +710,10 @@ static int register_cpu_online(unsigned int cpu)
 	struct device_attribute *attrs, *pmc_attrs;
 	int i, nattrs;
 
+	/* For cpus present at boot a reference was already grabbed in register_cpu() */
+	if (!s->of_node)
+		s->of_node = of_get_cpu_node(cpu, NULL);
+
 #ifdef CONFIG_PPC64
 	if (cpu_has_feature(CPU_FTR_SMT))
 		device_create_file(s, &dev_attr_smt_snooze_delay);
@@ -864,6 +868,8 @@ static int unregister_cpu_online(unsigned int cpu)
 	}
 #endif
 	cacheinfo_cpu_offline(cpu);
+	of_node_put(s->of_node);
+	s->of_node = NULL;
 #endif /* CONFIG_HOTPLUG_CPU */
 	return 0;
 }
-- 
1.8.3.1

Re: [2/2] powerpc/sysfs: fix reference leak of cpu device_nodes present at boot

[Michael Ellerman]

On Tue, 2017-04-18 at 00:24:39 UTC, Tyrel Datwyler wrote:
> For cpus present at boot each logical cpu acquires a reference to the
> associated device node of the core. This happens in register_cpu() which
> is called by topology_init(). The result of this is that we end up with
> a reference held by each thread of the core. However, these references
> are never freed if the cpu core is dlpar removed.
> 
> This patch fixes the reference leaks by acquiring and releasing the
> references in the cpu hotplug callbacks un/register_cpu_online().
> With this patch symmetric reference counting is observed with both cpus
> present at boot, and those dlpar added after boot.
> 
> Cc: stable@vger.kernel.org
> fixes: f86e4718f24b ("driver/core: cpu: initialize of_node in cpu's device struture")
> Signed-off-by: Tyrel Datwyler <tyreld@linux.vnet.ibm.com>

Applied to powerpc next, thanks.

https://git.kernel.org/powerpc/c/e76ca27790a514590af782f83f6eae

cheers

Re: [1/2] powerpc/pseries: fix of_node_put() underflow during dlpar remove

[Michael Ellerman]

On Tue, 2017-04-18 at 00:21:40 UTC, Tyrel Datwyler wrote:
> Historically device_node references were tracked using a kref embedded
> as a struct field. Commit 75b57ecf9 refactored device_nodes to be
> kobjects such that the device tree could by more simply exposed to
> userspace using sysfs. Commit 0829f6d1f6 followed up these changes to
> better control the kobject lifecycle and in particular the referecne
> counting via of_node_get(), of_node_put(), and of_node_init(). A side
> effect of this second commit was that it introduced an of_node_put()
> call when a dynamic node is detached that removes the initial kobj
> reference created by of_node_init() . Traditionally as the original
> dynamic device node user the pseries code had assumed responsibilty for
> releasing this final reference in its platform specific DLPAR detach code.
> 
> This patch fixes a refcount underflow introduced by commit 0829f6d1f6,
> and recently exposed by the upstreaming of the recount API.
> 
> Messages like the following are no longer seen in the kernel log with this
> patch following DLPAR remove operations of cpus and pci devices.
> 
>   [  269.589441] rpadlpar_io: slot PHB 72 removed
>   [  270.589997] refcount_t: underflow; use-after-free.
>   [  270.590019] ------------[ cut here ]------------
>   [  270.590025] WARNING: CPU: 5 PID: 3335 at
>      lib/refcount.c:128 refcount_sub_and_test+0xf4/0x110
> 
> Cc: stable@vger.kernel.org
> Fixes: 0829f6d1f69e ("of: device_node kobject lifecycle fixes")
> Signed-off-by: Tyrel Datwyler <tyreld@linux.vnet.ibm.com>

Applied to powerpc next, thanks.

https://git.kernel.org/powerpc/c/68baf692c435339e6295cb470ea554

cheers