of: Deep-copy names of platform devices

PATCH Superseded Stepan Moskovchenko 5 participants 9 messages Aug 13, 2014 – Aug 16, 2014

Subsystems (2)

Arm Qualcomm mailing list Device tree

lore

Share & tools

lore

Stepan Moskovchenko

author

2014-08-13 00:57 UTC #0 lore raw

PATCH 1/1 of: Deep-copy names of platform devices

When we parse the device tree and allocate platform devices, the 'name' of the newly-created platform_device is set to point to the 'name' field of the 'struct device' embedded within the platform_device. This is dangerous, because the name of the 'struct device' is dynamically allocated. Drivers may call dev_set_name() on the device, which will free and reallocate the name of the device, leaving the 'name' of the platform_device pointing to the now-freed memory.

Furthermore, if the dev_set_name() call is made from a driver's probe() function and a subsequent request results in probe deferral, the dangling 'name' reference may lead to the device being re-probed using the wrong driver.

To mitigate these scenarios, we use kstrdup to perform a deep copy of the device name when assigning the name of the platform_device, so that the platform_device name is unaffected by any calls to dev_set_name() that might made by drivers to rename the embedded 'struct device'.

Signed-off-by Stepan Moskovchenko <[email protected]>

---
This is technically a 'v2' patch, but it looks like I used an old version of MAINTAINERS, so I'll just re-send this as a new patch to avoid confusion for people who missed the original.

I suppose creating a 'pdev_set_name' API may seem like another possibility, but I feel that dev.name and pdev.name have two different meanings. One is used for device/driver binding purposes, whereas the other serves a more general identification purpose, and is used for things like sysfs. Drivers might want to change dev.name while leaving the pdev.name alone. I guess yet another possibility would be to prohibit calling dev_set_name() on devices created from device tree, but a driver does not necessarily know how a given platform_device was allocated.

Steve

 drivers/of/device.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

drivers/of/device.c +2 -1

index f685e55..3e116f6 100644
@@ -54,7 +54,7 @@ int of_device_add(struct platform_device *ofdev)
 
 	/* name and id have to be set so that the platform bus doesn't get
 	 * confused on matching */
-	ofdev->name = dev_name(&ofdev->dev);
+	ofdev->name = kstrdup(dev_name(&ofdev->dev), GFP_KERNEL);
 	ofdev->id = -1;
 
 	/* device_add will assume that this device is on the same node as

@@ -76,6 +76,7 @@ EXPORT_SYMBOL(of_device_register);
 void of_device_unregister(struct platform_device *ofdev)
 {
 	device_unregister(&ofdev->dev);
+	kfree(ofdev->name);
 }
 EXPORT_SYMBOL(of_device_unregister);

--
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation

Stephen Boyd

2014-08-13 01:46 UTC #1 lore raw

Quoted 14 lines · Stepan Moskovchenko

drivers/of/device.c +1 -1

index f685e55..3e116f6 100644
@@ -54,7 +54,7 @@ int of_device_add(struct platform_device *ofdev)
 
 	/* name and id have to be set so that the platform bus doesn't get
 	 * confused on matching */
-	ofdev->name = dev_name(&ofdev->dev);
+	ofdev->name = kstrdup(dev_name(&ofdev->dev), GFP_KERNEL);
 	ofdev->id = -1;
 
 	/* device_add will assume that this device is on the same node as

drivers/of/device.c EXPORT_SYMBOL(of_device_register); · line 79

Quoted 4 lines

drivers/of/device.c +1 -0

@@ -76,6 +76,7 @@ EXPORT_SYMBOL(of_device_register);
 void of_device_unregister(struct platform_device *ofdev)
 {
 	device_unregister(&ofdev->dev);
+	kfree(ofdev->name);

This probably ought to be swapped because we don't know if ofdev isn't pointing to freed memory after device_unregister().

Signature · 2 lines

Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

Stepan Moskovchenko

author

2014-08-13 02:30 UTC #2 lore raw

PATCH v2 1/1 of: Deep-copy names of platform devices

Signed-off-by Stepan Moskovchenko <[email protected]>

---
* v2 - swap cleanup order

 drivers/of/device.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

drivers/of/device.c +2 -1

index f685e55..e9beae6 100644
@@ -54,7 +54,7 @@ int of_device_add(struct platform_device *ofdev)
 
 	/* name and id have to be set so that the platform bus doesn't get
 	 * confused on matching */
