[v2,4/5] powerpc: pseries: only store the device node basename in full_name

Message ID 20170821151651.25096-5-robh@kernel.org
State New
Headers show
Series
  • [v2,1/5] powerpc: Convert to using %pOF instead of full_name
Related show

Commit Message

Rob Herring Aug. 21, 2017, 3:16 p.m.
With dependencies on full_name containing the entire device node path
removed, stop storing the full_name in nodes created by
dlpar_configure_connector() and pSeries_reconfig_add_node().

Signed-off-by: Rob Herring <robh@kernel.org>

Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Michael Ellerman <mpe@ellerman.id.au>
Cc: linuxppc-dev@lists.ozlabs.org
---
v2:
- Rework dlpar_parse_cc_node() to a single allocation and strcpy instead
  of kasprintf
  
 arch/powerpc/platforms/pseries/dlpar.c    | 30 +++++++-----------------------
 arch/powerpc/platforms/pseries/reconfig.c |  2 +-
 2 files changed, 8 insertions(+), 24 deletions(-)

-- 
2.11.0

Comments

Rob Herring Oct. 3, 2017, 6:44 p.m. | #1
On Tue, Oct 3, 2017 at 4:26 AM, Michael Ellerman <mpe@ellerman.id.au> wrote:
> Hi Rob,

>

> Unfortunately this one has a bug, which only showed up after some stress

> testing.

>

> Rob Herring <robh@kernel.org> writes:

>> With dependencies on full_name containing the entire device node path

>> removed, stop storing the full_name in nodes created by

>> dlpar_configure_connector() and pSeries_reconfig_add_node().

>>

>> Signed-off-by: Rob Herring <robh@kernel.org>

>> Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>

>> Cc: Paul Mackerras <paulus@samba.org>

>> Cc: Michael Ellerman <mpe@ellerman.id.au>

>> Cc: linuxppc-dev@lists.ozlabs.org

>> ---

>> v2:

>> - Rework dlpar_parse_cc_node() to a single allocation and strcpy instead

>>   of kasprintf

>>

>>  arch/powerpc/platforms/pseries/dlpar.c    | 30 +++++++-----------------------

>>  arch/powerpc/platforms/pseries/reconfig.c |  2 +-

>>  2 files changed, 8 insertions(+), 24 deletions(-)

>>

>> diff --git a/arch/powerpc/platforms/pseries/dlpar.c b/arch/powerpc/platforms/pseries/dlpar.c

>> index 783f36364690..31a0615aeea1 100644

>> --- a/arch/powerpc/platforms/pseries/dlpar.c

>> +++ b/arch/powerpc/platforms/pseries/dlpar.c

>> @@ -75,28 +75,17 @@ static struct property *dlpar_parse_cc_property(struct cc_workarea *ccwa)

>>       return prop;

>>  }

>>

>> -static struct device_node *dlpar_parse_cc_node(struct cc_workarea *ccwa,

>> -                                            const char *path)

>> +static struct device_node *(dlpar_parse_cc_node)(struct cc_workarea *ccwa)

>>  {

>>       struct device_node *dn;

>> -     char *name;

>> -

>> -     /* If parent node path is "/" advance path to NULL terminator to

>> -      * prevent double leading slashs in full_name.

>> -      */

>> -     if (!path[1])

>> -             path++;

>> +     const char *name = (const char *)ccwa + be32_to_cpu(ccwa->name_offset);

>>

>> -     dn = kzalloc(sizeof(*dn), GFP_KERNEL);

>> +     dn = kzalloc(sizeof(*dn) + strlen(name) + 1, GFP_KERNEL);

>>       if (!dn)

>>               return NULL;

>>

>> -     name = (char *)ccwa + be32_to_cpu(ccwa->name_offset);

>> -     dn->full_name = kasprintf(GFP_KERNEL, "%s/%s", path, name);

>> -     if (!dn->full_name) {

>> -             kfree(dn);

>> -             return NULL;

>> -     }

>> +     dn->full_name = (char *)(dn + 1);

>> +     strcpy((char *)dn->full_name, name);

>

> Allocating the full_name on the tail of the struct this way breaks the

> call to kfree(node->full_name) in of_node_release().

>

> eg:

>

>

> [  322.543581] Freeing node->full_name c0000007f4ed4090

> [  322.543583] =============================================================================

> [  322.543586] BUG kmalloc-192 (Tainted: G    B          ): Invalid object pointer 0xc0000007f4ed4090

> [  322.543588] -----------------------------------------------------------------------------

>

> [  322.543592] INFO: Slab 0xf000000001fd3b40 objects=292 used=72 fp=0xc0000007f4ed41a8 flags=0x13ffff800000101

> [  322.543595] CPU: 40 PID: 5 Comm: kworker/u192:0 Tainted: G    B           4.14.0-rc2-gcc-6.3.1-next-20170929-dirty #429

> [  322.543600] Workqueue: pseries hotplug workque pseries_hp_work_fn

> [  322.543602] Call Trace:

> [  322.543605] [c0000003f769f660] [c000000000a6b710] dump_stack+0xb0/0xf0 (unreliable)

> [  322.543609] [c0000003f769f6a0] [c00000000032cd5c] slab_err+0x9c/0xc0

> [  322.543612] [c0000003f769f790] [c00000000033440c] free_debug_processing+0x19c/0x490

> [  322.543615] [c0000003f769f860] [c0000000003349fc] __slab_free+0x2fc/0x4b0

> [  322.543619] [c0000003f769f960] [c0000000008f2484] of_node_release+0xe4/0x1a0

> [  322.543622] [c0000003f769fa00] [c000000000a71c04] kobject_put+0xd4/0x160

> [  322.543625] [c0000003f769fa80] [c0000000008f10c4] of_node_put+0x34/0x50

> [  322.543628] [c0000003f769fab0] [c0000000000c0248] dlpar_cpu_remove_by_index+0x108/0x130

> [  322.543631] [c0000003f769fb40] [c0000000000c166c] dlpar_cpu+0x27c/0x510

> [  322.543634] [c0000003f769fbf0] [c0000000000bb2d0] handle_dlpar_errorlog+0xc0/0x160

> [  322.543638] [c0000003f769fc60] [c0000000000bb3ac] pseries_hp_work_fn+0x3c/0xa0

> [  322.543641] [c0000003f769fc90] [c00000000012200c] process_one_work+0x2bc/0x560

> [  322.543645] [c0000003f769fd20] [c000000000122348] worker_thread+0x98/0x5d0

> [  322.543648] [c0000003f769fdc0] [c00000000012b4e4] kthread+0x164/0x1b0

> [  322.543651] [c0000003f769fe30] [c00000000000bae0] ret_from_kernel_thread+0x5c/0x7c

>

>

> The obvious fix is just to allocate it separately as before, eg ~=:


Yes, I'll go back to doing 2 allocs like v1, but using kstrdup as was
also pointed out.

Rob

Patch

diff --git a/arch/powerpc/platforms/pseries/dlpar.c b/arch/powerpc/platforms/pseries/dlpar.c
index 783f36364690..31a0615aeea1 100644
--- a/arch/powerpc/platforms/pseries/dlpar.c
+++ b/arch/powerpc/platforms/pseries/dlpar.c
@@ -75,28 +75,17 @@  static struct property *dlpar_parse_cc_property(struct cc_workarea *ccwa)
 	return prop;
 }
 
