On Thu, Mar 09, 2017 at 01:34:00PM -0800, Tyrel Datwyler wrote: > On 03/08/2017 08:37 PM, Bharata B Rao wrote: > > The following warning is seen when a CPU is hot unplugged on a PowerKVM > > guest: > > Is this the case with cpus present at boot? What about cpus hotplugged > after boot?
I have observed this for CPUs that are hotplugged. > > My suspicion is that the refcount was wrong to begin with. See my > comments below. The use of the of_node_put() calls is correct as in each > case we incremented the ref count earlier in the same function. > > > > > refcount_t: underflow; use-after-free. > > ------------[ cut here ]------------ > > WARNING: CPU: 0 PID: 53 at lib/refcount.c:128 > > refcount_sub_and_test+0xd8/0xf0 > > Modules linked in: > > CPU: 0 PID: 53 Comm: kworker/u510:1 Not tainted 4.11.0-rc1 #3 > > Workqueue: pseries hotplug workque pseries_hp_work_fn > > task: c0000000fb475000 task.stack: c0000000fb81c000 > > NIP: c0000000006f0808 LR: c0000000006f0804 CTR: c0000000007b98c0 > > REGS: c0000000fb81f710 TRAP: 0700 Not tainted (4.11.0-rc1) > > MSR: 800000000282b033 <SF,VEC,VSX,EE,FP,ME,IR,DR,RI,LE> > > CR: 48002222 XER: 20000000 > > CFAR: c000000000c438e0 SOFTE: 1 > > GPR00: c0000000006f0804 c0000000fb81f990 c000000001573b00 0000000000000026 > > GPR04: 0000000000000000 000000000000016c 667265652e0d0a73 652d61667465722d > > GPR08: 0000000000000007 0000000000000007 0000000000000001 0000000000000006 > > GPR12: 0000000000002200 c00000000ff40000 c00000000010c578 c0000001f11b9f40 > > GPR16: c0000001fe0312a8 c0000001fe031078 c0000001fe031020 0000000000000001 > > GPR20: 0000000000000000 0000000000000000 c000000001454808 fffffffffffffef7 > > GPR24: 0000000000000000 c0000001f1677648 0000000000000000 0000000000000000 > > GPR28: 0000000010000008 c000000000e4d3d8 0000000000000000 c0000001eaae07d8 > > NIP [c0000000006f0808] refcount_sub_and_test+0xd8/0xf0 > > LR [c0000000006f0804] refcount_sub_and_test+0xd4/0xf0 > > Call Trace: > > [c0000000fb81f990] [c0000000006f0804] refcount_sub_and_test+0xd4/0xf0 > > (unreliable) > > [c0000000fb81f9f0] [c0000000006d04b4] kobject_put+0x44/0x2a0 > > [c0000000fb81fa70] [c0000000009d5284] of_node_put+0x34/0x50 > > [c0000000fb81faa0] [c0000000000aceb8] dlpar_cpu_remove_by_index+0x108/0x130 > > [c0000000fb81fb30] [c0000000000ae128] dlpar_cpu+0x78/0x550 > > [c0000000fb81fbe0] [c0000000000a7b40] handle_dlpar_errorlog+0xc0/0x160 > > [c0000000fb81fc50] [c0000000000a7c74] pseries_hp_work_fn+0x94/0xa0 > > [c0000000fb81fc80] [c000000000102cec] process_one_work+0x23c/0x540 > > [c0000000fb81fd20] [c00000000010309c] worker_thread+0xac/0x620 > > [c0000000fb81fdc0] [c00000000010c6c4] kthread+0x154/0x1a0 > > [c0000000fb81fe30] [c00000000000bbe0] ret_from_kernel_thread+0x5c/0x7c > > > > Fix this by ensuring that of_node_put() is called only from the > > error path in dlpar_cpu_remove_by_index(). In the normal path, > > of_node_put() happens as part of dlpar_detach_node(). > > > > Signed-off-by: Bharata B Rao <bhar...@linux.vnet.ibm.com> > > Cc: Nathan Fontenot <nf...@linux.vnet.ibm.com> > > --- > > Changes in v1: > > - Fixed the refcount problem in the userspace driven unplug path > > in addition to in-kernel unplug path. (Sachin Sant) > > > > v0: https://patchwork.ozlabs.org/patch/736547/ > > > > arch/powerpc/platforms/pseries/hotplug-cpu.c | 12 ++++++++---- > > 1 file changed, 8 insertions(+), 4 deletions(-) > > > > diff --git a/arch/powerpc/platforms/pseries/hotplug-cpu.c > > b/arch/powerpc/platforms/pseries/hotplug-cpu.c > > index 7bc0e91..c5ed510 100644 > > --- a/arch/powerpc/platforms/pseries/hotplug-cpu.c > > +++ b/arch/powerpc/platforms/pseries/hotplug-cpu.c > > @@ -619,7 +619,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); > > I think there is another issue at play here because this is wrong. > Regardless of whether the dlpar_cpu_remove() succeeds or fails we still > need of_node_put() for both cases because we incremented the ref count > earlier in this function with a call to cpu_drc_index_to_dn() call. That > call doesn't, but shoul, document that it returns a device_node with > incremented refcount. > > > return rc; > > } > > > > @@ -856,9 +857,12 @@ static ssize_t dlpar_cpu_release(const char *buf, > > size_t count) > > } > > > > rc = dlpar_cpu_remove(dn, drc_index); > > - of_node_put(dn); > > - > > - return rc ? rc : count; > > + if (rc) { > > + of_node_put(dn); > > + return rc; > > + } else { > > + return count; > > + } > > Same comment as above. The call earlier in the function to > of_find_node_by_path() returned a device_node struct with its ref count > incremented. So, regardless of whether dlpar_cpu_remove() succeeds or > fails we need decrement the ref count with of_node_put(). > > Looking closer at the call paths for attach and detach one will notice > that __of_attach_node_sysfs() does not take a device_node reference with > of_node_get(), but __of_detach_node_sysfs() does a of_node_put(). In the > old days we use to keep the device tree in /proc. Now it lives in sysfs > and is symlinked to /proc for userspace ABI reasons. Further, pseries > was the only platform in those days that did any sort of dynamic OF > operations. So, in those dark days we were responsible for calling > of_node_put in dlpar_detach_node() to decrement the of_node_init() > reference. Looking at the comments in __of_detach_node_sysfs() it seems > that they expect to decrement that reference there now. > > void __of_detach_node_sysfs(struct device_node *np) > { > > ...snip... > > /* finally remove the kobj_init ref */ > of_node_put(np); > } > So you suggest that adding of_node_get() to __of_attach_node_sysfs() is the right fix ? Regards, Bharata.