-	ofdev->name = dev_name(&ofdev->dev);
+	ofdev->name = kstrdup(dev_name(&ofdev->dev), GFP_KERNEL);
 	ofdev->id = -1;
 
 	/* device_add will assume that this device is on the same node as

@@ -75,6 +75,7 @@ EXPORT_SYMBOL(of_device_register);
 
 void of_device_unregister(struct platform_device *ofdev)
 {
+	kfree(ofdev->name);
 	device_unregister(&ofdev->dev);
 }
 EXPORT_SYMBOL(of_device_unregister);

--
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation

Rob Herring

2014-08-15 16:38 UTC #3 lore raw

Adding Greg...

Quoted 10 lines · Stepan Moskovchenko

When we parse the device tree and allocate platform devices, the 'name' of the newly-created platform_device is set to point to the 'name' field of the 'struct device' embedded within the platform_device. This is dangerous, because the name of the 'struct device' is dynamically allocated. Drivers may call dev_set_name() on the device, which will free and reallocate the name of the device, leaving the 'name' of the platform_device pointing to the now-freed memory.

Quoted 4 lines

Furthermore, if the dev_set_name() call is made from a driver's probe() function and a subsequent request results in probe deferral, the dangling 'name' reference may lead to the device being re-probed using the wrong driver.

This seems wrong. I don't think we want drivers to change their own device's name. The name is not supposed to change after registration.

Rob

Quoted 38 lines

To mitigate these scenarios, we use kstrdup to perform a deep copy of the device name when assigning the name of the platform_device, so that the platform_device name is unaffected by any calls to dev_set_name() that might made by drivers to rename the embedded 'struct device'.
Signed-off-by Stepan Moskovchenko <[email protected]>
---
* v2 - swap cleanup order
 drivers/of/device.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
drivers/of/device.c +2 -2
index f685e55..e9beae6 100644
@@ -54,7 +54,7 @@ int of_device_add(struct platform_device *ofdev)
 
        /* name and id have to be set so that the platform bus doesn't get
         * confused on matching */