-static struct device_node *dlpar_parse_cc_node(struct cc_workarea *ccwa,
-					       const char *path)
+static struct device_node *(dlpar_parse_cc_node)(struct cc_workarea *ccwa)
 {
 	struct device_node *dn;
-	char *name;
-
-	/* If parent node path is "/" advance path to NULL terminator to
-	 * prevent double leading slashs in full_name.
-	 */
-	if (!path[1])
-		path++;
+	const char *name = (const char *)ccwa + be32_to_cpu(ccwa->name_offset);
 
-	dn = kzalloc(sizeof(*dn), GFP_KERNEL);
+	dn = kzalloc(sizeof(*dn) + strlen(name) + 1, GFP_KERNEL);
 	if (!dn)
 		return NULL;
 
-	name = (char *)ccwa + be32_to_cpu(ccwa->name_offset);
-	dn->full_name = kasprintf(GFP_KERNEL, "%s/%s", path, name);
-	if (!dn->full_name) {
-		kfree(dn);
-		return NULL;
-	}
+	dn->full_name = (char *)(dn + 1);
+	strcpy((char *)dn->full_name, name);
 
 	of_node_set_flag(dn, OF_DYNAMIC);
 	of_node_init(dn);
@@ -148,7 +137,6 @@  struct device_node *dlpar_configure_connector(__be32 drc_index,
 	struct property *last_property = NULL;
 	struct cc_workarea *ccwa;
 	char *data_buf;
-	const char *parent_path = parent->full_name;
 	int cc_token;
 	int rc = -1;
 
@@ -182,7 +170,7 @@  struct device_node *dlpar_configure_connector(__be32 drc_index,
 			break;
 
 		case NEXT_SIBLING:
-			dn = dlpar_parse_cc_node(ccwa, parent_path);
+			dn = dlpar_parse_cc_node(ccwa);
 			if (!dn)
 				goto cc_error;
 
@@ -192,10 +180,7 @@  struct device_node *dlpar_configure_connector(__be32 drc_index,
 			break;
 
 		case NEXT_CHILD:
-			if (first_dn)
-				parent_path = last_dn->full_name;
-
-			dn = dlpar_parse_cc_node(ccwa, parent_path);
+			dn = dlpar_parse_cc_node(ccwa);
 			if (!dn)
 				goto cc_error;
 
@@ -226,7 +211,6 @@  struct device_node *dlpar_configure_connector(__be32 drc_index,
 
 		case PREV_PARENT:
 			last_dn = last_dn->parent;
-			parent_path = last_dn->parent->full_name;
 			break;
 
 		case CALL_AGAIN:
diff --git a/arch/powerpc/platforms/pseries/reconfig.c b/arch/powerpc/platforms/pseries/reconfig.c
index 296c188fd5ca..f24d8159c9e1 100644
--- a/arch/powerpc/platforms/pseries/reconfig.c
+++ b/arch/powerpc/platforms/pseries/reconfig.c
@@ -33,7 +33,7 @@  static int pSeries_reconfig_add_node(const char *path, struct property *proplist
 	if (!np)
 		goto out_err;
 
-	np->full_name = kstrdup(path, GFP_KERNEL);
+	np->full_name = kstrdup(kbasename(path), GFP_KERNEL);
 	if (!np->full_name)
 		goto out_err;