| Message ID | 20220419113435.246203-1-krzysztof.kozlowski@linaro.org |
|---|---|
| Headers | show |
| Series | Fix broken usage of driver_override (and kfree of static memory) | expand |
On 19/04/2022 13:34, Krzysztof Kozlowski wrote: Hi Greg, Rafael, The patchset was for some time on the lists, got some reviews, some changes/feedback which I hope I applied/responded. Entire set depends on the driver core changes, so maybe you could pick up everything via drivers core tree? > Dependencies (and stable): > ========================== > 1. All patches, including last three fixes, depend on the first patch > introducing the helper. > 2. The last three commits - fixes - are probably not backportable > directly, because of this dependency. I don't know how to express > this dependency here, since stable-kernel-rules.rst mentions only commits as > possible dependencies. Best regards, Krzysztof
On Tue, Apr 19, 2022 at 1:34 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). > > Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> > --- > drivers/base/driver.c | 69 +++++++++++++++++++++++++++++++++ > drivers/base/platform.c | 28 ++----------- > include/linux/device/driver.h | 2 + > include/linux/platform_device.h | 6 ++- > 4 files changed, 80 insertions(+), 25 deletions(-) > > diff --git a/drivers/base/driver.c b/drivers/base/driver.c > index 8c0d33e182fd..1b9d47b10bd0 100644 > --- a/drivers/base/driver.c > +++ b/drivers/base/driver.c > @@ -30,6 +30,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. I would stick to one-line description here and possibly expand them in the body of the comment. Regardless, I think that the series is an improvement, so please feel free to add Reviewed-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> to this patch and Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> to the other patches in the series. > + * @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. > 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; > -- > 2.32.0 >
On Wed, Apr 20, 2022 at 11:20:06AM +0200, Krzysztof Kozlowski wrote: > On 19/04/2022 13:34, Krzysztof Kozlowski wrote: > > Hi Greg, Rafael, > > The patchset was for some time on the lists, got some reviews, some > changes/feedback which I hope I applied/responded. > > Entire set depends on the driver core changes, so maybe you could pick > up everything via drivers core tree? Ok, will do, thanks. greg k-h
Hi, This is a continuation of my old patchset from 2019. [1] Back then, few drivers set driver_override wrong. I fixed Exynos in a different way after discussions. QCOM NGD was not fixed and a new user appeared - IMX SCU. It seems "char *" in driver_override looks too consty, so we tend to make a mistake of storing there string literals. Changes since latest v7 ======================= 1. Patch #1: remove out_free label, document clearing override in kerneldoc and in code-comments (Andy). 2. Patch #12 (rpmsg): do not duplicate string (Biju). Changes since latest v6 ======================= 1. Patch #1: Don't check for !dev and handle len==0 (Andy). 2. New patch #11 (rpmsg): split constifying of local variable to a new patch. Changes since latest v5 ======================= 1. Patch #11 (rpmsg): split from previous patch 11 - easier to understand the need of it. 2. Fix build issue in patch 12 (rpmsg). Changes since latest v4 ======================= 1. Correct commit msgs and comments after Andy's review. 2. Re-order code in new helper (patch #1) (Andy). 3. Add tags. Changes since latest v3 ======================= 1. Wrap comments, extend comment in driver_set_override() about newline. 2. Minor commit msg fixes. 3. Add tags. Changes since latest v2 ======================= 1. Make all driver_override fields as "const char *", just like SPI and VDPA. (Mark) 2. Move "count" check to the new helper and add "count" argument. (Michael) 3. Fix typos in docs, patch subject. Extend doc. (Michael, Bjorn) 4. Compare pointers to reduce number of string readings in the helper. 5. Fix clk-imx return value. Changes since latest v1 (not the old 2019 solution): ==================================================== https://lore.kernel.org/all/708eabb1-7b35-d525-d4c3-451d4a3de84f@rasmusvillemoes.dk/ 1. Add helper for setting driver_override. 2. Use the helper. Dependencies (and stable): ========================== 1. All patches, including last three fixes, depend on the first patch introducing the helper. 2. The last three commits - fixes - are probably not backportable directly, because of this dependency. I don't know how to express this dependency here, since stable-kernel-rules.rst mentions only commits as possible dependencies. [1] https://lore.kernel.org/all/1550484960-2392-3-git-send-email-krzk@kernel.org/ Best regards, Krzysztof Krzysztof Kozlowski (12): driver: platform: Add helper for safer setting of driver_override amba: Use driver_set_override() instead of open-coding fsl-mc: Use driver_set_override() instead of open-coding hv: Use driver_set_override() instead of open-coding PCI: Use driver_set_override() instead of open-coding s390/cio: Use driver_set_override() instead of open-coding spi: Use helper for safer setting of driver_override vdpa: Use helper for safer setting of driver_override clk: imx: scu: Fix kfree() of static memory on setting driver_override slimbus: qcom-ngd: Fix kfree() of static memory on setting driver_override rpmsg: Constify local variable in field store macro rpmsg: Fix kfree() of static memory on setting driver_override drivers/amba/bus.c | 28 ++----------- drivers/base/driver.c | 69 +++++++++++++++++++++++++++++++++ drivers/base/platform.c | 28 ++----------- drivers/bus/fsl-mc/fsl-mc-bus.c | 25 ++---------- drivers/clk/imx/clk-scu.c | 7 +++- drivers/hv/vmbus_drv.c | 28 ++----------- drivers/pci/pci-sysfs.c | 28 ++----------- drivers/rpmsg/rpmsg_core.c | 3 +- drivers/rpmsg/rpmsg_internal.h | 13 ++++++- drivers/rpmsg/rpmsg_ns.c | 14 ++++++- drivers/s390/cio/cio.h | 6 ++- drivers/s390/cio/css.c | 28 ++----------- drivers/slimbus/qcom-ngd-ctrl.c | 13 ++++++- drivers/spi/spi.c | 26 ++----------- drivers/vdpa/vdpa.c | 29 ++------------ include/linux/amba/bus.h | 6 ++- include/linux/device/driver.h | 2 + include/linux/fsl/mc.h | 6 ++- include/linux/hyperv.h | 6 ++- include/linux/pci.h | 6 ++- include/linux/platform_device.h | 6 ++- include/linux/rpmsg.h | 6 ++- include/linux/spi/spi.h | 2 + include/linux/vdpa.h | 4 +- 24 files changed, 184 insertions(+), 205 deletions(-)