-       ofdev->name = dev_name(&ofdev->dev);
+       ofdev->name = kstrdup(dev_name(&ofdev->dev), GFP_KERNEL);
        ofdev->id = -1;
 
        /* device_add will assume that this device is on the same node as
@@ -75,6 +75,7 @@ EXPORT_SYMBOL(of_device_register);
 
 void of_device_unregister(struct platform_device *ofdev)
 {
+       kfree(ofdev->name);
        device_unregister(&ofdev->dev);
 }
 EXPORT_SYMBOL(of_device_unregister);
--
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation

Greg Kroah-Hartman

2014-08-16 04:19 UTC #4 lore raw

Quoted 14 lines · Rob Herring

Adding Greg...
Quoted 10 lines · Stepan Moskovchenko
When we parse the device tree and allocate platform devices, the 'name' of the newly-created platform_device is set to point to the 'name' field of the 'struct device' embedded within the platform_device. This is dangerous, because the name of the 'struct device' is dynamically allocated. Drivers may call dev_set_name() on the device, which will free and reallocate the name of the device, leaving the 'name' of the platform_device pointing to the now-freed memory.

Quoted 7 lines

Quoted 4 lines
Furthermore, if the dev_set_name() call is made from a driver's probe() function and a subsequent request results in probe deferral, the dangling 'name' reference may lead to the device being re-probed using the wrong driver.
This seems wrong. I don't think we want drivers to change their own device's name. The name is not supposed to change after registration.

That is correct. Well, you can change a name, using device_rename(), but that's the only way to do it, not through dev_set_name(), as is pointed out here, that will cause problems.

So I don't think this patch is needed at all.

thanks,

greg k-h

Grant Likely

2014-08-16 18:29 UTC #5 lore raw

In reply to Rob Herring

Quoted 14 lines · Rob Herring

Adding Greg...
Quoted 10 lines · Stepan Moskovchenko
When we parse the device tree and allocate platform devices, the 'name' of the newly-created platform_device is set to point to the 'name' field of the 'struct device' embedded within the platform_device. This is dangerous, because the name of the 'struct device' is dynamically allocated. Drivers may call dev_set_name() on the device, which will free and reallocate the name of the device, leaving the 'name' of the platform_device pointing to the now-freed memory.

Quoted 7 lines

Quoted 4 lines
Furthermore, if the dev_set_name() call is made from a driver's probe() function and a subsequent request results in probe deferral, the dangling 'name' reference may lead to the device being re-probed using the wrong driver.
This seems wrong. I don't think we want drivers to change their own device's name. The name is not supposed to change after registration.

Hmmm, you're right. The lifecycle question is theoretically valid but it isn't something that should be done. In this case, it is the drivers/of/platform code that 'owns' the name.

Stepan, under what circumstance would a device call dev_set_name() after registration?

Quoted 41 lines

Rob
Quoted 38 lines
To mitigate these scenarios, we use kstrdup to perform a deep copy of the device name when assigning the name of the platform_device, so that the platform_device name is unaffected by any calls to dev_set_name() that might made by drivers to rename the embedded 'struct device'.
Signed-off-by Stepan Moskovchenko <[email protected]>
---
* v2 - swap cleanup order
 drivers/of/device.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
drivers/of/device.c +2 -2
index f685e55..e9beae6 100644
@@ -54,7 +54,7 @@ int of_device_add(struct platform_device *ofdev)
 
        /* name and id have to be set so that the platform bus doesn't get
         * confused on matching */
-       ofdev->name = dev_name(&ofdev->dev);
+       ofdev->name = kstrdup(dev_name(&ofdev->dev), GFP_KERNEL);
        ofdev->id = -1;
 
        /* device_add will assume that this device is on the same node as
@@ -75,6 +75,7 @@ EXPORT_SYMBOL(of_device_register);
 
 void of_device_unregister(struct platform_device *ofdev)
 {
+       kfree(ofdev->name);
        device_unregister(&ofdev->dev);
 }
 EXPORT_SYMBOL(of_device_unregister);
--
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation

Grant Likely

2014-08-15 10:45 UTC #6 lore raw

In reply to Stephen Boyd

Quoted 13 lines · Stephen Boyd

Quoted 12 lines · Stepan Moskovchenko

drivers/of/device.c +1 -1

index f685e55..3e116f6 100644
@@ -54,7 +54,7 @@ int of_device_add(struct platform_device *ofdev)
 
 	/* name and id have to be set so that the platform bus doesn't get
 	 * confused on matching */
-	ofdev->name = dev_name(&ofdev->dev);
+	ofdev->name = kstrdup(dev_name(&ofdev->dev), GFP_KERNEL);
 	ofdev->id = -1;

Quoted 9 lines

