* [PATCH v2] leds: lp55xx: avoid dereferencing a stale platform_data pointer
@ 2015-08-04 9:18 Toshi Kikuchi
2015-08-06 5:44 ` Kim, Milo
2015-08-06 8:19 ` Kim, Milo
0 siblings, 2 replies; 6+ messages in thread
From: Toshi Kikuchi @ 2015-08-04 9:18 UTC (permalink / raw)
To: linux-leds; +Cc: milo.kim
lp55xx_of_populate_pdata() allocates platform_data dynamically if DT
is used. The memory area is allocated with devm_kzalloc() so it is
automatically freed if the driver is unloaded. The driver should clear
the pointer to platform_data before it is unloaded, otherwise it will
use the stale pointer accidentally if the driver is reloaded.
Change-Id: Ia097ed83b92cec5a52534a71dcb5322869c2a7b1
Signed-off-by: Toshi Kikuchi <toshik@chromium.org>
---
drivers/leds/leds-lp5521.c | 1 +
drivers/leds/leds-lp5523.c | 1 +
drivers/leds/leds-lp5562.c | 1 +
drivers/leds/leds-lp55xx-common.c | 7 +++++++
drivers/leds/leds-lp55xx-common.h | 2 ++
drivers/leds/leds-lp8501.c | 1 +
6 files changed, 13 insertions(+)
diff --git a/drivers/leds/leds-lp5521.c b/drivers/leds/leds-lp5521.c
index 8ca197a..faba1f9 100644
--- a/drivers/leds/leds-lp5521.c
+++ b/drivers/leds/leds-lp5521.c
@@ -581,6 +581,7 @@ static int lp5521_remove(struct i2c_client *client)
lp55xx_unregister_sysfs(chip);
lp55xx_unregister_leds(led, chip);
lp55xx_deinit_device(chip);
+ lp55xx_of_reset_pdata(&client->dev, client->dev.of_node);
return 0;
}
diff --git a/drivers/leds/leds-lp5523.c b/drivers/leds/leds-lp5523.c
index 584dbbc..4fc2850 100644
--- a/drivers/leds/leds-lp5523.c
+++ b/drivers/leds/leds-lp5523.c
@@ -947,6 +947,7 @@ static int lp5523_remove(struct i2c_client *client)
lp55xx_unregister_sysfs(chip);
lp55xx_unregister_leds(led, chip);
lp55xx_deinit_device(chip);
+ lp55xx_of_reset_pdata(&client->dev, client->dev.of_node);
return 0;
}
diff --git a/drivers/leds/leds-lp5562.c b/drivers/leds/leds-lp5562.c
index ca85724..9d8c250 100644
--- a/drivers/leds/leds-lp5562.c
+++ b/drivers/leds/leds-lp5562.c
@@ -581,6 +581,7 @@ static int lp5562_remove(struct i2c_client *client)
lp55xx_unregister_sysfs(chip);
lp55xx_unregister_leds(led, chip);
lp55xx_deinit_device(chip);
+ lp55xx_of_reset_pdata(&client->dev, client->dev.of_node);
return 0;
}
diff --git a/drivers/leds/leds-lp55xx-common.c b/drivers/leds/leds-lp55xx-common.c
index 96d51e9..ec29163 100644
--- a/drivers/leds/leds-lp55xx-common.c
+++ b/drivers/leds/leds-lp55xx-common.c
@@ -594,6 +594,13 @@ int lp55xx_of_populate_pdata(struct device *dev, struct device_node *np)
}
EXPORT_SYMBOL_GPL(lp55xx_of_populate_pdata);
+void lp55xx_of_reset_pdata(struct device *dev, struct device_node *np)
+{
+ if (np)
+ dev->platform_data = NULL;
+}
+EXPORT_SYMBOL_GPL(lp55xx_of_reset_pdata);
+
MODULE_AUTHOR("Milo Kim <milo.kim@ti.com>");
MODULE_DESCRIPTION("LP55xx Common Driver");
MODULE_LICENSE("GPL");
diff --git a/drivers/leds/leds-lp55xx-common.h b/drivers/leds/leds-lp55xx-common.h
index cceab48..2a6a8fc 100644
--- a/drivers/leds/leds-lp55xx-common.h
+++ b/drivers/leds/leds-lp55xx-common.h
@@ -204,5 +204,7 @@ extern void lp55xx_unregister_sysfs(struct lp55xx_chip *chip);
/* common device tree population function */
extern int lp55xx_of_populate_pdata(struct device *dev,
struct device_node *np);
+extern void lp55xx_of_reset_pdata(struct device *dev,
+ struct device_node *np);
#endif /* _LEDS_LP55XX_COMMON_H */
diff --git a/drivers/leds/leds-lp8501.c b/drivers/leds/leds-lp8501.c
index d3098e3..d75a6f3 100644
--- a/drivers/leds/leds-lp8501.c
+++ b/drivers/leds/leds-lp8501.c
@@ -375,6 +375,7 @@ static int lp8501_remove(struct i2c_client *client)
lp55xx_unregister_sysfs(chip);
lp55xx_unregister_leds(led, chip);
lp55xx_deinit_device(chip);
+ lp55xx_of_reset_pdata(&client->dev, client->dev.of_node);
return 0;
}
--
2.2.1.62.g3f15098
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v2] leds: lp55xx: avoid dereferencing a stale platform_data pointer
2015-08-04 9:18 [PATCH v2] leds: lp55xx: avoid dereferencing a stale platform_data pointer Toshi Kikuchi
@ 2015-08-06 5:44 ` Kim, Milo
2015-08-06 8:19 ` Kim, Milo
1 sibling, 0 replies; 6+ messages in thread
From: Kim, Milo @ 2015-08-06 5:44 UTC (permalink / raw)
To: Toshi Kikuchi, linux-leds
Hi Toshi,
On 8/4/2015 6:18 PM, Toshi Kikuchi wrote:
> lp55xx_of_populate_pdata() allocates platform_data dynamically if DT
> is used. The memory area is allocated with devm_kzalloc() so it is
> automatically freed if the driver is unloaded. The driver should clear
> the pointer to platform_data before it is unloaded, otherwise it will
> use the stale pointer accidentally if the driver is reloaded.
>
> Change-Id: Ia097ed83b92cec5a52534a71dcb5322869c2a7b1
> Signed-off-by: Toshi Kikuchi <toshik@chromium.org>
Acked-by: Milo Kim <milo.kim@ti.com>
> ---
> drivers/leds/leds-lp5521.c | 1 +
> drivers/leds/leds-lp5523.c | 1 +
> drivers/leds/leds-lp5562.c | 1 +
> drivers/leds/leds-lp55xx-common.c | 7 +++++++
> drivers/leds/leds-lp55xx-common.h | 2 ++
> drivers/leds/leds-lp8501.c | 1 +
> 6 files changed, 13 insertions(+)
>
> diff --git a/drivers/leds/leds-lp5521.c b/drivers/leds/leds-lp5521.c
> index 8ca197a..faba1f9 100644
> --- a/drivers/leds/leds-lp5521.c
> +++ b/drivers/leds/leds-lp5521.c
> @@ -581,6 +581,7 @@ static int lp5521_remove(struct i2c_client *client)
> lp55xx_unregister_sysfs(chip);
> lp55xx_unregister_leds(led, chip);
> lp55xx_deinit_device(chip);
> + lp55xx_of_reset_pdata(&client->dev, client->dev.of_node);
>
> return 0;
> }
> diff --git a/drivers/leds/leds-lp5523.c b/drivers/leds/leds-lp5523.c
> index 584dbbc..4fc2850 100644
> --- a/drivers/leds/leds-lp5523.c
> +++ b/drivers/leds/leds-lp5523.c
> @@ -947,6 +947,7 @@ static int lp5523_remove(struct i2c_client *client)
> lp55xx_unregister_sysfs(chip);
> lp55xx_unregister_leds(led, chip);
> lp55xx_deinit_device(chip);
> + lp55xx_of_reset_pdata(&client->dev, client->dev.of_node);
>
> return 0;
> }
> diff --git a/drivers/leds/leds-lp5562.c b/drivers/leds/leds-lp5562.c
> index ca85724..9d8c250 100644
> --- a/drivers/leds/leds-lp5562.c
> +++ b/drivers/leds/leds-lp5562.c
> @@ -581,6 +581,7 @@ static int lp5562_remove(struct i2c_client *client)
> lp55xx_unregister_sysfs(chip);
> lp55xx_unregister_leds(led, chip);
> lp55xx_deinit_device(chip);
> + lp55xx_of_reset_pdata(&client->dev, client->dev.of_node);
>
> return 0;
> }
> diff --git a/drivers/leds/leds-lp55xx-common.c b/drivers/leds/leds-lp55xx-common.c
> index 96d51e9..ec29163 100644
> --- a/drivers/leds/leds-lp55xx-common.c
> +++ b/drivers/leds/leds-lp55xx-common.c
> @@ -594,6 +594,13 @@ int lp55xx_of_populate_pdata(struct device *dev, struct device_node *np)
> }
> EXPORT_SYMBOL_GPL(lp55xx_of_populate_pdata);
>
> +void lp55xx_of_reset_pdata(struct device *dev, struct device_node *np)
> +{
> + if (np)
> + dev->platform_data = NULL;
> +}
> +EXPORT_SYMBOL_GPL(lp55xx_of_reset_pdata);
> +
> MODULE_AUTHOR("Milo Kim <milo.kim@ti.com>");
> MODULE_DESCRIPTION("LP55xx Common Driver");
> MODULE_LICENSE("GPL");
> diff --git a/drivers/leds/leds-lp55xx-common.h b/drivers/leds/leds-lp55xx-common.h
> index cceab48..2a6a8fc 100644
> --- a/drivers/leds/leds-lp55xx-common.h
> +++ b/drivers/leds/leds-lp55xx-common.h
> @@ -204,5 +204,7 @@ extern void lp55xx_unregister_sysfs(struct lp55xx_chip *chip);
> /* common device tree population function */
> extern int lp55xx_of_populate_pdata(struct device *dev,
> struct device_node *np);
> +extern void lp55xx_of_reset_pdata(struct device *dev,
> + struct device_node *np);
>
> #endif /* _LEDS_LP55XX_COMMON_H */
> diff --git a/drivers/leds/leds-lp8501.c b/drivers/leds/leds-lp8501.c
> index d3098e3..d75a6f3 100644
> --- a/drivers/leds/leds-lp8501.c
> +++ b/drivers/leds/leds-lp8501.c
> @@ -375,6 +375,7 @@ static int lp8501_remove(struct i2c_client *client)
> lp55xx_unregister_sysfs(chip);
> lp55xx_unregister_leds(led, chip);
> lp55xx_deinit_device(chip);
> + lp55xx_of_reset_pdata(&client->dev, client->dev.of_node);
>
> return 0;
> }
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] leds: lp55xx: avoid dereferencing a stale platform_data pointer
2015-08-04 9:18 [PATCH v2] leds: lp55xx: avoid dereferencing a stale platform_data pointer Toshi Kikuchi
2015-08-06 5:44 ` Kim, Milo
@ 2015-08-06 8:19 ` Kim, Milo
2015-08-06 8:38 ` Toshi Kikuchi
1 sibling, 1 reply; 6+ messages in thread
From: Kim, Milo @ 2015-08-06 8:19 UTC (permalink / raw)
To: Toshi Kikuchi; +Cc: linux-leds
[-- Attachment #1: Type: text/plain, Size: 2046 bytes --]
Hi Toshi,
On 8/4/2015 6:18 PM, Toshi Kikuchi wrote:
> lp55xx_of_populate_pdata() allocates platform_data dynamically if DT
> is used. The memory area is allocated with devm_kzalloc() so it is
> automatically freed if the driver is unloaded. The driver should clear
> the pointer to platform_data before it is unloaded, otherwise it will
> use the stale pointer accidentally if the driver is reloaded.
Your patch is good but I just found that I should modify
lp55xx_of_populate_pdata().
Allocated platform data should not be pointed to I2C device platform
data. Please see my comments below.
>
> Change-Id: Ia097ed83b92cec5a52534a71dcb5322869c2a7b1
> Signed-off-by: Toshi Kikuchi <toshik@chromium.org>
(snip)
>
> +void lp55xx_of_reset_pdata(struct device *dev, struct device_node *np)
> +{
> + if (np)
> + dev->platform_data = NULL;
> +}
> +EXPORT_SYMBOL_GPL(lp55xx_of_reset_pdata);
The driver should not change original value of platform data,
'client->dev.platform_data'. The platform side configures specific
values and driver just uses it. The values should be kept even after the
driver is attached or detached.
So, I'd like to modify lp55xx_of_populate_pdata() as follows.
1. Return the pointer of lp55xx_platform_data in
lp55xx_of_populate_pdata(). The returned value will point to the private
data structure, 'lp55xx_chip->pdata'
2. Each lp55xx driver checks the pointer and handles error case
Then, original platform data configuration will be kept regardless of
loading or unloading the driver.
The driver uses it if the platform data exists or allocates the memory
and copies them from the DT if it's NULL.
After the driver is unloaded, then allocated memory is freed by device
resource management.
If the driver is loaded again, 'client->dev.platform_data' is same as
initial load, so stale pointer error will not happen.
May I ask you something? Could you check attached patch if available? I
can't test it on real target board at this moment. I would appreciate if
you could test my code.
Best regards,
Milo
[-- Attachment #2: fix-stale-pointer-error.patch --]
[-- Type: text/plain, Size: 5813 bytes --]
diff --git a/drivers/leds/leds-lp5521.c b/drivers/leds/leds-lp5521.c
index 8ca197a..63a9254 100644
--- a/drivers/leds/leds-lp5521.c
+++ b/drivers/leds/leds-lp5521.c
@@ -514,20 +514,19 @@ static int lp5521_probe(struct i2c_client *client,
int ret;
struct lp55xx_chip *chip;
struct lp55xx_led *led;
- struct lp55xx_platform_data *pdata;
+ struct lp55xx_platform_data *pdata = dev_get_platdata(&client->dev);
struct device_node *np = client->dev.of_node;
- if (!dev_get_platdata(&client->dev)) {
+ if (!pdata) {
if (np) {
- ret = lp55xx_of_populate_pdata(&client->dev, np);
- if (ret < 0)
- return ret;
+ pdata = lp55xx_of_populate_pdata(&client->dev, np);
+ if (IS_ERR(pdata))
+ return PTR_ERR(pdata);
} else {
dev_err(&client->dev, "no platform data\n");
return -EINVAL;
}
}
- pdata = dev_get_platdata(&client->dev);
chip = devm_kzalloc(&client->dev, sizeof(*chip), GFP_KERNEL);
if (!chip)
diff --git a/drivers/leds/leds-lp5523.c b/drivers/leds/leds-lp5523.c
index 9e1716f..2351984 100644
--- a/drivers/leds/leds-lp5523.c
+++ b/drivers/leds/leds-lp5523.c
@@ -732,20 +732,19 @@ static int lp5523_probe(struct i2c_client *client,
int ret;
struct lp55xx_chip *chip;
struct lp55xx_led *led;
- struct lp55xx_platform_data *pdata;
+ struct lp55xx_platform_data *pdata = dev_get_platdata(&client->dev);
struct device_node *np = client->dev.of_node;
- if (!dev_get_platdata(&client->dev)) {
+ if (!pdata) {
if (np) {
- ret = lp55xx_of_populate_pdata(&client->dev, np);
- if (ret < 0)
- return ret;
+ pdata = lp55xx_of_populate_pdata(&client->dev, np);
+ if (IS_ERR(pdata))
+ return PTR_ERR(pdata);
} else {
dev_err(&client->dev, "no platform data\n");
return -EINVAL;
}
}
- pdata = dev_get_platdata(&client->dev);
chip = devm_kzalloc(&client->dev, sizeof(*chip), GFP_KERNEL);
if (!chip)
diff --git a/drivers/leds/leds-lp5562.c b/drivers/leds/leds-lp5562.c
index ca85724..0360c59 100644
--- a/drivers/leds/leds-lp5562.c
+++ b/drivers/leds/leds-lp5562.c
@@ -515,20 +515,19 @@ static int lp5562_probe(struct i2c_client *client,
int ret;
struct lp55xx_chip *chip;
struct lp55xx_led *led;
- struct lp55xx_platform_data *pdata;
+ struct lp55xx_platform_data *pdata = dev_get_platdata(&client->dev);
struct device_node *np = client->dev.of_node;
- if (!dev_get_platdata(&client->dev)) {
+ if (!pdata) {
if (np) {
- ret = lp55xx_of_populate_pdata(&client->dev, np);
- if (ret < 0)
- return ret;
+ pdata = lp55xx_of_populate_pdata(&client->dev, np);
+ if (IS_ERR(pdata))
+ return PTR_ERR(pdata);
} else {
dev_err(&client->dev, "no platform data\n");
return -EINVAL;
}
}
- pdata = dev_get_platdata(&client->dev);
chip = devm_kzalloc(&client->dev, sizeof(*chip), GFP_KERNEL);
if (!chip)
diff --git a/drivers/leds/leds-lp55xx-common.c b/drivers/leds/leds-lp55xx-common.c
index 96d51e9..59b7683 100644
--- a/drivers/leds/leds-lp55xx-common.c
+++ b/drivers/leds/leds-lp55xx-common.c
@@ -543,7 +543,8 @@ void lp55xx_unregister_sysfs(struct lp55xx_chip *chip)
}
EXPORT_SYMBOL_GPL(lp55xx_unregister_sysfs);
-int lp55xx_of_populate_pdata(struct device *dev, struct device_node *np)
+struct lp55xx_platform_data *lp55xx_of_populate_pdata(struct device *dev,
+ struct device_node *np)
{
struct device_node *child;
struct lp55xx_platform_data *pdata;
@@ -553,17 +554,17 @@ int lp55xx_of_populate_pdata(struct device *dev, struct device_node *np)
pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
if (!pdata)
- return -ENOMEM;
+ return ERR_PTR(-ENOMEM);
num_channels = of_get_child_count(np);
if (num_channels == 0) {
dev_err(dev, "no LED channels\n");
- return -EINVAL;
+ return ERR_PTR(-EINVAL);
}
cfg = devm_kzalloc(dev, sizeof(*cfg) * num_channels, GFP_KERNEL);
if (!cfg)
- return -ENOMEM;
+ return ERR_PTR(-ENOMEM);
pdata->led_config = &cfg[0];
pdata->num_channels = num_channels;
@@ -588,9 +589,7 @@ int lp55xx_of_populate_pdata(struct device *dev, struct device_node *np)
/* LP8501 specific */
of_property_read_u8(np, "pwr-sel", (u8 *)&pdata->pwr_sel);
- dev->platform_data = pdata;
-
- return 0;
+ return pdata;
}
EXPORT_SYMBOL_GPL(lp55xx_of_populate_pdata);
diff --git a/drivers/leds/leds-lp55xx-common.h b/drivers/leds/leds-lp55xx-common.h
index cceab48..c7f1e61 100644
--- a/drivers/leds/leds-lp55xx-common.h
+++ b/drivers/leds/leds-lp55xx-common.h
@@ -202,7 +202,7 @@ extern int lp55xx_register_sysfs(struct lp55xx_chip *chip);
extern void lp55xx_unregister_sysfs(struct lp55xx_chip *chip);
/* common device tree population function */
-extern int lp55xx_of_populate_pdata(struct device *dev,
- struct device_node *np);
+extern struct lp55xx_platform_data
+*lp55xx_of_populate_pdata(struct device *dev, struct device_node *np);
#endif /* _LEDS_LP55XX_COMMON_H */
diff --git a/drivers/leds/leds-lp8501.c b/drivers/leds/leds-lp8501.c
index d3098e3..3f54f6f 100644
--- a/drivers/leds/leds-lp8501.c
+++ b/drivers/leds/leds-lp8501.c
@@ -308,20 +308,19 @@ static int lp8501_probe(struct i2c_client *client,
int ret;
struct lp55xx_chip *chip;
struct lp55xx_led *led;
- struct lp55xx_platform_data *pdata;
+ struct lp55xx_platform_data *pdata = dev_get_platdata(&client->dev);
struct device_node *np = client->dev.of_node;
- if (!dev_get_platdata(&client->dev)) {
+ if (!pdata) {
if (np) {
- ret = lp55xx_of_populate_pdata(&client->dev, np);
- if (ret < 0)
- return ret;
+ pdata = lp55xx_of_populate_pdata(&client->dev, np);
+ if (IS_ERR(pdata))
+ return PTR_ERR(pdata);
} else {
dev_err(&client->dev, "no platform data\n");
return -EINVAL;
}
}
- pdata = dev_get_platdata(&client->dev);
chip = devm_kzalloc(&client->dev, sizeof(*chip), GFP_KERNEL);
if (!chip)
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v2] leds: lp55xx: avoid dereferencing a stale platform_data pointer
2015-08-06 8:19 ` Kim, Milo
@ 2015-08-06 8:38 ` Toshi Kikuchi
2015-08-06 19:13 ` Toshi Kikuchi
0 siblings, 1 reply; 6+ messages in thread
From: Toshi Kikuchi @ 2015-08-06 8:38 UTC (permalink / raw)
To: Kim, Milo; +Cc: linux-leds
Hi Milo,
On Thu, Aug 6, 2015 at 1:19 AM, Kim, Milo <milo.kim@ti.com> wrote:
> Your patch is good but I just found that I should modify lp55xx_of_populate_pdata().
> Allocated platform data should not be pointed to I2C device platform data.
Yes, I like your new patch better. We don't have to modify the i2c
device platform data in the first place.
> May I ask you something? Could you check attached patch if available? I
> can't test it on real target board at this moment. I would appreciate if you
> could test my code.
Sure. I'll test the patch and let you know the result.
Thanks,
Toshi
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] leds: lp55xx: avoid dereferencing a stale platform_data pointer
2015-08-06 8:38 ` Toshi Kikuchi
@ 2015-08-06 19:13 ` Toshi Kikuchi
2015-08-06 22:39 ` Kim, Milo
0 siblings, 1 reply; 6+ messages in thread
From: Toshi Kikuchi @ 2015-08-06 19:13 UTC (permalink / raw)
To: Kim, Milo; +Cc: linux-leds
Hi Milo,
On Thu, Aug 6, 2015 at 1:38 AM, Toshi Kikuchi <toshik@chromium.org> wrote:
>> May I ask you something? Could you check attached patch if available? I
>> can't test it on real target board at this moment. I would appreciate if you
>> could test my code.
>
> Sure. I'll test the patch and let you know the result.
I just tested it and it works fine.
Toshi
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] leds: lp55xx: avoid dereferencing a stale platform_data pointer
2015-08-06 19:13 ` Toshi Kikuchi
@ 2015-08-06 22:39 ` Kim, Milo
0 siblings, 0 replies; 6+ messages in thread
From: Kim, Milo @ 2015-08-06 22:39 UTC (permalink / raw)
To: Toshi Kikuchi; +Cc: linux-leds
Hi Toshi,
On 8/7/2015 4:13 AM, Toshi Kikuchi wrote:
> Hi Milo,
>
> On Thu, Aug 6, 2015 at 1:38 AM, Toshi Kikuchi <toshik@chromium.org> wrote:
>>> May I ask you something? Could you check attached patch if available? I
>>> can't test it on real target board at this moment. I would appreciate if you
>>> could test my code.
>>
>> Sure. I'll test the patch and let you know the result.
>
> I just tested it and it works fine.
Many thanks for your help. I'll send the patch officially later.
Best regards,
Milo
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2015-08-06 23:46 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2015-08-04 9:18 [PATCH v2] leds: lp55xx: avoid dereferencing a stale platform_data pointer Toshi Kikuchi
2015-08-06 5:44 ` Kim, Milo
2015-08-06 8:19 ` Kim, Milo
2015-08-06 8:38 ` Toshi Kikuchi
2015-08-06 19:13 ` Toshi Kikuchi
2015-08-06 22:39 ` Kim, Milo
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).