| Message ID | 20220403183758.192236-2-krzysztof.kozlowski@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v6,01/12] driver: platform: Add helper for safer setting of driver_override | expand |
On 04/04/2022 11:16, Andy Shevchenko wrote: > On Sun, Apr 3, 2022 at 9:38 PM Krzysztof Kozlowski > <krzysztof.kozlowski@linaro.org> wrote: >> >> 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). > > ... > >> +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; > > Still not sure if we should distinguish (s == NULL && len == 0) from > (s != NULL && len == 0). > Supplying the latter seems confusing (yes, I see that in the old code). Perhaps > !s test, in case you want to leave it, should be also commented. The old semantics were focused on sysfs usage, so clearing is by passing an empty string. In the case of sysfs empty string is actually "\n". I intend to keep the semantics also for the in-kernel usage and in such case empty string can be also "". If I understand your comment correctly, you propose to change it to NULL for in-kernel usage, but that would change the semantics. > Another approach is to split above to two checks and move !s after !len. I don't follow why... The !override and !s are invalid uses. !len is a valid user for internal callers, just like "\n" is for sysfs. > >> + /* >> + * 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; > > Perhaps explain the case in the comment here? You mean the case we discuss here (to clear override with "")? Sure. > >> + if (!len) { >> + device_lock(dev); >> + old = *override; >> + *override = NULL; > >> + device_unlock(dev); >> + goto out_free; > > You may deduplicate this one, by > > goto out_unlock_free; > > But I understand your intention to keep lock-unlock in one place, so > perhaps dropping that label would be even better in this case and > keeping it Yes, exactly. > > kfree(old); > return 0; > > here instead of goto. Slightly more code, but indeed maybe easier to follow. I'll do like this. Best regards, Krzysztof
On Mon, Apr 4, 2022 at 12:34 PM Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: > On 04/04/2022 11:16, Andy Shevchenko wrote: > > On Sun, Apr 3, 2022 at 9:38 PM Krzysztof Kozlowski > > <krzysztof.kozlowski@linaro.org> wrote: ... > >> +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; > > > > Still not sure if we should distinguish (s == NULL && len == 0) from > > (s != NULL && len == 0). > > Supplying the latter seems confusing (yes, I see that in the old code). Perhaps > > !s test, in case you want to leave it, should be also commented. > > The old semantics were focused on sysfs usage, so clearing is by passing > an empty string. In the case of sysfs empty string is actually "\n". I > intend to keep the semantics also for the in-kernel usage and in such > case empty string can be also "". > > If I understand your comment correctly, you propose to change it to NULL > for in-kernel usage, but that would change the semantics. Yes. It's also possible to have a wrapper for sysfs use. > > Another approach is to split above to two checks and move !s after !len. > > I don't follow why... The !override and !s are invalid uses. !len is a > valid user for internal callers, just like "\n" is for sysfs. I understand but always supplying s maybe an overhead for in-kernel usages. In any case, it's not critical right now, just a remark that it can be modified. > >> + /* > >> + * 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; > > > > Perhaps explain the case in the comment here? > > You mean the case we discuss here (to clear override with "")? Sure. Yep. Before the below check. > >> + if (!len) { > >> + device_lock(dev); > >> + old = *override; > >> + *override = NULL; > > > >> + device_unlock(dev); > >> + goto out_free; > > > > You may deduplicate this one, by > > > > goto out_unlock_free; > > > > But I understand your intention to keep lock-unlock in one place, so > > perhaps dropping that label would be even better in this case and > > keeping it > > Yes, exactly. > > > kfree(old); > > return 0; > > > > here instead of goto. > > Slightly more code, but indeed maybe easier to follow. I'll do like this.
diff --git a/drivers/base/driver.c b/drivers/base/driver.c index 8c0d33e182fd..a6bfe69a8ecc 100644 --- a/drivers/base/driver.c +++ b/drivers/base/driver.c @@ -30,6 +30,71 @@ 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 + * @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) { + device_lock(dev); + old = *override; + *override = NULL; + device_unlock(dev); + goto out_free; + } + + 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 { + kfree(new); + *override = NULL; + } + device_unlock(dev); + +out_free: + 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. diff --git a/drivers/base/platform.c b/drivers/base/platform.c index 8cc272fd5c99..b684157b7f2f 100644 --- a/drivers/base/platform.c +++ b/drivers/base/platform.c @@ -1275,31 +1275,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; } diff --git a/include/linux/device/driver.h b/include/linux/device/driver.h index 15e7c5e15d62..700453017e1c 100644 --- a/include/linux/device/driver.h +++ b/include/linux/device/driver.h @@ -151,6 +151,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, diff --git a/include/linux/platform_device.h b/include/linux/platform_device.h index 7c96f169d274..582d83ed9a91 100644 --- a/include/linux/platform_device.h +++ b/include/linux/platform_device.h @@ -31,7 +31,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;
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). Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> --- drivers/base/driver.c | 65 +++++++++++++++++++++++++++++++++ drivers/base/platform.c | 28 ++------------ include/linux/device/driver.h | 2 + include/linux/platform_device.h | 6 ++- 4 files changed, 76 insertions(+), 25 deletions(-)