drivers/of/device.c EXPORT_SYMBOL(of_device_register); · line 79
Quoted 6 lines
drivers/of/device.c +1 -0
 	/* device_add will assume that this device is on the same node as
@@ -76,6 +76,7 @@ EXPORT_SYMBOL(of_device_register);
 void of_device_unregister(struct platform_device *ofdev)
 {
 	device_unregister(&ofdev->dev);
+	kfree(ofdev->name);
This probably ought to be swapped because we don't know if ofdev isn't pointing to freed memory after device_unregister().

Actually, the only safe place to free the memory is inside the platform_device release function. platform_device_release() in drivers/base/platform.c. Unfortunately there isn't a good way from that function to figure out if the name has been allocated the 'OF' way.

It is not safe to free it here because there could still be references to the platform_device after device_unregister exits.

Grant Likely

2014-08-15 10:52 UTC #7 lore raw

Quoted 24 lines · Grant Likely

Quoted 13 lines · Stephen Boyd
Quoted 12 lines · Stepan Moskovchenko
drivers/of/device.c +1 -1
index f685e55..3e116f6 100644
@@ -54,7 +54,7 @@ int of_device_add(struct platform_device *ofdev)
 
    /* name and id have to be set so that the platform bus doesn't get
     * confused on matching */
-   ofdev->name = dev_name(&ofdev->dev);
+   ofdev->name = kstrdup(dev_name(&ofdev->dev), GFP_KERNEL);
    ofdev->id = -1;
 
Quoted 9 lines
drivers/of/device.c EXPORT_SYMBOL(of_device_register); · line 79
Quoted 6 lines
drivers/of/device.c +1 -0
    /* device_add will assume that this device is on the same node as
@@ -76,6 +76,7 @@ EXPORT_SYMBOL(of_device_register);
 void of_device_unregister(struct platform_device *ofdev)
 {
    device_unregister(&ofdev->dev);
+   kfree(ofdev->name);
This probably ought to be swapped because we don't know if ofdev isn't pointing to freed memory after device_unregister().

Quoted 7 lines

Actually, the only safe place to free the memory is inside the platform_device release function. platform_device_release() in drivers/base/platform.c. Unfortunately there isn't a good way from that function to figure out if the name has been allocated the 'OF' way.
It is not safe to free it here because there could still be references to the platform_device after device_unregister exits.

We could fix the problem by making platform_device_alloc() also do a kstrdup() when assigning the name. Then the release function can unconditionally free the name field. Care to try that out?

Grant Likely

2014-08-15 11:01 UTC #8 lore raw

Quoted 30 lines · Grant Likely

Quoted 29 lines · Grant Likely
Quoted 13 lines · Stephen Boyd
Quoted 12 lines · Stepan Moskovchenko
drivers/of/device.c +1 -1
index f685e55..3e116f6 100644
@@ -54,7 +54,7 @@ int of_device_add(struct platform_device *ofdev)
 
    /* name and id have to be set so that the platform bus doesn't get
     * confused on matching */
-   ofdev->name = dev_name(&ofdev->dev);
+   ofdev->name = kstrdup(dev_name(&ofdev->dev), GFP_KERNEL);
    ofdev->id = -1;
 
Quoted 9 lines
drivers/of/device.c EXPORT_SYMBOL(of_device_register); · line 79
Quoted 6 lines
drivers/of/device.c +1 -0
    /* device_add will assume that this device is on the same node as
@@ -76,6 +76,7 @@ EXPORT_SYMBOL(of_device_register);
 void of_device_unregister(struct platform_device *ofdev)
 {
    device_unregister(&ofdev->dev);
+   kfree(ofdev->name);
This probably ought to be swapped because we don't know if ofdev isn't pointing to freed memory after device_unregister().
Actually, the only safe place to free the memory is inside the platform_device release function. platform_device_release() in drivers/base/platform.c. Unfortunately there isn't a good way from that function to figure out if the name has been allocated the 'OF' way.

Quoted 6 lines

Quoted 2 lines
It is not safe to free it here because there could still be references to the platform_device after device_unregister exits.
We could fix the problem by making platform_device_alloc() also do a kstrdup() when assigning the name. Then the release function can unconditionally free the name field. Care to try that out?

I don't know how well received that will be though. The vast majority of non-OF callers of platform_device_alloc() pass in a static string that will never disappear.

Another possible solutions: Override the release function for platform devices allocated by of_device_alloc() to free the name field.

g.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html