driver: platform: Add helper for safer setting of driver_override
Subsystems (4)
PATCH 1/5 driver: platform: Add helper for safer setting of driver_override
From: Krzysztof Kozlowski <[email protected]>
commit 6c2f421174273de8f83cde4286d1c076d43a2d35 upstream.
Several core drivers and buses expect that driver_override is a dynamically allocated memory thus later they can kfree() it.
However such assumption is not documented, there were in the past and there are already users setting it to a string literal. This leads to kfree() of static memory during device release (e.g. in error paths or during unbind):
kernel BUG at ../mm/slub.c:3960!
Internal error: Oops - BUG: 0 [#1] PREEMPT SMP ARM
...
(kfree) from [<c058da50>] (platform_device_release+0x88/0xb4)
(platform_device_release) from [<c0585be0>] (device_release+0x2c/0x90)
(device_release) from [<c0a69050>] (kobject_put+0xec/0x20c)
(kobject_put) from [<c0f2f120>] (exynos5_clk_probe+0x154/0x18c)
(exynos5_clk_probe) from [<c058de70>] (platform_drv_probe+0x6c/0xa4)
(platform_drv_probe) from [<c058b7ac>] (really_probe+0x280/0x414)
(really_probe) from [<c058baf4>] (driver_probe_device+0x78/0x1c4)
(driver_probe_device) from [<c0589854>] (bus_for_each_drv+0x74/0xb8)
(bus_for_each_drv) from [<c058b48c>] (__device_attach+0xd4/0x16c)
(__device_attach) from [<c058a638>] (bus_probe_device+0x88/0x90)
(bus_probe_device) from [<c05871fc>] (device_add+0x3dc/0x62c)
(device_add) from [<c075ff10>] (of_platform_device_create_pdata+0x94/0xbc)
(of_platform_device_create_pdata) from [<c07600ec>] (of_platform_bus_create+0x1a8/0x4fc)
(of_platform_bus_create) from [<c0760150>] (of_platform_bus_create+0x20c/0x4fc)
(of_platform_bus_create) from [<c07605f0>] (of_platform_populate+0x84/0x118)
(of_platform_populate) from [<c0f3c964>] (of_platform_default_populate_init+0xa0/0xb8)
(of_platform_default_populate_init) from [<c01031f8>] (do_one_initcall+0x8c/0x404)Provide a helper which clearly documents the usage of driver_override. This will allow later to reuse the helper and reduce the amount of duplicated code.
Convert the platform driver to use a new helper and make the driver_override field const char (it is not modified by the core).
--- drivers/base/driver.c | 69 +++++++++++++++++++++++++++++++++ drivers/base/platform.c | 28 ++----------- include/linux/device.h | 2 + include/linux/platform_device.h | 6 ++- 4 files changed, 80 insertions(+), 25 deletions(-)
drivers/base/driver.c +69 -0
@@ -31,6 +31,75 @@ static struct device *next_device(struct klist_iter *i)return dev; } +/** +* driver_set_override() - Helper to set or clear driver override. +* @dev: Device to change +* @override: Address of string to change (e.g. &device->driver_override); +* The contents will be freed and hold newly allocated override. +* @s: NUL-terminated string, new driver name to force a match, pass empty +* string to clear it ("" or "\n", where the latter is only for sysfs +* interface). +* @len: length of @s +* +* Helper to set or clear driver override in a device, intended for the cases +* when the driver_override field is allocated by driver/bus code. +* +* Returns: 0 on success or a negative error code on failure. +*/ +int driver_set_override (struct device *dev,const char **override, +const char *s,size_t len) +{ +const char *new, *old; +char *cp; + +if (!override || !s) +return -EINVAL ; + +/* +* The stored value will be used in sysfs show callback (sysfs_emit()), +* which has a length limit of PAGE_SIZE and adds a trailing newline. +* Thus we can store one character less to avoid truncation during sysfs +* show. +*/ +if (len >= (PAGE_SIZE -1 )) +return -EINVAL ; + +if (!len) { +/* Empty string passed - clear override */ +device_lock (dev); + old = *override; + *override =NULL ; +device_unlock (dev); +kfree (old); + +return 0 ; + } + + cp =strnchr (s, len,'\n' ); +if (cp) + len = cp - s; + + new =kstrndup (s, len,GFP_KERNEL ); +if (!new) +return -ENOMEM ; + +device_lock (dev); + old = *override; +if (cp != s) { + *override = new; + }else { +/* "\n" passed - clear override */ +kfree (new); + *override =NULL ; + } +device_unlock (dev); + +kfree (old); + +return 0 ; +} +EXPORT_SYMBOL_GPL (driver_set_override); +/** * driver_for_each_device - Iterator for devices bound to a driver. * @drv: Driver we're iterating.
drivers/base/platform.c +4 -24
@@ -873,31 +873,11 @@ static ssize_t driver_override_store(struct device *dev, const char *buf, size_t count) {struct platform_device *pdev =to_platform_device (dev); -char *driver_override, *old, *cp; - -/* We need to keep extra room for a newline */ -if (count >= (PAGE_SIZE -1 )) -return -EINVAL ; - - driver_override =kstrndup (buf, count,GFP_KERNEL ); -if (!driver_override) -return -ENOMEM ; - - cp =strchr (driver_override,'\n' ); -if (cp) - *cp ='\0' ; - -device_lock (dev); - old = pdev->driver_override; -if (strlen (driver_override)) { - pdev->driver_override = driver_override; - }else { -kfree (driver_override); - pdev->driver_override =NULL ; - } -device_unlock (dev); +int ret; -kfree (old); + ret =driver_set_override (dev, &pdev->driver_override, buf, count); +if (ret) +return ret;return count; }
include/linux/device.h +2 -0
@@ -319,6 +319,8 @@ extern int __must_check driver_create_file(struct device_driver *driver,extern void driver_remove_file (struct device_driver *driver,const struct driver_attribute *attr); +int driver_set_override (struct device *dev,const char **override, +const char *s,size_t len); extern int __must_check driver_for_each_device(struct device_driver *drv, struct device *start, void *data,
include/linux/platform_device.h +5 -1
@@ -29,7 +29,11 @@ struct platform_device {struct resource *resource;const struct platform_device_id *id_entry; -char *driver_override;/* Driver name to force a match */ +/* +* Driver name to force a match. Do not set directly, because core +* frees it. Use driver_set_override() to set or clear it. +*/ +const char *driver_override;/* MFD cell pointer */ struct mfd_cell *mfd_cell;
Signature · 1 line
2.42.0.820.g83a721a137-goog
PATCH 2/5 rpmsg: Fix kfree() of static memory on setting driver_override
From: Krzysztof Kozlowski <[email protected]>
commit 42cd402b8fd4672b692400fe5f9eecd55d2794ac upstream.
The driver_override field from platform driver should not be initialized from static memory (string literal) because the core later kfree() it, for example when driver_override is set via sysfs.
Use dedicated helper to set driver_override properly.
--- drivers/rpmsg/rpmsg_internal.h | 13 +++++++++++-- include/linux/rpmsg.h | 6 ++++-- 2 files changed, 15 insertions(+), 4 deletions(-)
drivers/rpmsg/rpmsg_internal.h +11 -2
@@ -91,10 +91,19 @@ struct device *rpmsg_find_device(struct device *parent,*/ static inline int rpmsg_chrdev_register_device (struct rpmsg_device *rpdev) { +int ret; +strcpy (rpdev->id.name,"rpmsg_chrdev" ); - rpdev->driver_override ="rpmsg_chrdev" ; + ret =driver_set_override (&rpdev->dev, &rpdev->driver_override, + rpdev->id.name,strlen (rpdev->id.name)); +if (ret) +return ret; + + ret =rpmsg_register_device (rpdev); +if (ret) +kfree (rpdev->driver_override); -return rpmsg_register_device (rpdev); +return ret; }#endif
include/linux/rpmsg.h +4 -2
@@ -66,7 +66,9 @@ struct rpmsg_channel_info {* rpmsg_device - device that belong to the rpmsg bus * @dev: the device struct * @id: device id (used to match between rpmsg drivers and devices) -* @driver_override: driver name to force a match +* @driver_override: driver name to force a match; do not set directly, +* because core frees it; use driver_set_override() to +* set or clear it. * @src: local address * @dst: destination address * @ept: the rpmsg endpoint of this channel
@@ -75,7 +77,7 @@ struct rpmsg_channel_info {struct rpmsg_device {struct device dev;struct rpmsg_device_id id; -char *driver_override; +const char *driver_override;u32 src;u32 dst;struct rpmsg_endpoint *ept;
Signature · 1 line
2.42.0.820.g83a721a137-goog
PATCH 3/5 rpmsg: Fix calling device_lock() on non-initialized device
In reply to Lee JonesFrom: Krzysztof Kozlowski <[email protected]>
commit bb17d110cbf270d5247a6e261c5ad50e362d1675 upstream.
driver_set_override() helper uses device_lock() so it should not be called before rpmsg_register_device() (which calls device_register()). Effect can be seen with CONFIG_DEBUG_MUTEXES:
DEBUG_LOCKS_WARN_ON(lock->magic != lock)
WARNING: CPU: 3 PID: 57 at kernel/locking/mutex.c:582 __mutex_lock+0x1ec/0x430 ... Call trace: __mutex_lock+0x1ec/0x430 mutex_lock_nested+0x44/0x50 driver_set_override+0x124/0x150 qcom_glink_native_probe+0x30c/0x3b0 glink_rpm_probe+0x274/0x350 platform_probe+0x6c/0xe0 really_probe+0x17c/0x3d0 __driver_probe_device+0x114/0x190 driver_probe_device+0x3c/0xf0 ...
Refactor the rpmsg_register_device() function to use two-step device registering (initialization + add) and call driver_set_override() in proper moment.
This moves the code around, so while at it also NULL-ify the rpdev->driver_override in error path to be sure it won't be kfree() second time.
--- drivers/rpmsg/rpmsg_core.c | 33 ++++++++++++++++++++++++++++++--- drivers/rpmsg/rpmsg_internal.h | 14 +------------- include/linux/rpmsg.h | 8 ++++++++ 3 files changed, 39 insertions(+), 16 deletions(-)
drivers/rpmsg/rpmsg_core.c +30 -3
@@ -474,24 +474,51 @@ static struct bus_type rpmsg_bus = { .remove = rpmsg_dev_remove, }; -int rpmsg_register_device (struct rpmsg_device *rpdev) +/* +* A helper for registering rpmsg device with driver override and name. +* Drivers should not be using it, but instead rpmsg_register_device(). +*/ +int rpmsg_register_device_override (struct rpmsg_device *rpdev, +const char *driver_override) {struct device *dev = &rpdev->dev;int ret; +if (driver_override) +strcpy (rpdev->id.name, driver_override); +dev_set_name (&rpdev->dev,"%s.%s.%d.%d" ,dev_name (dev->parent), rpdev->id.name, rpdev->src, rpdev->dst); rpdev->dev.bus = &rpmsg_bus; - ret =device_register (&rpdev->dev); +device_initialize (dev); +if (driver_override) { + ret =driver_set_override (dev, &rpdev->driver_override, + driver_override, +strlen (driver_override)); +if (ret) { +dev_err (dev,"device_set_override failed: %d\n" , ret); +return ret; + } + } + + ret =device_add (dev);if (ret) { -dev_err (dev,"device_register failed: %d\n" , ret); +dev_err (dev,"device_add failed: %d\n" , ret); +kfree (rpdev->driver_override); + rpdev->driver_override =NULL ;put_device (&rpdev->dev); }return ret; } +EXPORT_SYMBOL (rpmsg_register_device_override); + +int rpmsg_register_device (struct rpmsg_device *rpdev) +{ +return rpmsg_register_device_override (rpdev,NULL ); +}EXPORT_SYMBOL (rpmsg_register_device);/*
drivers/rpmsg/rpmsg_internal.h +1 -13
@@ -91,19 +91,7 @@ struct device *rpmsg_find_device(struct device *parent,*/ static inline int rpmsg_chrdev_register_device (struct rpmsg_device *rpdev) { -int ret; - -strcpy (rpdev->id.name,"rpmsg_chrdev" ); - ret =driver_set_override (&rpdev->dev, &rpdev->driver_override, - rpdev->id.name,strlen (rpdev->id.name)); -if (ret) -return ret; - - ret =rpmsg_register_device (rpdev); -if (ret) -kfree (rpdev->driver_override); - -return ret; +return rpmsg_register_device_override (rpdev,"rpmsg_ctrl" ); }#endif
include/linux/rpmsg.h +8 -0
@@ -140,6 +140,8 @@ struct rpmsg_driver {#if IS_ENABLED (CONFIG_RPMSG ) +int rpmsg_register_device_override (struct rpmsg_device *rpdev, +const char *driver_override);int register_rpmsg_device (struct rpmsg_device *dev);void unregister_rpmsg_device (struct rpmsg_device *dev);int __register_rpmsg_driver (struct rpmsg_driver *drv,struct module *owner);
@@ -164,6 +166,12 @@ unsigned int rpmsg_poll(struct rpmsg_endpoint *ept, struct file *filp,#else +static inline int rpmsg_register_device_override (struct rpmsg_device *rpdev, +const char *driver_override) +{ +return -ENXIO ; +} +static inline int register_rpmsg_device (struct rpmsg_device *dev) {return -ENXIO ;
Signature · 1 line
2.42.0.820.g83a721a137-goog
PATCH 4/5 rpmsg: glink: Release driver_override
In reply to Lee JonesFrom: Bjorn Andersson <[email protected]>
commit fb80ef67e8ff6a00d3faad4cb348dafdb8eccfd8 upstream.
Upon termination of the rpmsg_device, driver_override needs to be freed to avoid leaking the potentially assigned string.
--- drivers/rpmsg/qcom_glink_native.c | 1 + 1 file changed, 1 insertion(+)
drivers/rpmsg/qcom_glink_native.c +1 -0
@@ -1354,6 +1354,7 @@ static void qcom_glink_rpdev_release(struct device *dev)struct glink_channel *channel =to_glink_channel (rpdev->ept); channel->rpdev =NULL ; +kfree (rpdev->driver_override);kfree (rpdev); }
Signature · 1 line
2.42.0.820.g83a721a137-goog
PATCH 5/5 rpmsg: Fix possible refcount leak in rpmsg_register_device_override()
In reply to Lee JonesFrom: Hangyu Hua <[email protected]>
commit d7bd416d35121c95fe47330e09a5c04adbc5f928 upstream.
rpmsg_register_device_override need to call put_device to free vch when driver_set_override fails.
Fix this by adding a put_device() to the error path.
--- drivers/rpmsg/rpmsg_core.c | 1 + 1 file changed, 1 insertion(+)
drivers/rpmsg/rpmsg_core.c +1 -0
@@ -499,6 +499,7 @@ int rpmsg_register_device_override(struct rpmsg_device *rpdev, strlen(driver_override));if (ret) {dev_err (dev,"device_set_override failed: %d\n" , ret); +put_device (dev);return ret; } }
Signature · 1 line
2.42.0.820.g83a721a137-goog
Greg Kroah-Hartman
maintainerQuoted 3 lines · Lee Jones
From: Krzysztof Kozlowski <[email protected]>
commit
6c2f421174273de8f83cde4286d1c076d43a2d35upstream.
All now queued up, thanks,
greg k-h