On 28/02/2022 11:51, Abel Vesa wrote:
On 22-02-27 14:52:04, Krzysztof Kozlowski 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) (do_one_initcall) from [<c0f012c0>] (kernel_init_freeable+0x3d0/0x4d8) (kernel_init_freeable) from [<c0a7def0>] (kernel_init+0x8/0x114) (kernel_init) from [<c01010b4>] (ret_from_fork+0x14/0x20)Provide a helper which clearly documents the usage of driver_override. This will allow later to reuse the helper and reduce amount of duplicated code.
Convert the platform driver to use new helper and make the driver_override field const char (it is not modified by the core).
Signed-off-by: Krzysztof Kozlowski krzysztof.kozlowski@canonical.com
drivers/base/driver.c | 51 +++++++++++++++++++++++++++++++++ drivers/base/platform.c | 28 +++--------------- include/linux/device/driver.h | 2 ++ include/linux/platform_device.h | 7 ++++- 4 files changed, 63 insertions(+), 25 deletions(-)
diff --git a/drivers/base/driver.c b/drivers/base/driver.c index 8c0d33e182fd..353750b0bbc5 100644 --- a/drivers/base/driver.c +++ b/drivers/base/driver.c @@ -30,6 +30,57 @@ 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)TBH, I think it would make more sense to have this generic driver_set_override receive only the dev and the string. And then, each bus type will have their own implementation that handle things their own way. This would allow all the drivers that will use this to do something like this:
ret = driver_set_override(&pdev->dev, "override_string");
I think it would look more cleaner.
The interface in general is not for the drivers. Drivers use it in exceptions (few cases in entire kernel) but many times they actually do not need to.
Adding a dedicated driver_set_override() brings intention that such usage is welcomed... but it's not. :)
Best regards, Krzysztof