From: Michael Ellerman <mpe@ellerman.id.au> Date: 2017-09-20 11:39:13
Rob Herring [off-list ref] writes:
On Fri, Sep 15, 2017 at 6:04 AM, abdul [off-list ref] wrote:
quoted
Hi,
Mainline kernel panics during DLPAR CPU add/remove operation.
Machine Type: Power8 PowerVM LPAR
kernel 4.13.0
Did 4.12 work or when was it last working? I'm not seeing anything
recent in the DT code that looks suspicious.
I'm pretty sure it's:
int dlpar_attach_node(struct device_node *dn, struct device_node *parent)
{
int rc;
dn->parent = parent;
rc = of_attach_node(dn);
if (rc) {
printk(KERN_ERR "Failed to add device node %pOF\n", dn);
return rc;
}
of_node_put(dn->parent);
HERE ^^^^^^^^^^
return 0;
}
Prior to 215ee763f8cb ("powerpc: pseries: remove dlpar_attach_node
dependency on full path"), we re-looked up the parent, and got another
reference on it. That meant the put before the return there was correct.
But now it's not because the caller has a reference to parent but it's
not ours to drop.
Testing a fix, will report back.
cheers
On Fri, Sep 15, 2017 at 6:04 AM, abdul [off-list ref] wrote:
quoted
Hi,
Mainline kernel panics during DLPAR CPU add/remove operation.
Machine Type: Power8 PowerVM LPAR
kernel 4.13.0
Did 4.12 work or when was it last working? I'm not seeing anything
recent in the DT code that looks suspicious.
I'm pretty sure it's:
int dlpar_attach_node(struct device_node *dn, struct device_node *parent)
{
int rc;
dn->parent = parent;
rc = of_attach_node(dn);
if (rc) {
printk(KERN_ERR "Failed to add device node %pOF\n", dn);
return rc;
}
of_node_put(dn->parent);
HERE ^^^^^^^^^^
return 0;
}
Prior to 215ee763f8cb ("powerpc: pseries: remove dlpar_attach_node
dependency on full path"), we re-looked up the parent, and got another
reference on it. That meant the put before the return there was correct.
But now it's not because the caller has a reference to parent but it's
not ours to drop.
Testing a fix, will report back.
So, that patch slipped past me. Not only is the parent reference not ours to drop, but
when I went and looked at dlpar_cpu_add() I also noticed that of_node_put() was done on
the parent prior to the call to dlpar_attach_node(). With the addition of "parent" to the
dlpar_attach_node() parameter list dlpar_cpu_add() needs to be fixed up to hold the
"parent" reference until after dlpar_attach_node() returns.
-Tyrel
A reference to the parent device node is held by add_dt_node() for the
node to be added. If the call to dlpar_configure_connector() fails
add_dt_node() returns ENOENT and that reference is not freed.
Add a call to of_node_put(parent_dn) prior to bailing out after a failed
dlpar_configure_connector() call.
Cc: stable@vger.kernel.org # v3.12+
Fixes: 8d5ff320766f ("powerpc/pseries: Make dlpar_configure_connector parent node aware")
Signed-off-by: Tyrel Datwyler <redacted>
---
arch/powerpc/platforms/pseries/mobility.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
Commit 215ee763f8cb ("powerpc: pseries: remove dlpar_attach_node dependency on
full path") reworked dlpar_attach_node() to no longer look up the parent
node "/cpus", but instead to have the parent node passed by the caller in the
function parameter list. As a result dlpar_attach_node() is no longer
responsible for freeing the reference to the parent node. However,
commit 215ee763f8cb failed to remove the of_node_put(parent) call in
dlpar_attach_node(), or to take into account that the reference to the
parent in the caller dlpar_cpu_add() needs to be held until after
dlpar_attach_node() returns. As a result doing repeated cpu add/remove dlpar
operations will eventually result in the following error:
OF: ERROR: Bad of_node_put() on /cpus
CPU: 0 PID: 10896 Comm: drmgr Not tainted 4.13.0-autotest #1
Call Trace:
[c00000026ecdf810] [c00000000278a2a4] dump_stack+0x15c/0x1f8
(unreliable)
[c00000026ecdf850] [c0000000025371a4] of_node_release+0x1a4/0x1c0
[c00000026ecdf8e0] [c0000000027948c8] kobject_put+0x1a8/0x310
[c00000026ecdf960] [c000000002794bdc] kobject_del+0xbc/0xf0
[c00000026ecdf990] [c000000002535ff4] __of_detach_node_sysfs+0x144/0x210
[c00000026ecdf9d0] [c000000002536f70] of_detach_node+0xf0/0x180
[c00000026ecdfa40] [c0000000016ed494] dlpar_detach_node+0xc4/0x120
[c00000026ecdfa80] [c0000000016f47d0] dlpar_cpu_remove+0x280/0x560
[c00000026ecdfb60] [c0000000016f4d9c] dlpar_cpu_release+0xbc/0x1b0
[c00000026ecdfbb0] [c00000000161279c] arch_cpu_release+0x6c/0xb0
[c00000026ecdfbe0] [c00000000218ebf0] cpu_release_store+0xa0/0x100
[c00000026ecdfc20] [c000000002178388] dev_attr_store+0x68/0xa0
[c00000026ecdfc50] [c000000001bfaae8] sysfs_kf_write+0xa8/0xf0
[c00000026ecdfc80] [c000000001bf8a3c] kernfs_fop_write+0x2cc/0x400
[c00000026ecdfce0] [c000000001ad33fc] __vfs_write+0x5c/0x340
[c00000026ecdfd80] [c000000001ad89e8] vfs_write+0x1a8/0x3d0
[c00000026ecdfdd0] [c000000001ad9178] SyS_write+0xa8/0x1a0
[c00000026ecdfe30] [c0000000015eb8e0] system_call+0x58/0x6c
Fix the issue by removing the of_node_put(parent) call from
dlpar_attach_node(), and ensuring that the reference to the parent node
is properly held and released by the caller dlpar_cpu_add().
Cc: stable@vger.kernel.org # v4.13+
Fixes: 215ee763f8cb ("powerpc: pseries: remove dlpar_attach_node dependency on full path")
Signed-off-by: Tyrel Datwyler <redacted>
Reported-by: Abdul Haleem <redacted>
---
arch/powerpc/platforms/pseries/dlpar.c | 1 -
arch/powerpc/platforms/pseries/hotplug-cpu.c | 4 +++-
2 files changed, 3 insertions(+), 2 deletions(-)
From: Michael Ellerman <mpe@ellerman.id.au> Date: 2017-09-21 09:54:55
Hi Tyrel,
Thanks for jumping on this.
Tyrel Datwyler [off-list ref] writes:
Commit 215ee763f8cb ("powerpc: pseries: remove dlpar_attach_node dependency on
full path") reworked dlpar_attach_node() to no longer look up the parent
node "/cpus", but instead to have the parent node passed by the caller in the
function parameter list. As a result dlpar_attach_node() is no longer
responsible for freeing the reference to the parent node. However,
commit 215ee763f8cb failed to remove the of_node_put(parent) call in
dlpar_attach_node(), or to take into account that the reference to the
parent in the caller dlpar_cpu_add() needs to be held until after
dlpar_attach_node() returns. As a result doing repeated cpu add/remove dlpar
operations will eventually result in the following error:
OF: ERROR: Bad of_node_put() on /cpus
CPU: 0 PID: 10896 Comm: drmgr Not tainted 4.13.0-autotest #1
Call Trace:
[c00000026ecdf810] [c00000000278a2a4] dump_stack+0x15c/0x1f8
(unreliable)
[c00000026ecdf850] [c0000000025371a4] of_node_release+0x1a4/0x1c0
[c00000026ecdf8e0] [c0000000027948c8] kobject_put+0x1a8/0x310
[c00000026ecdf960] [c000000002794bdc] kobject_del+0xbc/0xf0
[c00000026ecdf990] [c000000002535ff4] __of_detach_node_sysfs+0x144/0x210
[c00000026ecdf9d0] [c000000002536f70] of_detach_node+0xf0/0x180
[c00000026ecdfa40] [c0000000016ed494] dlpar_detach_node+0xc4/0x120
[c00000026ecdfa80] [c0000000016f47d0] dlpar_cpu_remove+0x280/0x560
[c00000026ecdfb60] [c0000000016f4d9c] dlpar_cpu_release+0xbc/0x1b0
[c00000026ecdfbb0] [c00000000161279c] arch_cpu_release+0x6c/0xb0
[c00000026ecdfbe0] [c00000000218ebf0] cpu_release_store+0xa0/0x100
[c00000026ecdfc20] [c000000002178388] dev_attr_store+0x68/0xa0
[c00000026ecdfc50] [c000000001bfaae8] sysfs_kf_write+0xa8/0xf0
[c00000026ecdfc80] [c000000001bf8a3c] kernfs_fop_write+0x2cc/0x400
[c00000026ecdfce0] [c000000001ad33fc] __vfs_write+0x5c/0x340
[c00000026ecdfd80] [c000000001ad89e8] vfs_write+0x1a8/0x3d0
[c00000026ecdfdd0] [c000000001ad9178] SyS_write+0xa8/0x1a0
[c00000026ecdfe30] [c0000000015eb8e0] system_call+0x58/0x6c
I usually omit all the addresses in the change log, they don't add much
for future readers. I fixed it up myself.
Fix the issue by removing the of_node_put(parent) call from
dlpar_attach_node(), and ensuring that the reference to the parent node
is properly held and released by the caller dlpar_cpu_add().
Fixes: 215ee763f8cb ("powerpc: pseries: remove dlpar_attach_node dependency on full path")
Cc: stable@vger.kernel.org # v4.13+
It doesn't need to go to stable, v4.13 doesn't have that commit, it's
only in v4.14-rc1.
I dropped the stable tag.
From: Michael Ellerman <mpe@ellerman.id.au> Date: 2017-09-21 09:57:14
Tyrel Datwyler [off-list ref] writes:
On 09/20/2017 04:39 AM, Michael Ellerman wrote:
quoted
Rob Herring [off-list ref] writes:
quoted
On Fri, Sep 15, 2017 at 6:04 AM, abdul [off-list ref] wrote:
quoted
Mainline kernel panics during DLPAR CPU add/remove operation.
Machine Type: Power8 PowerVM LPAR
kernel 4.13.0
Did 4.12 work or when was it last working? I'm not seeing anything
recent in the DT code that looks suspicious.
I'm pretty sure it's:
int dlpar_attach_node(struct device_node *dn, struct device_node *parent)
{
int rc;
dn->parent = parent;
rc = of_attach_node(dn);
if (rc) {
printk(KERN_ERR "Failed to add device node %pOF\n", dn);
return rc;
}
of_node_put(dn->parent);
HERE ^^^^^^^^^^
return 0;
}
Prior to 215ee763f8cb ("powerpc: pseries: remove dlpar_attach_node
dependency on full path"), we re-looked up the parent, and got another
reference on it. That meant the put before the return there was correct.
But now it's not because the caller has a reference to parent but it's
not ours to drop.
Testing a fix, will report back.
So, that patch slipped past me. Not only is the parent reference not ours to drop, but
when I went and looked at dlpar_cpu_add() I also noticed that of_node_put() was done on
the parent prior to the call to dlpar_attach_node(). With the addition of "parent" to the
dlpar_attach_node() parameter list dlpar_cpu_add() needs to be fixed up to hold the
"parent" reference until after dlpar_attach_node() returns.
Yep. I wrote the same patch :)
Rob asked me to test it, which I did, but /cpus starts out with an
elevated ref count, so you have to do ~30 (on my system) DLPAR removes
to hit the bug, which I didn't do.
I've updated my test script to do roughly $(nproc) x 10 DLPAR removes,
which is hopefully sufficient to catch these bugs in future.
cheers
Hi Tyrel,
Thanks for jumping on this.
Tyrel Datwyler [off-list ref] writes:
quoted
Commit 215ee763f8cb ("powerpc: pseries: remove dlpar_attach_node dependency on
full path") reworked dlpar_attach_node() to no longer look up the parent
node "/cpus", but instead to have the parent node passed by the caller in the
function parameter list. As a result dlpar_attach_node() is no longer
responsible for freeing the reference to the parent node. However,
commit 215ee763f8cb failed to remove the of_node_put(parent) call in
dlpar_attach_node(), or to take into account that the reference to the
parent in the caller dlpar_cpu_add() needs to be held until after
dlpar_attach_node() returns. As a result doing repeated cpu add/remove dlpar
operations will eventually result in the following error:
OF: ERROR: Bad of_node_put() on /cpus
CPU: 0 PID: 10896 Comm: drmgr Not tainted 4.13.0-autotest #1
Call Trace:
[c00000026ecdf810] [c00000000278a2a4] dump_stack+0x15c/0x1f8
(unreliable)
[c00000026ecdf850] [c0000000025371a4] of_node_release+0x1a4/0x1c0
[c00000026ecdf8e0] [c0000000027948c8] kobject_put+0x1a8/0x310
[c00000026ecdf960] [c000000002794bdc] kobject_del+0xbc/0xf0
[c00000026ecdf990] [c000000002535ff4] __of_detach_node_sysfs+0x144/0x210
[c00000026ecdf9d0] [c000000002536f70] of_detach_node+0xf0/0x180
[c00000026ecdfa40] [c0000000016ed494] dlpar_detach_node+0xc4/0x120
[c00000026ecdfa80] [c0000000016f47d0] dlpar_cpu_remove+0x280/0x560
[c00000026ecdfb60] [c0000000016f4d9c] dlpar_cpu_release+0xbc/0x1b0
[c00000026ecdfbb0] [c00000000161279c] arch_cpu_release+0x6c/0xb0
[c00000026ecdfbe0] [c00000000218ebf0] cpu_release_store+0xa0/0x100
[c00000026ecdfc20] [c000000002178388] dev_attr_store+0x68/0xa0
[c00000026ecdfc50] [c000000001bfaae8] sysfs_kf_write+0xa8/0xf0
[c00000026ecdfc80] [c000000001bf8a3c] kernfs_fop_write+0x2cc/0x400
[c00000026ecdfce0] [c000000001ad33fc] __vfs_write+0x5c/0x340
[c00000026ecdfd80] [c000000001ad89e8] vfs_write+0x1a8/0x3d0
[c00000026ecdfdd0] [c000000001ad9178] SyS_write+0xa8/0x1a0
[c00000026ecdfe30] [c0000000015eb8e0] system_call+0x58/0x6c
I usually omit all the addresses in the change log, they don't add much
for future readers. I fixed it up myself.
Ok, good point. I'll be sure to try and remember for future patches.
quoted
Fix the issue by removing the of_node_put(parent) call from
dlpar_attach_node(), and ensuring that the reference to the parent node
is properly held and released by the caller dlpar_cpu_add().
Fixes: 215ee763f8cb ("powerpc: pseries: remove dlpar_attach_node dependency on full path")
Cc: stable@vger.kernel.org # v4.13+
It doesn't need to go to stable, v4.13 doesn't have that commit, it's
only in v4.14-rc1.
My bad, Abdul mentioned 4.13.0 in his email, but sure enough you are correct.
[root@ltcalpine2-lp2 linux]# git tag --contains 215ee763f8cb
v4.14-rc1
Thanks for catching that.
So, that patch slipped past me. Not only is the parent reference not ours to drop, but
when I went and looked at dlpar_cpu_add() I also noticed that of_node_put() was done on
the parent prior to the call to dlpar_attach_node(). With the addition of "parent" to the
dlpar_attach_node() parameter list dlpar_cpu_add() needs to be fixed up to hold the
"parent" reference until after dlpar_attach_node() returns.
Yep. I wrote the same patch :)
Rob asked me to test it, which I did, but /cpus starts out with an
elevated ref count, so you have to do ~30 (on my system) DLPAR removes
to hit the bug, which I didn't do.
Yeah, there are a lot of things that grab references to /cpus. So, I had a good idea that
I needed to loop a few times adding and removing multiple cpus to trigger the issue. Its
also obvious when using those OF trace points I wrote a while back that refcount for /cpus
is dropping off uncharacteristically in response to symmetrical adds/removes of cpus. I
saw your note about getting that patchset resubmitted. I'll try and get that queued back
up soon.
-Tyrel
I've updated my test script to do roughly $(nproc) x 10 DLPAR removes,
which is hopefully sufficient to catch these bugs in future.
cheers
From: Michael Ellerman <hidden> Date: 2017-09-22 01:03:36
On Wed, 2017-09-20 at 21:02:51 UTC, Tyrel Datwyler wrote:
Commit 215ee763f8cb ("powerpc: pseries: remove dlpar_attach_node dependency on
full path") reworked dlpar_attach_node() to no longer look up the parent
node "/cpus", but instead to have the parent node passed by the caller in the
function parameter list. As a result dlpar_attach_node() is no longer
responsible for freeing the reference to the parent node. However,
commit 215ee763f8cb failed to remove the of_node_put(parent) call in
dlpar_attach_node(), or to take into account that the reference to the
parent in the caller dlpar_cpu_add() needs to be held until after
dlpar_attach_node() returns. As a result doing repeated cpu add/remove dlpar
operations will eventually result in the following error:
OF: ERROR: Bad of_node_put() on /cpus
CPU: 0 PID: 10896 Comm: drmgr Not tainted 4.13.0-autotest #1
Call Trace:
[c00000026ecdf810] [c00000000278a2a4] dump_stack+0x15c/0x1f8
(unreliable)
[c00000026ecdf850] [c0000000025371a4] of_node_release+0x1a4/0x1c0
[c00000026ecdf8e0] [c0000000027948c8] kobject_put+0x1a8/0x310
[c00000026ecdf960] [c000000002794bdc] kobject_del+0xbc/0xf0
[c00000026ecdf990] [c000000002535ff4] __of_detach_node_sysfs+0x144/0x210
[c00000026ecdf9d0] [c000000002536f70] of_detach_node+0xf0/0x180
[c00000026ecdfa40] [c0000000016ed494] dlpar_detach_node+0xc4/0x120
[c00000026ecdfa80] [c0000000016f47d0] dlpar_cpu_remove+0x280/0x560
[c00000026ecdfb60] [c0000000016f4d9c] dlpar_cpu_release+0xbc/0x1b0
[c00000026ecdfbb0] [c00000000161279c] arch_cpu_release+0x6c/0xb0
[c00000026ecdfbe0] [c00000000218ebf0] cpu_release_store+0xa0/0x100
[c00000026ecdfc20] [c000000002178388] dev_attr_store+0x68/0xa0
[c00000026ecdfc50] [c000000001bfaae8] sysfs_kf_write+0xa8/0xf0
[c00000026ecdfc80] [c000000001bf8a3c] kernfs_fop_write+0x2cc/0x400
[c00000026ecdfce0] [c000000001ad33fc] __vfs_write+0x5c/0x340
[c00000026ecdfd80] [c000000001ad89e8] vfs_write+0x1a8/0x3d0
[c00000026ecdfdd0] [c000000001ad9178] SyS_write+0xa8/0x1a0
[c00000026ecdfe30] [c0000000015eb8e0] system_call+0x58/0x6c
Fix the issue by removing the of_node_put(parent) call from
dlpar_attach_node(), and ensuring that the reference to the parent node
is properly held and released by the caller dlpar_cpu_add().
Cc: stable@vger.kernel.org # v4.13+
Fixes: 215ee763f8cb ("powerpc: pseries: remove dlpar_attach_node dependency on full path")
Signed-off-by: Tyrel Datwyler <redacted>
Reported-by: Abdul Haleem <redacted>
From: Abdul Haleem <hidden> Date: 2017-09-22 10:06:06
On Fri, 2017-09-22 at 11:03 +1000, Michael Ellerman wrote:
On Wed, 2017-09-20 at 21:02:51 UTC, Tyrel Datwyler wrote:
quoted
Commit 215ee763f8cb ("powerpc: pseries: remove dlpar_attach_node dependency on
full path") reworked dlpar_attach_node() to no longer look up the parent
node "/cpus", but instead to have the parent node passed by the caller in the
function parameter list. As a result dlpar_attach_node() is no longer
responsible for freeing the reference to the parent node. However,
commit 215ee763f8cb failed to remove the of_node_put(parent) call in
dlpar_attach_node(), or to take into account that the reference to the
parent in the caller dlpar_cpu_add() needs to be held until after
dlpar_attach_node() returns. As a result doing repeated cpu add/remove dlpar
operations will eventually result in the following error:
OF: ERROR: Bad of_node_put() on /cpus
CPU: 0 PID: 10896 Comm: drmgr Not tainted 4.13.0-autotest #1
Call Trace:
[c00000026ecdf810] [c00000000278a2a4] dump_stack+0x15c/0x1f8
(unreliable)
[c00000026ecdf850] [c0000000025371a4] of_node_release+0x1a4/0x1c0
[c00000026ecdf8e0] [c0000000027948c8] kobject_put+0x1a8/0x310
[c00000026ecdf960] [c000000002794bdc] kobject_del+0xbc/0xf0
[c00000026ecdf990] [c000000002535ff4] __of_detach_node_sysfs+0x144/0x210
[c00000026ecdf9d0] [c000000002536f70] of_detach_node+0xf0/0x180
[c00000026ecdfa40] [c0000000016ed494] dlpar_detach_node+0xc4/0x120
[c00000026ecdfa80] [c0000000016f47d0] dlpar_cpu_remove+0x280/0x560
[c00000026ecdfb60] [c0000000016f4d9c] dlpar_cpu_release+0xbc/0x1b0
[c00000026ecdfbb0] [c00000000161279c] arch_cpu_release+0x6c/0xb0
[c00000026ecdfbe0] [c00000000218ebf0] cpu_release_store+0xa0/0x100
[c00000026ecdfc20] [c000000002178388] dev_attr_store+0x68/0xa0
[c00000026ecdfc50] [c000000001bfaae8] sysfs_kf_write+0xa8/0xf0
[c00000026ecdfc80] [c000000001bf8a3c] kernfs_fop_write+0x2cc/0x400
[c00000026ecdfce0] [c000000001ad33fc] __vfs_write+0x5c/0x340
[c00000026ecdfd80] [c000000001ad89e8] vfs_write+0x1a8/0x3d0
[c00000026ecdfdd0] [c000000001ad9178] SyS_write+0xa8/0x1a0
[c00000026ecdfe30] [c0000000015eb8e0] system_call+0x58/0x6c
Fix the issue by removing the of_node_put(parent) call from
dlpar_attach_node(), and ensuring that the reference to the parent node
is properly held and released by the caller dlpar_cpu_add().
Cc: stable@vger.kernel.org # v4.13+
Fixes: 215ee763f8cb ("powerpc: pseries: remove dlpar_attach_node dependency on full path")
Signed-off-by: Tyrel Datwyler <redacted>
Reported-by: Abdul Haleem <redacted>
The patch fixes the problem, No warnings seen for 100 iterations of
DLPAR CPU add/remove operation.
Thanks Tyrel and Michael.
Tested-by: Abdul Haleem <redacted>
--
Regard's
Abdul Haleem
IBM Linux Technology Centre
From: Michael Ellerman <mpe@ellerman.id.au> Date: 2017-09-22 11:59:28
Tyrel Datwyler [off-list ref] writes:
On 09/21/2017 02:57 AM, Michael Ellerman wrote:
quoted
Tyrel Datwyler [off-list ref] writes:
quoted
On 09/20/2017 04:39 AM, Michael Ellerman wrote:
quoted
Rob Herring [off-list ref] writes:
<snip>
quoted
quoted
quoted
Testing a fix, will report back.
So, that patch slipped past me. Not only is the parent reference not ours to drop, but
when I went and looked at dlpar_cpu_add() I also noticed that of_node_put() was done on
the parent prior to the call to dlpar_attach_node(). With the addition of "parent" to the
dlpar_attach_node() parameter list dlpar_cpu_add() needs to be fixed up to hold the
"parent" reference until after dlpar_attach_node() returns.
Yep. I wrote the same patch :)
Rob asked me to test it, which I did, but /cpus starts out with an
elevated ref count, so you have to do ~30 (on my system) DLPAR removes
to hit the bug, which I didn't do.
Yeah, there are a lot of things that grab references to /cpus. So, I had a good idea that
I needed to loop a few times adding and removing multiple cpus to trigger the issue. Its
also obvious when using those OF trace points I wrote a while back that refcount for /cpus
is dropping off uncharacteristically in response to symmetrical adds/removes of cpus. I
saw your note about getting that patchset resubmitted. I'll try and get that queued back
up soon.
Thanks, it'd be great to get it in. I applied it from the list and used
it for testing this.
cheers