linux-pci.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v3 00/11] Fix broken usage of driver_override (and kfree of static memory)
@ 2022-02-27 13:52 Krzysztof Kozlowski
  2022-02-27 13:52 ` [PATCH v3 01/11] driver: platform: Add helper for safer setting of driver_override Krzysztof Kozlowski
                   ` (10 more replies)
  0 siblings, 11 replies; 26+ messages in thread
From: Krzysztof Kozlowski @ 2022-02-27 13:52 UTC (permalink / raw)
  To: Greg Kroah-Hartman, Rafael J. Wysocki, Stuart Yoder,
	Laurentiu Tudor, Abel Vesa, Shawn Guo, Sascha Hauer,
	Fabio Estevam, K. Y. Srinivasan, Haiyang Zhang,
	Stephen Hemminger, Wei Liu, Dexuan Cui, Bjorn Helgaas,
	Bjorn Andersson, Mathieu Poirier, Vineeth Vijayan,
	Peter Oberparleiter, Heiko Carstens, Vasily Gorbik,
	Alexander Gordeev, Christian Borntraeger, Sven Schnelle,
	Andy Gross, Srinivas Kandagatla, Mark Brown, Michael S. Tsirkin,
	Jason Wang, linux-kernel, linux-clk, NXP Linux Team,
	linux-arm-kernel, linux-hyperv, linux-pci, linux-remoteproc,
	linux-s390, linux-arm-msm, alsa-devel, linux-spi, virtualization,
	Linus Torvalds
  Cc: Rasmus Villemoes, Krzysztof Kozlowski

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 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 (11):
  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: Fix kfree() of static memory on setting driver_override

 drivers/amba/bus.c              | 28 +++---------------
 drivers/base/driver.c           | 51 +++++++++++++++++++++++++++++++++
 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          |  7 ++++-
 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        |  7 ++++-
 include/linux/device/driver.h   |  2 ++
 include/linux/fsl/mc.h          |  6 ++--
 include/linux/hyperv.h          |  7 ++++-
 include/linux/pci.h             |  7 ++++-
 include/linux/platform_device.h |  7 ++++-
 include/linux/rpmsg.h           |  6 ++--
 include/linux/spi/spi.h         |  2 ++
 include/linux/vdpa.h            |  4 ++-
 24 files changed, 171 insertions(+), 205 deletions(-)

-- 
2.32.0


^ permalink raw reply	[flat|nested] 26+ messages in thread

* [PATCH v3 01/11] driver: platform: Add helper for safer setting of driver_override
  2022-02-27 13:52 [PATCH v3 00/11] Fix broken usage of driver_override (and kfree of static memory) Krzysztof Kozlowski
@ 2022-02-27 13:52 ` Krzysztof Kozlowski
  2022-02-28 10:51   ` Abel Vesa
  2022-02-28 20:03   ` Bjorn Helgaas
  2022-02-27 13:52 ` [PATCH v3 02/11] amba: Use driver_set_override() instead of open-coding Krzysztof Kozlowski
                   ` (9 subsequent siblings)
  10 siblings, 2 replies; 26+ messages in thread
From: Krzysztof Kozlowski @ 2022-02-27 13:52 UTC (permalink / raw)
  To: Greg Kroah-Hartman, Rafael J. Wysocki, Stuart Yoder,
	Laurentiu Tudor, Abel Vesa, Shawn Guo, Sascha Hauer,
	Fabio Estevam, K. Y. Srinivasan, Haiyang Zhang,
	Stephen Hemminger, Wei Liu, Dexuan Cui, Bjorn Helgaas,
	Bjorn Andersson, Mathieu Poirier, Vineeth Vijayan,
	Peter Oberparleiter, Heiko Carstens, Vasily Gorbik,
	Alexander Gordeev, Christian Borntraeger, Sven Schnelle,
	Andy Gross, Srinivas Kandagatla, Mark Brown, Michael S. Tsirkin,
	Jason Wang, linux-kernel, linux-clk, NXP Linux Team,
	linux-arm-kernel, linux-hyperv, linux-pci, linux-remoteproc,
	linux-s390, linux-arm-msm, alsa-devel, linux-spi, virtualization,
	Linus Torvalds
  Cc: Rasmus Villemoes, Krzysztof Kozlowski

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)
+{
+	const char *new, *old;
+	char *cp;
+
+	if (!dev || !override || !s)
+		return -EINVAL;
+
+	/* We need to keep extra room for a newline */
+	if (len >= (PAGE_SIZE - 1))
+		return -EINVAL;
+
+	new = kstrndup(s, len, GFP_KERNEL);
+	if (!new)
+		return -ENOMEM;
+
+	cp = strchr(new, '\n');
+	if (cp)
+		*cp = '\0';
+
+	device_lock(dev);
+	old = *override;
+	if (cp != new) {
+		*override = new;
+	} else {
+		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 6cb04ac48bf0..8dd87f44bd74 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..e39963889aa3 100644
--- a/include/linux/platform_device.h
+++ b/include/linux/platform_device.h
@@ -31,7 +31,12 @@ 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


^ permalink raw reply related	[flat|nested] 26+ messages in thread

* [PATCH v3 02/11] amba: Use driver_set_override() instead of open-coding
  2022-02-27 13:52 [PATCH v3 00/11] Fix broken usage of driver_override (and kfree of static memory) Krzysztof Kozlowski
  2022-02-27 13:52 ` [PATCH v3 01/11] driver: platform: Add helper for safer setting of driver_override Krzysztof Kozlowski
@ 2022-02-27 13:52 ` Krzysztof Kozlowski
  2022-02-27 13:52 ` [PATCH v3 03/11] fsl-mc: " Krzysztof Kozlowski
                   ` (8 subsequent siblings)
  10 siblings, 0 replies; 26+ messages in thread
From: Krzysztof Kozlowski @ 2022-02-27 13:52 UTC (permalink / raw)
  To: Greg Kroah-Hartman, Rafael J. Wysocki, Stuart Yoder,
	Laurentiu Tudor, Abel Vesa, Shawn Guo, Sascha Hauer,
	Fabio Estevam, K. Y. Srinivasan, Haiyang Zhang,
	Stephen Hemminger, Wei Liu, Dexuan Cui, Bjorn Helgaas,
	Bjorn Andersson, Mathieu Poirier, Vineeth Vijayan,
	Peter Oberparleiter, Heiko Carstens, Vasily Gorbik,
	Alexander Gordeev, Christian Borntraeger, Sven Schnelle,
	Andy Gross, Srinivas Kandagatla, Mark Brown, Michael S. Tsirkin,
	Jason Wang, linux-kernel, linux-clk, NXP Linux Team,
	linux-arm-kernel, linux-hyperv, linux-pci, linux-remoteproc,
	linux-s390, linux-arm-msm, alsa-devel, linux-spi, virtualization,
	Linus Torvalds
  Cc: Rasmus Villemoes, Krzysztof Kozlowski

Use a helper for seting driver_override to reduce amount of duplicated
code. Make the driver_override field const char, because it is not
modified by the core and it matches other subsystems.

Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com>
---
 drivers/amba/bus.c       | 28 ++++------------------------
 include/linux/amba/bus.h |  7 ++++++-
 2 files changed, 10 insertions(+), 25 deletions(-)

diff --git a/drivers/amba/bus.c b/drivers/amba/bus.c
index e1a5eca3ae3c..9dffa17f50c0 100644
--- a/drivers/amba/bus.c
+++ b/drivers/amba/bus.c
@@ -94,31 +94,11 @@ static ssize_t driver_override_store(struct device *_dev,
 				     const char *buf, size_t count)
 {
 	struct amba_device *dev = to_amba_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 = dev->driver_override;
-	if (strlen(driver_override)) {
-		dev->driver_override = driver_override;
-	} else {
-		kfree(driver_override);
-		dev->driver_override = NULL;
-	}
-	device_unlock(_dev);
+	int ret;
 
-	kfree(old);
+	ret = driver_set_override(_dev, &dev->driver_override, buf, count);
+	if (ret)
+		return ret;
 
 	return count;
 }
diff --git a/include/linux/amba/bus.h b/include/linux/amba/bus.h
index 6c7f47846971..7c703cb7a3cf 100644
--- a/include/linux/amba/bus.h
+++ b/include/linux/amba/bus.h
@@ -70,7 +70,12 @@ struct amba_device {
 	unsigned int		cid;
 	struct amba_cs_uci_id	uci;
 	unsigned int		irq[AMBA_NR_IRQS];
-	char			*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.
+	 */
+	const char		*driver_override;
 };
 
 struct amba_driver {
-- 
2.32.0


^ permalink raw reply related	[flat|nested] 26+ messages in thread

* [PATCH v3 03/11] fsl-mc: Use driver_set_override() instead of open-coding
  2022-02-27 13:52 [PATCH v3 00/11] Fix broken usage of driver_override (and kfree of static memory) Krzysztof Kozlowski
  2022-02-27 13:52 ` [PATCH v3 01/11] driver: platform: Add helper for safer setting of driver_override Krzysztof Kozlowski
  2022-02-27 13:52 ` [PATCH v3 02/11] amba: Use driver_set_override() instead of open-coding Krzysztof Kozlowski
@ 2022-02-27 13:52 ` Krzysztof Kozlowski
  2022-02-27 13:52 ` [PATCH v3 04/11] hv: " Krzysztof Kozlowski
                   ` (7 subsequent siblings)
  10 siblings, 0 replies; 26+ messages in thread
From: Krzysztof Kozlowski @ 2022-02-27 13:52 UTC (permalink / raw)
  To: Greg Kroah-Hartman, Rafael J. Wysocki, Stuart Yoder,
	Laurentiu Tudor, Abel Vesa, Shawn Guo, Sascha Hauer,
	Fabio Estevam, K. Y. Srinivasan, Haiyang Zhang,
	Stephen Hemminger, Wei Liu, Dexuan Cui, Bjorn Helgaas,
	Bjorn Andersson, Mathieu Poirier, Vineeth Vijayan,
	Peter Oberparleiter, Heiko Carstens, Vasily Gorbik,
	Alexander Gordeev, Christian Borntraeger, Sven Schnelle,
	Andy Gross, Srinivas Kandagatla, Mark Brown, Michael S. Tsirkin,
	Jason Wang, linux-kernel, linux-clk, NXP Linux Team,
	linux-arm-kernel, linux-hyperv, linux-pci, linux-remoteproc,
	linux-s390, linux-arm-msm, alsa-devel, linux-spi, virtualization,
	Linus Torvalds
  Cc: Rasmus Villemoes, Krzysztof Kozlowski

Use a helper for seting driver_override to reduce amount of duplicated
code. Make the driver_override field const char, because it is not
modified by the core and it matches other subsystems.

Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com>
---
 drivers/bus/fsl-mc/fsl-mc-bus.c | 25 ++++---------------------
 include/linux/fsl/mc.h          |  6 ++++--
 2 files changed, 8 insertions(+), 23 deletions(-)

diff --git a/drivers/bus/fsl-mc/fsl-mc-bus.c b/drivers/bus/fsl-mc/fsl-mc-bus.c
index 8fd4a356a86e..ba01c7f4de92 100644
--- a/drivers/bus/fsl-mc/fsl-mc-bus.c
+++ b/drivers/bus/fsl-mc/fsl-mc-bus.c
@@ -166,31 +166,14 @@ static ssize_t driver_override_store(struct device *dev,
 				     const char *buf, size_t count)
 {
 	struct fsl_mc_device *mc_dev = to_fsl_mc_device(dev);
-	char *driver_override, *old = mc_dev->driver_override;
-	char *cp;
+	int ret;
 
 	if (WARN_ON(dev->bus != &fsl_mc_bus_type))
 		return -EINVAL;
 
-	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';
-
-	if (strlen(driver_override)) {
-		mc_dev->driver_override = driver_override;
-	} else {
-		kfree(driver_override);
-		mc_dev->driver_override = NULL;
-	}
-
-	kfree(old);
+	ret = driver_set_override(dev, &mc_dev->driver_override, buf, count);
+	if (ret)
+		return ret;
 
 	return count;
 }
diff --git a/include/linux/fsl/mc.h b/include/linux/fsl/mc.h
index 7b6c42bfb660..7a87ab9eba99 100644
--- a/include/linux/fsl/mc.h
+++ b/include/linux/fsl/mc.h
@@ -170,7 +170,9 @@ struct fsl_mc_obj_desc {
  * @regions: pointer to array of MMIO region entries
  * @irqs: pointer to array of pointers to interrupts allocated to this device
  * @resource: generic resource associated with this MC object device, if any.
- * @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.
  *
  * Generic device object for MC object devices that are "attached" to a
  * MC bus.
@@ -204,7 +206,7 @@ struct fsl_mc_device {
 	struct fsl_mc_device_irq **irqs;
 	struct fsl_mc_resource *resource;
 	struct device_link *consumer_link;
-	char   *driver_override;
+	const char *driver_override;
 };
 
 #define to_fsl_mc_device(_dev) \
-- 
2.32.0


^ permalink raw reply related	[flat|nested] 26+ messages in thread

* [PATCH v3 04/11] hv: Use driver_set_override() instead of open-coding
  2022-02-27 13:52 [PATCH v3 00/11] Fix broken usage of driver_override (and kfree of static memory) Krzysztof Kozlowski
                   ` (2 preceding siblings ...)
  2022-02-27 13:52 ` [PATCH v3 03/11] fsl-mc: " Krzysztof Kozlowski
@ 2022-02-27 13:52 ` Krzysztof Kozlowski
  2022-02-27 15:53   ` Michael Kelley (LINUX)
  2022-02-27 13:52 ` [PATCH v3 05/11] PCI: " Krzysztof Kozlowski
                   ` (6 subsequent siblings)
  10 siblings, 1 reply; 26+ messages in thread
From: Krzysztof Kozlowski @ 2022-02-27 13:52 UTC (permalink / raw)
  To: Greg Kroah-Hartman, Rafael J. Wysocki, Stuart Yoder,
	Laurentiu Tudor, Abel Vesa, Shawn Guo, Sascha Hauer,
	Fabio Estevam, K. Y. Srinivasan, Haiyang Zhang,
	Stephen Hemminger, Wei Liu, Dexuan Cui, Bjorn Helgaas,
	Bjorn Andersson, Mathieu Poirier, Vineeth Vijayan,
	Peter Oberparleiter, Heiko Carstens, Vasily Gorbik,
	Alexander Gordeev, Christian Borntraeger, Sven Schnelle,
	Andy Gross, Srinivas Kandagatla, Mark Brown, Michael S. Tsirkin,
	Jason Wang, linux-kernel, linux-clk, NXP Linux Team,
	linux-arm-kernel, linux-hyperv, linux-pci, linux-remoteproc,
	linux-s390, linux-arm-msm, alsa-devel, linux-spi, virtualization,
	Linus Torvalds
  Cc: Rasmus Villemoes, Krzysztof Kozlowski

Use a helper for seting driver_override to reduce amount of duplicated
code. Make the driver_override field const char, because it is not
modified by the core and it matches other subsystems.

Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com>
---
 drivers/hv/vmbus_drv.c | 28 ++++------------------------
 include/linux/hyperv.h |  7 ++++++-
 2 files changed, 10 insertions(+), 25 deletions(-)

diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
index 12a2b37e87f3..a0ff4139c3d2 100644
--- a/drivers/hv/vmbus_drv.c
+++ b/drivers/hv/vmbus_drv.c
@@ -575,31 +575,11 @@ static ssize_t driver_override_store(struct device *dev,
 				     const char *buf, size_t count)
 {
 	struct hv_device *hv_dev = device_to_hv_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 = hv_dev->driver_override;
-	if (strlen(driver_override)) {
-		hv_dev->driver_override = driver_override;
-	} else {
-		kfree(driver_override);
-		hv_dev->driver_override = NULL;
-	}
-	device_unlock(dev);
+	int ret;
 
-	kfree(old);
+	ret = driver_set_override(dev, &hv_dev->driver_override, buf, count);
+	if (ret)
+		return ret;
 
 	return count;
 }
diff --git a/include/linux/hyperv.h b/include/linux/hyperv.h
index fe2e0179ed51..beea11874be2 100644
--- a/include/linux/hyperv.h
+++ b/include/linux/hyperv.h
@@ -1257,7 +1257,12 @@ struct hv_device {
 	u16 device_id;
 
 	struct device device;
-	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;
 
 	struct vmbus_channel *channel;
 	struct kset	     *channels_kset;
-- 
2.32.0


^ permalink raw reply related	[flat|nested] 26+ messages in thread

* [PATCH v3 05/11] PCI: Use driver_set_override() instead of open-coding
  2022-02-27 13:52 [PATCH v3 00/11] Fix broken usage of driver_override (and kfree of static memory) Krzysztof Kozlowski
                   ` (3 preceding siblings ...)
  2022-02-27 13:52 ` [PATCH v3 04/11] hv: " Krzysztof Kozlowski
@ 2022-02-27 13:52 ` Krzysztof Kozlowski
  2022-02-28 20:06   ` Bjorn Helgaas
  2022-02-27 13:52 ` [PATCH v3 06/11] s390: cio: " Krzysztof Kozlowski
                   ` (5 subsequent siblings)
  10 siblings, 1 reply; 26+ messages in thread
From: Krzysztof Kozlowski @ 2022-02-27 13:52 UTC (permalink / raw)
  To: Greg Kroah-Hartman, Rafael J. Wysocki, Stuart Yoder,
	Laurentiu Tudor, Abel Vesa, Shawn Guo, Sascha Hauer,
	Fabio Estevam, K. Y. Srinivasan, Haiyang Zhang,
	Stephen Hemminger, Wei Liu, Dexuan Cui, Bjorn Helgaas,
	Bjorn Andersson, Mathieu Poirier, Vineeth Vijayan,
	Peter Oberparleiter, Heiko Carstens, Vasily Gorbik,
	Alexander Gordeev, Christian Borntraeger, Sven Schnelle,
	Andy Gross, Srinivas Kandagatla, Mark Brown, Michael S. Tsirkin,
	Jason Wang, linux-kernel, linux-clk, NXP Linux Team,
	linux-arm-kernel, linux-hyperv, linux-pci, linux-remoteproc,
	linux-s390, linux-arm-msm, alsa-devel, linux-spi, virtualization,
	Linus Torvalds
  Cc: Rasmus Villemoes, Krzysztof Kozlowski

Use a helper for seting driver_override to reduce amount of duplicated
code. Make the driver_override field const char, because it is not
modified by the core and it matches other subsystems.

Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com>
---
 drivers/pci/pci-sysfs.c | 28 ++++------------------------
 include/linux/pci.h     |  7 ++++++-
 2 files changed, 10 insertions(+), 25 deletions(-)

diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
index 602f0fb0b007..5c42965c32c2 100644
--- a/drivers/pci/pci-sysfs.c
+++ b/drivers/pci/pci-sysfs.c
@@ -567,31 +567,11 @@ static ssize_t driver_override_store(struct device *dev,
 				     const char *buf, size_t count)
 {
 	struct pci_dev *pdev = to_pci_dev(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/pci.h b/include/linux/pci.h
index 8253a5413d7c..5c00a8aebdf9 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -516,7 +516,12 @@ struct pci_dev {
 	u16		acs_cap;	/* ACS Capability offset */
 	phys_addr_t	rom;		/* Physical address if not from BAR */
 	size_t		romlen;		/* Length if not from BAR */
-	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;
 
 	unsigned long	priv_flags;	/* Private flags for the PCI driver */
 
-- 
2.32.0


^ permalink raw reply related	[flat|nested] 26+ messages in thread

* [PATCH v3 06/11] s390: cio: Use driver_set_override() instead of open-coding
  2022-02-27 13:52 [PATCH v3 00/11] Fix broken usage of driver_override (and kfree of static memory) Krzysztof Kozlowski
                   ` (4 preceding siblings ...)
  2022-02-27 13:52 ` [PATCH v3 05/11] PCI: " Krzysztof Kozlowski
@ 2022-02-27 13:52 ` Krzysztof Kozlowski
  2022-03-01 16:01   ` Vineeth Vijayan
  2022-02-27 13:53 ` [PATCH v3 07/11] spi: Use helper for safer setting of driver_override Krzysztof Kozlowski
                   ` (4 subsequent siblings)
  10 siblings, 1 reply; 26+ messages in thread
From: Krzysztof Kozlowski @ 2022-02-27 13:52 UTC (permalink / raw)
  To: Greg Kroah-Hartman, Rafael J. Wysocki, Stuart Yoder,
	Laurentiu Tudor, Abel Vesa, Shawn Guo, Sascha Hauer,
	Fabio Estevam, K. Y. Srinivasan, Haiyang Zhang,
	Stephen Hemminger, Wei Liu, Dexuan Cui, Bjorn Helgaas,
	Bjorn Andersson, Mathieu Poirier, Vineeth Vijayan,
	Peter Oberparleiter, Heiko Carstens, Vasily Gorbik,
	Alexander Gordeev, Christian Borntraeger, Sven Schnelle,
	Andy Gross, Srinivas Kandagatla, Mark Brown, Michael S. Tsirkin,
	Jason Wang, linux-kernel, linux-clk, NXP Linux Team,
	linux-arm-kernel, linux-hyperv, linux-pci, linux-remoteproc,
	linux-s390, linux-arm-msm, alsa-devel, linux-spi, virtualization,
	Linus Torvalds
  Cc: Rasmus Villemoes, Krzysztof Kozlowski

Use a helper for seting driver_override to reduce amount of duplicated
code. Make the driver_override field const char, because it is not
modified by the core and it matches other subsystems.

Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com>
---
 drivers/s390/cio/cio.h |  7 ++++++-
 drivers/s390/cio/css.c | 28 ++++------------------------
 2 files changed, 10 insertions(+), 25 deletions(-)

diff --git a/drivers/s390/cio/cio.h b/drivers/s390/cio/cio.h
index 1cb9daf9c645..e110c10613e8 100644
--- a/drivers/s390/cio/cio.h
+++ b/drivers/s390/cio/cio.h
@@ -103,7 +103,12 @@ struct subchannel {
 	struct work_struct todo_work;
 	struct schib_config config;
 	u64 dma_mask;
-	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;
 } __attribute__ ((aligned(8)));
 
 DECLARE_PER_CPU_ALIGNED(struct irb, cio_irb);
diff --git a/drivers/s390/cio/css.c b/drivers/s390/cio/css.c
index fa8293335077..913b6ddd040b 100644
--- a/drivers/s390/cio/css.c
+++ b/drivers/s390/cio/css.c
@@ -338,31 +338,11 @@ static ssize_t driver_override_store(struct device *dev,
 				     const char *buf, size_t count)
 {
 	struct subchannel *sch = to_subchannel(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 = sch->driver_override;
-	if (strlen(driver_override)) {
-		sch->driver_override = driver_override;
-	} else {
-		kfree(driver_override);
-		sch->driver_override = NULL;
-	}
-	device_unlock(dev);
+	int ret;
 
-	kfree(old);
+	ret = driver_set_override(dev, &sch->driver_override, buf, count);
+	if (ret)
+		return ret;
 
 	return count;
 }
-- 
2.32.0


^ permalink raw reply related	[flat|nested] 26+ messages in thread

* [PATCH v3 07/11] spi: Use helper for safer setting of driver_override
  2022-02-27 13:52 [PATCH v3 00/11] Fix broken usage of driver_override (and kfree of static memory) Krzysztof Kozlowski
                   ` (5 preceding siblings ...)
  2022-02-27 13:52 ` [PATCH v3 06/11] s390: cio: " Krzysztof Kozlowski
@ 2022-02-27 13:53 ` Krzysztof Kozlowski
  2022-02-28 13:07   ` Mark Brown
  2022-02-27 13:53 ` [PATCH v3 08/11] vdpa: " Krzysztof Kozlowski
                   ` (3 subsequent siblings)
  10 siblings, 1 reply; 26+ messages in thread
From: Krzysztof Kozlowski @ 2022-02-27 13:53 UTC (permalink / raw)
  To: Greg Kroah-Hartman, Rafael J. Wysocki, Stuart Yoder,
	Laurentiu Tudor, Abel Vesa, Shawn Guo, Sascha Hauer,
	Fabio Estevam, K. Y. Srinivasan, Haiyang Zhang,
	Stephen Hemminger, Wei Liu, Dexuan Cui, Bjorn Helgaas,
	Bjorn Andersson, Mathieu Poirier, Vineeth Vijayan,
	Peter Oberparleiter, Heiko Carstens, Vasily Gorbik,
	Alexander Gordeev, Christian Borntraeger, Sven Schnelle,
	Andy Gross, Srinivas Kandagatla, Mark Brown, Michael S. Tsirkin,
	Jason Wang, linux-kernel, linux-clk, NXP Linux Team,
	linux-arm-kernel, linux-hyperv, linux-pci, linux-remoteproc,
	linux-s390, linux-arm-msm, alsa-devel, linux-spi, virtualization,
	Linus Torvalds
  Cc: Rasmus Villemoes, Krzysztof Kozlowski

Use a helper for seting driver_override to reduce amount of duplicated
code.

Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com>
---
 drivers/spi/spi.c       | 26 ++++----------------------
 include/linux/spi/spi.h |  2 ++
 2 files changed, 6 insertions(+), 22 deletions(-)

diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index 4599b121d744..9d0b11017741 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -72,29 +72,11 @@ static ssize_t driver_override_store(struct device *dev,
 				     const char *buf, size_t count)
 {
 	struct spi_device *spi = to_spi_device(dev);
-	const char *end = memchr(buf, '\n', count);
-	const size_t len = end ? end - buf : count;
-	const char *driver_override, *old;
-
-	/* We need to keep extra room for a newline when displaying value */
-	if (len >= (PAGE_SIZE - 1))
-		return -EINVAL;
-
-	driver_override = kstrndup(buf, len, GFP_KERNEL);
-	if (!driver_override)
-		return -ENOMEM;
+	int ret;
 
-	device_lock(dev);
-	old = spi->driver_override;
-	if (len) {
-		spi->driver_override = driver_override;
-	} else {
-		/* Empty string, disable driver override */
-		spi->driver_override = NULL;
-		kfree(driver_override);
-	}
-	device_unlock(dev);
-	kfree(old);
+	ret = driver_set_override(dev, &spi->driver_override, buf, count);
+	if (ret)
+		return ret;
 
 	return count;
 }
diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h
index 7ab3fed7b804..f99bbb20dd4b 100644
--- a/include/linux/spi/spi.h
+++ b/include/linux/spi/spi.h
@@ -136,6 +136,8 @@ extern int spi_delay_exec(struct spi_delay *_delay, struct spi_transfer *xfer);
  *	for driver coldplugging, and in uevents used for hotplugging
  * @driver_override: If the name of a driver is written to this attribute, then
  *	the device will bind to the named driver and only the named driver.
+ *	Do not set directly, because core frees it; use driver_set_override() to
+ *	set or clear it.
  * @cs_gpio: LEGACY: gpio number of the chipselect line (optional, -ENOENT when
  *	not using a GPIO line) use cs_gpiod in new drivers by opting in on
  *	the spi_master.
-- 
2.32.0


^ permalink raw reply related	[flat|nested] 26+ messages in thread

* [PATCH v3 08/11] vdpa: Use helper for safer setting of driver_override
  2022-02-27 13:52 [PATCH v3 00/11] Fix broken usage of driver_override (and kfree of static memory) Krzysztof Kozlowski
                   ` (6 preceding siblings ...)
  2022-02-27 13:53 ` [PATCH v3 07/11] spi: Use helper for safer setting of driver_override Krzysztof Kozlowski
@ 2022-02-27 13:53 ` Krzysztof Kozlowski
  2022-02-27 13:53 ` [PATCH v3 09/11] clk: imx: scu: Fix kfree() of static memory on setting driver_override Krzysztof Kozlowski
                   ` (2 subsequent siblings)
  10 siblings, 0 replies; 26+ messages in thread
From: Krzysztof Kozlowski @ 2022-02-27 13:53 UTC (permalink / raw)
  To: Greg Kroah-Hartman, Rafael J. Wysocki, Stuart Yoder,
	Laurentiu Tudor, Abel Vesa, Shawn Guo, Sascha Hauer,
	Fabio Estevam, K. Y. Srinivasan, Haiyang Zhang,
	Stephen Hemminger, Wei Liu, Dexuan Cui, Bjorn Helgaas,
	Bjorn Andersson, Mathieu Poirier, Vineeth Vijayan,
	Peter Oberparleiter, Heiko Carstens, Vasily Gorbik,
	Alexander Gordeev, Christian Borntraeger, Sven Schnelle,
	Andy Gross, Srinivas Kandagatla, Mark Brown, Michael S. Tsirkin,
	Jason Wang, linux-kernel, linux-clk, NXP Linux Team,
	linux-arm-kernel, linux-hyperv, linux-pci, linux-remoteproc,
	linux-s390, linux-arm-msm, alsa-devel, linux-spi, virtualization,
	Linus Torvalds
  Cc: Rasmus Villemoes, Krzysztof Kozlowski

Use a helper for seting driver_override to reduce amount of duplicated
code.

Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com>
---
 drivers/vdpa/vdpa.c  | 29 ++++-------------------------
 include/linux/vdpa.h |  4 +++-
 2 files changed, 7 insertions(+), 26 deletions(-)

diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c
index 9846c9de4bfa..2d924a89ce28 100644
--- a/drivers/vdpa/vdpa.c
+++ b/drivers/vdpa/vdpa.c
@@ -77,32 +77,11 @@ static ssize_t driver_override_store(struct device *dev,
 				     const char *buf, size_t count)
 {
 	struct vdpa_device *vdev = dev_to_vdpa(dev);
-	const char *driver_override, *old;
-	char *cp;
+	int ret;
 
-	/* 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 = vdev->driver_override;
-	if (strlen(driver_override)) {
-		vdev->driver_override = driver_override;
-	} else {
-		kfree(driver_override);
-		vdev->driver_override = NULL;
-	}
-	device_unlock(dev);
-
-	kfree(old);
+	ret = driver_set_override(dev, &vdev->driver_override, buf, count);
+	if (ret)
+		return ret;
 
 	return count;
 }
diff --git a/include/linux/vdpa.h b/include/linux/vdpa.h
index 2de442ececae..89ec4e4d4cdc 100644
--- a/include/linux/vdpa.h
+++ b/include/linux/vdpa.h
@@ -64,7 +64,9 @@ struct vdpa_mgmt_dev;
  * struct vdpa_device - representation of a vDPA device
  * @dev: underlying device
  * @dma_dev: the actual device that is performing DMA
- * @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.
  * @config: the configuration ops for this device.
  * @cf_mutex: Protects get and set access to configuration layout.
  * @index: device index
-- 
2.32.0


^ permalink raw reply related	[flat|nested] 26+ messages in thread

* [PATCH v3 09/11] clk: imx: scu: Fix kfree() of static memory on setting driver_override
  2022-02-27 13:52 [PATCH v3 00/11] Fix broken usage of driver_override (and kfree of static memory) Krzysztof Kozlowski
                   ` (7 preceding siblings ...)
  2022-02-27 13:53 ` [PATCH v3 08/11] vdpa: " Krzysztof Kozlowski
@ 2022-02-27 13:53 ` Krzysztof Kozlowski
  2022-02-28 10:55   ` Abel Vesa
  2022-02-27 13:53 ` [PATCH v3 10/11] slimbus: qcom-ngd: " Krzysztof Kozlowski
  2022-02-27 13:53 ` [PATCH v3 11/11] rpmsg: " Krzysztof Kozlowski
  10 siblings, 1 reply; 26+ messages in thread
From: Krzysztof Kozlowski @ 2022-02-27 13:53 UTC (permalink / raw)
  To: Greg Kroah-Hartman, Rafael J. Wysocki, Stuart Yoder,
	Laurentiu Tudor, Abel Vesa, Shawn Guo, Sascha Hauer,
	Fabio Estevam, K. Y. Srinivasan, Haiyang Zhang,
	Stephen Hemminger, Wei Liu, Dexuan Cui, Bjorn Helgaas,
	Bjorn Andersson, Mathieu Poirier, Vineeth Vijayan,
	Peter Oberparleiter, Heiko Carstens, Vasily Gorbik,
	Alexander Gordeev, Christian Borntraeger, Sven Schnelle,
	Andy Gross, Srinivas Kandagatla, Mark Brown, Michael S. Tsirkin,
	Jason Wang, linux-kernel, linux-clk, NXP Linux Team,
	linux-arm-kernel, linux-hyperv, linux-pci, linux-remoteproc,
	linux-s390, linux-arm-msm, alsa-devel, linux-spi, virtualization,
	Linus Torvalds
  Cc: Rasmus Villemoes, Krzysztof Kozlowski, stable

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.

Fixes: 77d8f3068c63 ("clk: imx: scu: add two cells binding support")
Cc: <stable@vger.kernel.org>
Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com>
---
 drivers/clk/imx/clk-scu.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/clk/imx/clk-scu.c b/drivers/clk/imx/clk-scu.c
index 083da31dc3ea..4b2268b7d0d0 100644
--- a/drivers/clk/imx/clk-scu.c
+++ b/drivers/clk/imx/clk-scu.c
@@ -683,7 +683,12 @@ struct clk_hw *imx_clk_scu_alloc_dev(const char *name,
 		return ERR_PTR(ret);
 	}
 
-	pdev->driver_override = "imx-scu-clk";
+	ret = driver_set_override(&pdev->dev, &pdev->driver_override,
+				  "imx-scu-clk", strlen("imx-scu-clk"));
+	if (ret) {
+		platform_device_put(pdev);
+		return ERR_PTR(ret);
+	}
 
 	ret = imx_clk_scu_attach_pd(&pdev->dev, rsrc_id);
 	if (ret)
-- 
2.32.0


^ permalink raw reply related	[flat|nested] 26+ messages in thread

* [PATCH v3 10/11] slimbus: qcom-ngd: Fix kfree() of static memory on setting driver_override
  2022-02-27 13:52 [PATCH v3 00/11] Fix broken usage of driver_override (and kfree of static memory) Krzysztof Kozlowski
                   ` (8 preceding siblings ...)
  2022-02-27 13:53 ` [PATCH v3 09/11] clk: imx: scu: Fix kfree() of static memory on setting driver_override Krzysztof Kozlowski
@ 2022-02-27 13:53 ` Krzysztof Kozlowski
  2022-03-01 17:28   ` Srinivas Kandagatla
  2022-02-27 13:53 ` [PATCH v3 11/11] rpmsg: " Krzysztof Kozlowski
  10 siblings, 1 reply; 26+ messages in thread
From: Krzysztof Kozlowski @ 2022-02-27 13:53 UTC (permalink / raw)
  To: Greg Kroah-Hartman, Rafael J. Wysocki, Stuart Yoder,
	Laurentiu Tudor, Abel Vesa, Shawn Guo, Sascha Hauer,
	Fabio Estevam, K. Y. Srinivasan, Haiyang Zhang,
	Stephen Hemminger, Wei Liu, Dexuan Cui, Bjorn Helgaas,
	Bjorn Andersson, Mathieu Poirier, Vineeth Vijayan,
	Peter Oberparleiter, Heiko Carstens, Vasily Gorbik,
	Alexander Gordeev, Christian Borntraeger, Sven Schnelle,
	Andy Gross, Srinivas Kandagatla, Mark Brown, Michael S. Tsirkin,
	Jason Wang, linux-kernel, linux-clk, NXP Linux Team,
	linux-arm-kernel, linux-hyperv, linux-pci, linux-remoteproc,
	linux-s390, linux-arm-msm, alsa-devel, linux-spi, virtualization,
	Linus Torvalds
  Cc: Rasmus Villemoes, Krzysztof Kozlowski, stable

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.

Fixes: 917809e2280b ("slimbus: ngd: Add qcom SLIMBus NGD driver")
Cc: <stable@vger.kernel.org>
Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com>
---
 drivers/slimbus/qcom-ngd-ctrl.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/drivers/slimbus/qcom-ngd-ctrl.c b/drivers/slimbus/qcom-ngd-ctrl.c
index 7040293c2ee8..e5d9fdb81eb0 100644
--- a/drivers/slimbus/qcom-ngd-ctrl.c
+++ b/drivers/slimbus/qcom-ngd-ctrl.c
@@ -1434,6 +1434,7 @@ static int of_qcom_slim_ngd_register(struct device *parent,
 	const struct of_device_id *match;
 	struct device_node *node;
 	u32 id;
+	int ret;
 
 	match = of_match_node(qcom_slim_ngd_dt_match, parent->of_node);
 	data = match->data;
@@ -1455,7 +1456,17 @@ static int of_qcom_slim_ngd_register(struct device *parent,
 		}
 		ngd->id = id;
 		ngd->pdev->dev.parent = parent;
-		ngd->pdev->driver_override = QCOM_SLIM_NGD_DRV_NAME;
+
+		ret = driver_set_override(&ngd->pdev->dev,
+					  &ngd->pdev->driver_override,
+					  QCOM_SLIM_NGD_DRV_NAME,
+					  strlen(QCOM_SLIM_NGD_DRV_NAME));
+		if (ret) {
+			platform_device_put(ngd->pdev);
+			kfree(ngd);
+			of_node_put(node);
+			return ret;
+		}
 		ngd->pdev->dev.of_node = node;
 		ctrl->ngd = ngd;
 
-- 
2.32.0


^ permalink raw reply related	[flat|nested] 26+ messages in thread

* [PATCH v3 11/11] rpmsg: Fix kfree() of static memory on setting driver_override
  2022-02-27 13:52 [PATCH v3 00/11] Fix broken usage of driver_override (and kfree of static memory) Krzysztof Kozlowski
                   ` (9 preceding siblings ...)
  2022-02-27 13:53 ` [PATCH v3 10/11] slimbus: qcom-ngd: " Krzysztof Kozlowski
@ 2022-02-27 13:53 ` Krzysztof Kozlowski
  2022-03-11 20:19   ` Bjorn Andersson
  10 siblings, 1 reply; 26+ messages in thread
From: Krzysztof Kozlowski @ 2022-02-27 13:53 UTC (permalink / raw)
  To: Greg Kroah-Hartman, Rafael J. Wysocki, Stuart Yoder,
	Laurentiu Tudor, Abel Vesa, Shawn Guo, Sascha Hauer,
	Fabio Estevam, K. Y. Srinivasan, Haiyang Zhang,
	Stephen Hemminger, Wei Liu, Dexuan Cui, Bjorn Helgaas,
	Bjorn Andersson, Mathieu Poirier, Vineeth Vijayan,
	Peter Oberparleiter, Heiko Carstens, Vasily Gorbik,
	Alexander Gordeev, Christian Borntraeger, Sven Schnelle,
	Andy Gross, Srinivas Kandagatla, Mark Brown, Michael S. Tsirkin,
	Jason Wang, linux-kernel, linux-clk, NXP Linux Team,
	linux-arm-kernel, linux-hyperv, linux-pci, linux-remoteproc,
	linux-s390, linux-arm-msm, alsa-devel, linux-spi, virtualization,
	Linus Torvalds
  Cc: Rasmus Villemoes, Krzysztof Kozlowski, stable

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.

Fixes: 950a7388f02b ("rpmsg: Turn name service into a stand alone driver")
Fixes: c0cdc19f84a4 ("rpmsg: Driver for user space endpoint interface")
Cc: <stable@vger.kernel.org>
Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com>
---
 drivers/rpmsg/rpmsg_core.c     |  3 ++-
 drivers/rpmsg/rpmsg_internal.h | 13 +++++++++++--
 drivers/rpmsg/rpmsg_ns.c       | 14 ++++++++++++--
 include/linux/rpmsg.h          |  6 ++++--
 4 files changed, 29 insertions(+), 7 deletions(-)

diff --git a/drivers/rpmsg/rpmsg_core.c b/drivers/rpmsg/rpmsg_core.c
index d9e612f4f0f2..6e2bf2742973 100644
--- a/drivers/rpmsg/rpmsg_core.c
+++ b/drivers/rpmsg/rpmsg_core.c
@@ -397,7 +397,8 @@ field##_store(struct device *dev, struct device_attribute *attr,	\
 	      const char *buf, size_t sz)				\
 {									\
 	struct rpmsg_device *rpdev = to_rpmsg_device(dev);		\
-	char *new, *old;						\
+	const char *old;						\
+	char *new;							\
 									\
 	new = kstrndup(buf, sz, GFP_KERNEL);				\
 	if (!new)							\
diff --git a/drivers/rpmsg/rpmsg_internal.h b/drivers/rpmsg/rpmsg_internal.h
index b1245d3ed7c6..31345d6e9a7e 100644
--- a/drivers/rpmsg/rpmsg_internal.h
+++ b/drivers/rpmsg/rpmsg_internal.h
@@ -92,10 +92,19 @@ int rpmsg_release_channel(struct rpmsg_device *rpdev,
  */
 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,
+				  "rpmsg_chrdev", strlen("rpmsg_chrdev"));
+	if (ret)
+		return ret;
+
+	ret = rpmsg_register_device(rpdev);
+	if (ret)
+		kfree(rpdev->driver_override);
 
-	return rpmsg_register_device(rpdev);
+	return ret;
 }
 
 #endif
diff --git a/drivers/rpmsg/rpmsg_ns.c b/drivers/rpmsg/rpmsg_ns.c
index 762ff1ae279f..95a51543f5ad 100644
--- a/drivers/rpmsg/rpmsg_ns.c
+++ b/drivers/rpmsg/rpmsg_ns.c
@@ -20,12 +20,22 @@
  */
 int rpmsg_ns_register_device(struct rpmsg_device *rpdev)
 {
+	int ret;
+
 	strcpy(rpdev->id.name, "rpmsg_ns");
-	rpdev->driver_override = "rpmsg_ns";
+	ret = driver_set_override(&rpdev->dev, &rpdev->driver_override,
+				  "rpmsg_ns", strlen("rpmsg_ns"));
+	if (ret)
+		return ret;
+
 	rpdev->src = RPMSG_NS_ADDR;
 	rpdev->dst = RPMSG_NS_ADDR;
 
-	return rpmsg_register_device(rpdev);
+	ret = rpmsg_register_device(rpdev);
+	if (ret)
+		kfree(rpdev->driver_override);
+
+	return ret;
 }
 EXPORT_SYMBOL(rpmsg_ns_register_device);
 
diff --git a/include/linux/rpmsg.h b/include/linux/rpmsg.h
index 02fa9116cd60..20c8cd1cde21 100644
--- a/include/linux/rpmsg.h
+++ b/include/linux/rpmsg.h
@@ -41,7 +41,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
@@ -51,7 +53,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;
-- 
2.32.0


^ permalink raw reply related	[flat|nested] 26+ messages in thread

* RE: [PATCH v3 04/11] hv: Use driver_set_override() instead of open-coding
  2022-02-27 13:52 ` [PATCH v3 04/11] hv: " Krzysztof Kozlowski
@ 2022-02-27 15:53   ` Michael Kelley (LINUX)
  0 siblings, 0 replies; 26+ messages in thread
From: Michael Kelley (LINUX) @ 2022-02-27 15:53 UTC (permalink / raw)
  To: Krzysztof Kozlowski, Greg Kroah-Hartman, Rafael J. Wysocki,
	Stuart Yoder, Laurentiu Tudor, Abel Vesa, Shawn Guo,
	Sascha Hauer, Fabio Estevam, KY Srinivasan, Haiyang Zhang,
	Stephen Hemminger, Wei Liu, Dexuan Cui, Bjorn Helgaas,
	Bjorn Andersson, Mathieu Poirier, Vineeth Vijayan,
	Peter Oberparleiter, Heiko Carstens, Vasily Gorbik,
	Alexander Gordeev, Christian Borntraeger, Sven Schnelle,
	Andy Gross, Srinivas Kandagatla, Mark Brown, Michael S. Tsirkin,
	Jason Wang, linux-kernel, linux-clk, NXP Linux Team,
	linux-arm-kernel, linux-hyperv, linux-pci, linux-remoteproc,
	linux-s390, linux-arm-msm, alsa-devel, linux-spi, virtualization,
	Linus Torvalds
  Cc: Rasmus Villemoes

From: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com> Sent: Sunday, February 27, 2022 5:52 AM
> 
> Use a helper for seting driver_override to reduce amount of duplicated
> code. Make the driver_override field const char, because it is not
> modified by the core and it matches other subsystems.
> 
> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com>
> ---
>  drivers/hv/vmbus_drv.c | 28 ++++------------------------
>  include/linux/hyperv.h |  7 ++++++-
>  2 files changed, 10 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
> index 12a2b37e87f3..a0ff4139c3d2 100644
> --- a/drivers/hv/vmbus_drv.c
> +++ b/drivers/hv/vmbus_drv.c
> @@ -575,31 +575,11 @@ static ssize_t driver_override_store(struct device *dev,
>  				     const char *buf, size_t count)
>  {
>  	struct hv_device *hv_dev = device_to_hv_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 = hv_dev->driver_override;
> -	if (strlen(driver_override)) {
> -		hv_dev->driver_override = driver_override;
> -	} else {
> -		kfree(driver_override);
> -		hv_dev->driver_override = NULL;
> -	}
> -	device_unlock(dev);
> +	int ret;
> 
> -	kfree(old);
> +	ret = driver_set_override(dev, &hv_dev->driver_override, buf, count);
> +	if (ret)
> +		return ret;
> 
>  	return count;
>  }
> diff --git a/include/linux/hyperv.h b/include/linux/hyperv.h
> index fe2e0179ed51..beea11874be2 100644
> --- a/include/linux/hyperv.h
> +++ b/include/linux/hyperv.h
> @@ -1257,7 +1257,12 @@ struct hv_device {
>  	u16 device_id;
> 
>  	struct device device;
> -	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;
> 
>  	struct vmbus_channel *channel;
>  	struct kset	     *channels_kset;
> --
> 2.32.0

Reviewed-by: Michael Kelley <mikelley@microsoft.com>


^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH v3 01/11] driver: platform: Add helper for safer setting of driver_override
  2022-02-27 13:52 ` [PATCH v3 01/11] driver: platform: Add helper for safer setting of driver_override Krzysztof Kozlowski
@ 2022-02-28 10:51   ` Abel Vesa
  2022-02-28 11:30     ` Krzysztof Kozlowski
  2022-02-28 20:03   ` Bjorn Helgaas
  1 sibling, 1 reply; 26+ messages in thread
From: Abel Vesa @ 2022-02-28 10:51 UTC (permalink / raw)
  To: Krzysztof Kozlowski
  Cc: Greg Kroah-Hartman, Rafael J. Wysocki, Stuart Yoder,
	Laurentiu Tudor, Shawn Guo, Sascha Hauer, Fabio Estevam,
	K. Y. Srinivasan, Haiyang Zhang, Stephen Hemminger, Wei Liu,
	Dexuan Cui, Bjorn Helgaas, Bjorn Andersson, Mathieu Poirier,
	Vineeth Vijayan, Peter Oberparleiter, Heiko Carstens,
	Vasily Gorbik, Alexander Gordeev, Christian Borntraeger,
	Sven Schnelle, Andy Gross, Srinivas Kandagatla, Mark Brown,
	Michael S. Tsirkin, Jason Wang, linux-kernel, linux-clk,
	NXP Linux Team, linux-arm-kernel, linux-hyperv, linux-pci,
	linux-remoteproc, linux-s390, linux-arm-msm, alsa-devel,
	linux-spi, virtualization, Linus Torvalds, Rasmus Villemoes

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.

> +{
> +	const char *new, *old;
> +	char *cp;
> +
> +	if (!dev || !override || !s)
> +		return -EINVAL;
> +
> +	/* We need to keep extra room for a newline */
> +	if (len >= (PAGE_SIZE - 1))
> +		return -EINVAL;
> +
> +	new = kstrndup(s, len, GFP_KERNEL);
> +	if (!new)
> +		return -ENOMEM;
> +
> +	cp = strchr(new, '\n');
> +	if (cp)
> +		*cp = '\0';
> +
> +	device_lock(dev);
> +	old = *override;
> +	if (cp != new) {
> +		*override = new;
> +	} else {
> +		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 6cb04ac48bf0..8dd87f44bd74 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..e39963889aa3 100644
> --- a/include/linux/platform_device.h
> +++ b/include/linux/platform_device.h
> @@ -31,7 +31,12 @@ 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
>

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH v3 09/11] clk: imx: scu: Fix kfree() of static memory on setting driver_override
  2022-02-27 13:53 ` [PATCH v3 09/11] clk: imx: scu: Fix kfree() of static memory on setting driver_override Krzysztof Kozlowski
@ 2022-02-28 10:55   ` Abel Vesa
  0 siblings, 0 replies; 26+ messages in thread
From: Abel Vesa @ 2022-02-28 10:55 UTC (permalink / raw)
  To: Krzysztof Kozlowski
  Cc: Greg Kroah-Hartman, Rafael J. Wysocki, Stuart Yoder,
	Laurentiu Tudor, Shawn Guo, Sascha Hauer, Fabio Estevam,
	K. Y. Srinivasan, Haiyang Zhang, Stephen Hemminger, Wei Liu,
	Dexuan Cui, Bjorn Helgaas, Bjorn Andersson, Mathieu Poirier,
	Vineeth Vijayan, Peter Oberparleiter, Heiko Carstens,
	Vasily Gorbik, Alexander Gordeev, Christian Borntraeger,
	Sven Schnelle, Andy Gross, Srinivas Kandagatla, Mark Brown,
	Michael S. Tsirkin, Jason Wang, linux-kernel, linux-clk,
	NXP Linux Team, linux-arm-kernel, linux-hyperv, linux-pci,
	linux-remoteproc, linux-s390, linux-arm-msm, alsa-devel,
	linux-spi, virtualization, Linus Torvalds, Rasmus Villemoes,
	stable

On 22-02-27 14:53:27, Krzysztof Kozlowski wrote:
> 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.
> 
> Fixes: 77d8f3068c63 ("clk: imx: scu: add two cells binding support")
> Cc: <stable@vger.kernel.org>
> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com>
> ---
>  drivers/clk/imx/clk-scu.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/clk/imx/clk-scu.c b/drivers/clk/imx/clk-scu.c
> index 083da31dc3ea..4b2268b7d0d0 100644
> --- a/drivers/clk/imx/clk-scu.c
> +++ b/drivers/clk/imx/clk-scu.c
> @@ -683,7 +683,12 @@ struct clk_hw *imx_clk_scu_alloc_dev(const char *name,
>  		return ERR_PTR(ret);
>  	}
>  
> -	pdev->driver_override = "imx-scu-clk";
> +	ret = driver_set_override(&pdev->dev, &pdev->driver_override,
> +				  "imx-scu-clk", strlen("imx-scu-clk"));

See my comment to patch 01/11.

I would like this to look like the following:

	ret = driver_set_override(&pdev->dev, "imx-scu-clk");

> +	if (ret) {
> +		platform_device_put(pdev);
> +		return ERR_PTR(ret);
> +	}
>  
>  	ret = imx_clk_scu_attach_pd(&pdev->dev, rsrc_id);
>  	if (ret)
> -- 
> 2.32.0
>

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH v3 01/11] driver: platform: Add helper for safer setting of driver_override
  2022-02-28 10:51   ` Abel Vesa
@ 2022-02-28 11:30     ` Krzysztof Kozlowski
  2022-02-28 11:56       ` Abel Vesa
  0 siblings, 1 reply; 26+ messages in thread
From: Krzysztof Kozlowski @ 2022-02-28 11:30 UTC (permalink / raw)
  To: Abel Vesa
  Cc: Greg Kroah-Hartman, Rafael J. Wysocki, Stuart Yoder,
	Laurentiu Tudor, Shawn Guo, Sascha Hauer, Fabio Estevam,
	K. Y. Srinivasan, Haiyang Zhang, Stephen Hemminger, Wei Liu,
	Dexuan Cui, Bjorn Helgaas, Bjorn Andersson, Mathieu Poirier,
	Vineeth Vijayan, Peter Oberparleiter, Heiko Carstens,
	Vasily Gorbik, Alexander Gordeev, Christian Borntraeger,
	Sven Schnelle, Andy Gross, Srinivas Kandagatla, Mark Brown,
	Michael S. Tsirkin, Jason Wang, linux-kernel, linux-clk,
	NXP Linux Team, linux-arm-kernel, linux-hyperv, linux-pci,
	linux-remoteproc, linux-s390, linux-arm-msm, alsa-devel,
	linux-spi, virtualization, Linus Torvalds, Rasmus Villemoes

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

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH v3 01/11] driver: platform: Add helper for safer setting of driver_override
  2022-02-28 11:30     ` Krzysztof Kozlowski
@ 2022-02-28 11:56       ` Abel Vesa
  0 siblings, 0 replies; 26+ messages in thread
From: Abel Vesa @ 2022-02-28 11:56 UTC (permalink / raw)
  To: Krzysztof Kozlowski
  Cc: Greg Kroah-Hartman, Rafael J. Wysocki, Stuart Yoder,
	Laurentiu Tudor, Shawn Guo, Sascha Hauer, Fabio Estevam,
	K. Y. Srinivasan, Haiyang Zhang, Stephen Hemminger, Wei Liu,
	Dexuan Cui, Bjorn Helgaas, Bjorn Andersson, Mathieu Poirier,
	Vineeth Vijayan, Peter Oberparleiter, Heiko Carstens,
	Vasily Gorbik, Alexander Gordeev, Christian Borntraeger,
	Sven Schnelle, Andy Gross, Srinivas Kandagatla, Mark Brown,
	Michael S. Tsirkin, Jason Wang, linux-kernel, linux-clk,
	NXP Linux Team, linux-arm-kernel, linux-hyperv, linux-pci,
	linux-remoteproc, linux-s390, linux-arm-msm, alsa-devel,
	linux-spi, virtualization, Linus Torvalds, Rasmus Villemoes

On 22-02-28 12:30:45, Krzysztof Kozlowski wrote:
> 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. :)

I understand that. Anyway, I was more focused on this looking cleaner.
But I'll let the others weigh in on this before applying the imx clk
patch.

Thanks.

> 
> 
> Best regards,
> Krzysztof

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH v3 07/11] spi: Use helper for safer setting of driver_override
  2022-02-27 13:53 ` [PATCH v3 07/11] spi: Use helper for safer setting of driver_override Krzysztof Kozlowski
@ 2022-02-28 13:07   ` Mark Brown
  0 siblings, 0 replies; 26+ messages in thread
From: Mark Brown @ 2022-02-28 13:07 UTC (permalink / raw)
  To: Krzysztof Kozlowski
  Cc: Greg Kroah-Hartman, Rafael J. Wysocki, Stuart Yoder,
	Laurentiu Tudor, Abel Vesa, Shawn Guo, Sascha Hauer,
	Fabio Estevam, K. Y. Srinivasan, Haiyang Zhang,
	Stephen Hemminger, Wei Liu, Dexuan Cui, Bjorn Helgaas,
	Bjorn Andersson, Mathieu Poirier, Vineeth Vijayan,
	Peter Oberparleiter, Heiko Carstens, Vasily Gorbik,
	Alexander Gordeev, Christian Borntraeger, Sven Schnelle,
	Andy Gross, Srinivas Kandagatla, Michael S. Tsirkin, Jason Wang,
	linux-kernel, linux-clk, NXP Linux Team, linux-arm-kernel,
	linux-hyperv, linux-pci, linux-remoteproc, linux-s390,
	linux-arm-msm, alsa-devel, linux-spi, virtualization,
	Linus Torvalds, Rasmus Villemoes

[-- Attachment #1: Type: text/plain, Size: 195 bytes --]

On Sun, Feb 27, 2022 at 02:53:25PM +0100, Krzysztof Kozlowski wrote:
> Use a helper for seting driver_override to reduce amount of duplicated
> code.

Reviwed-by: Mark Brown <broonie@kernel.org>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH v3 01/11] driver: platform: Add helper for safer setting of driver_override
  2022-02-27 13:52 ` [PATCH v3 01/11] driver: platform: Add helper for safer setting of driver_override Krzysztof Kozlowski
  2022-02-28 10:51   ` Abel Vesa
@ 2022-02-28 20:03   ` Bjorn Helgaas
  2022-03-01  8:49     ` Krzysztof Kozlowski
  1 sibling, 1 reply; 26+ messages in thread
From: Bjorn Helgaas @ 2022-02-28 20:03 UTC (permalink / raw)
  To: Krzysztof Kozlowski
  Cc: Greg Kroah-Hartman, Rafael J. Wysocki, Stuart Yoder,
	Laurentiu Tudor, Abel Vesa, Shawn Guo, Sascha Hauer,
	Fabio Estevam, K. Y. Srinivasan, Haiyang Zhang,
	Stephen Hemminger, Wei Liu, Dexuan Cui, Bjorn Helgaas,
	Bjorn Andersson, Mathieu Poirier, Vineeth Vijayan,
	Peter Oberparleiter, Heiko Carstens, Vasily Gorbik,
	Alexander Gordeev, Christian Borntraeger, Sven Schnelle,
	Andy Gross, Srinivas Kandagatla, Mark Brown, Michael S. Tsirkin,
	Jason Wang, linux-kernel, linux-clk, NXP Linux Team,
	linux-arm-kernel, linux-hyperv, linux-pci, linux-remoteproc,
	linux-s390, linux-arm-msm, alsa-devel, linux-spi, virtualization,
	Linus Torvalds, Rasmus Villemoes

On Sun, Feb 27, 2022 at 02:52:04PM +0100, Krzysztof Kozlowski wrote:
> Several core drivers and buses expect that driver_override is a
> dynamically allocated memory thus later they can kfree() it.

> +int driver_set_override(struct device *dev, const char **override,
> +			const char *s, size_t len)
> +{
> +	const char *new, *old;
> +	char *cp;
> +
> +	if (!dev || !override || !s)
> +		return -EINVAL;
> +
> +	/* We need to keep extra room for a newline */

It would help a lot to extend this comment with a hint about where the
room for a newline is needed.  It was confusing even before, but it's
much more so now that the check is in a completely different file than
the "show" functions that need the space.

> +	if (len >= (PAGE_SIZE - 1))
> +		return -EINVAL;

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH v3 05/11] PCI: Use driver_set_override() instead of open-coding
  2022-02-27 13:52 ` [PATCH v3 05/11] PCI: " Krzysztof Kozlowski
@ 2022-02-28 20:06   ` Bjorn Helgaas
  2022-03-01  8:41     ` Krzysztof Kozlowski
  0 siblings, 1 reply; 26+ messages in thread
From: Bjorn Helgaas @ 2022-02-28 20:06 UTC (permalink / raw)
  To: Krzysztof Kozlowski
  Cc: Greg Kroah-Hartman, Rafael J. Wysocki, Stuart Yoder,
	Laurentiu Tudor, Abel Vesa, Shawn Guo, Sascha Hauer,
	Fabio Estevam, K. Y. Srinivasan, Haiyang Zhang,
	Stephen Hemminger, Wei Liu, Dexuan Cui, Bjorn Helgaas,
	Bjorn Andersson, Mathieu Poirier, Vineeth Vijayan,
	Peter Oberparleiter, Heiko Carstens, Vasily Gorbik,
	Alexander Gordeev, Christian Borntraeger, Sven Schnelle,
	Andy Gross, Srinivas Kandagatla, Mark Brown, Michael S. Tsirkin,
	Jason Wang, linux-kernel, linux-clk, NXP Linux Team,
	linux-arm-kernel, linux-hyperv, linux-pci, linux-remoteproc,
	linux-s390, linux-arm-msm, alsa-devel, linux-spi, virtualization,
	Linus Torvalds, Rasmus Villemoes

On Sun, Feb 27, 2022 at 02:52:08PM +0100, Krzysztof Kozlowski wrote:
> Use a helper for seting driver_override to reduce amount of duplicated
> code. Make the driver_override field const char, because it is not
> modified by the core and it matches other subsystems.

s/seting/setting/
or even better, s/for seting/to set/

> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com>

Acked-by: Bjorn Helgaas <bhelgaas@google.com>

> -	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.

Wrap this comment to fill 78 columns or so.

> +	 */
> +	const char	*driver_override;
>  
>  	unsigned long	priv_flags;	/* Private flags for the PCI driver */
>  
> -- 
> 2.32.0
> 

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH v3 05/11] PCI: Use driver_set_override() instead of open-coding
  2022-02-28 20:06   ` Bjorn Helgaas
@ 2022-03-01  8:41     ` Krzysztof Kozlowski
  0 siblings, 0 replies; 26+ messages in thread
From: Krzysztof Kozlowski @ 2022-03-01  8:41 UTC (permalink / raw)
  To: Bjorn Helgaas
  Cc: Greg Kroah-Hartman, Rafael J. Wysocki, Stuart Yoder,
	Laurentiu Tudor, Abel Vesa, Shawn Guo, Sascha Hauer,
	Fabio Estevam, K. Y. Srinivasan, Haiyang Zhang,
	Stephen Hemminger, Wei Liu, Dexuan Cui, Bjorn Helgaas,
	Bjorn Andersson, Mathieu Poirier, Vineeth Vijayan,
	Peter Oberparleiter, Heiko Carstens, Vasily Gorbik,
	Alexander Gordeev, Christian Borntraeger, Sven Schnelle,
	Andy Gross, Srinivas Kandagatla, Mark Brown, Michael S. Tsirkin,
	Jason Wang, linux-kernel, linux-clk, NXP Linux Team,
	linux-arm-kernel, linux-hyperv, linux-pci, linux-remoteproc,
	linux-s390, linux-arm-msm, alsa-devel, linux-spi, virtualization,
	Linus Torvalds, Rasmus Villemoes

On 28/02/2022 21:06, Bjorn Helgaas wrote:
> On Sun, Feb 27, 2022 at 02:52:08PM +0100, Krzysztof Kozlowski wrote:
>> Use a helper for seting driver_override to reduce amount of duplicated
>> code. Make the driver_override field const char, because it is not
>> modified by the core and it matches other subsystems.
> 
> s/seting/setting/
> or even better, s/for seting/to set/
> 
>> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com>
> 
> Acked-by: Bjorn Helgaas <bhelgaas@google.com>
> 
>> -	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.
> 
> Wrap this comment to fill 78 columns or so.

Thanks, I'll fix both.


Best regards,
Krzysztof

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH v3 01/11] driver: platform: Add helper for safer setting of driver_override
  2022-02-28 20:03   ` Bjorn Helgaas
@ 2022-03-01  8:49     ` Krzysztof Kozlowski
  0 siblings, 0 replies; 26+ messages in thread
From: Krzysztof Kozlowski @ 2022-03-01  8:49 UTC (permalink / raw)
  To: Bjorn Helgaas
  Cc: Greg Kroah-Hartman, Rafael J. Wysocki, Stuart Yoder,
	Laurentiu Tudor, Abel Vesa, Shawn Guo, Sascha Hauer,
	Fabio Estevam, K. Y. Srinivasan, Haiyang Zhang,
	Stephen Hemminger, Wei Liu, Dexuan Cui, Bjorn Helgaas,
	Bjorn Andersson, Mathieu Poirier, Vineeth Vijayan,
	Peter Oberparleiter, Heiko Carstens, Vasily Gorbik,
	Alexander Gordeev, Christian Borntraeger, Sven Schnelle,
	Andy Gross, Srinivas Kandagatla, Mark Brown, Michael S. Tsirkin,
	Jason Wang, linux-kernel, linux-clk, NXP Linux Team,
	linux-arm-kernel, linux-hyperv, linux-pci, linux-remoteproc,
	linux-s390, linux-arm-msm, alsa-devel, linux-spi, virtualization,
	Linus Torvalds, Rasmus Villemoes

On 28/02/2022 21:03, Bjorn Helgaas wrote:
> On Sun, Feb 27, 2022 at 02:52:04PM +0100, Krzysztof Kozlowski wrote:
>> Several core drivers and buses expect that driver_override is a
>> dynamically allocated memory thus later they can kfree() it.
> 
>> +int driver_set_override(struct device *dev, const char **override,
>> +			const char *s, size_t len)
>> +{
>> +	const char *new, *old;
>> +	char *cp;
>> +
>> +	if (!dev || !override || !s)
>> +		return -EINVAL;
>> +
>> +	/* We need to keep extra room for a newline */
> 
> It would help a lot to extend this comment with a hint about where the
> room for a newline is needed.  It was confusing even before, but it's
> much more so now that the check is in a completely different file than
> the "show" functions that need the space.
> 

Indeed, this needs explanation.


Best regards,
Krzysztof

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH v3 06/11] s390: cio: Use driver_set_override() instead of open-coding
  2022-02-27 13:52 ` [PATCH v3 06/11] s390: cio: " Krzysztof Kozlowski
@ 2022-03-01 16:01   ` Vineeth Vijayan
  2022-03-02 11:01     ` Krzysztof Kozlowski
  0 siblings, 1 reply; 26+ messages in thread
From: Vineeth Vijayan @ 2022-03-01 16:01 UTC (permalink / raw)
  To: Krzysztof Kozlowski, Greg Kroah-Hartman, Rafael J. Wysocki,
	Stuart Yoder, Laurentiu Tudor, Abel Vesa, Shawn Guo,
	Sascha Hauer, Fabio Estevam, K. Y. Srinivasan, Haiyang Zhang,
	Stephen Hemminger, Wei Liu, Dexuan Cui, Bjorn Helgaas,
	Bjorn Andersson, Mathieu Poirier, Peter Oberparleiter,
	Heiko Carstens, Vasily Gorbik, Alexander Gordeev,
	Christian Borntraeger, Sven Schnelle, Andy Gross,
	Srinivas Kandagatla, Mark Brown, Michael S. Tsirkin, Jason Wang,
	linux-kernel, linux-clk, NXP Linux Team, linux-arm-kernel,
	linux-hyperv, linux-pci, linux-remoteproc, linux-s390,
	linux-arm-msm, alsa-devel, linux-spi, virtualization,
	Linus Torvalds
  Cc: Rasmus Villemoes


On 2/27/22 14:52, Krzysztof Kozlowski wrote:
> Use a helper for seting driver_override to reduce amount of duplicated
> code. Make the driver_override field const char, because it is not
> modified by the core and it matches other subsystems.
s/seting/setting/

Also could you please change the title to start with "s390/cio:"
instead of "s390 : cio"

Otherwise,

Acked-by: Vineeth Vijayan <vneethv@linux.ibm.com>
> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com>
> ---
>   drivers/s390/cio/cio.h |  7 ++++++-
>   drivers/s390/cio/css.c | 28 ++++------------------------
>   2 files changed, 10 insertions(+), 25 deletions(-)
>
> diff --git a/drivers/s390/cio/cio.h b/drivers/s390/cio/cio.h
> index 1cb9daf9c645..e110c10613e8 100644
> --- a/drivers/s390/cio/cio.h
> +++ b/drivers/s390/cio/cio.h
> @@ -103,7 +103,12 @@ struct subchannel {
>   	struct work_struct todo_work;
>   	struct schib_config config;
>   	u64 dma_mask;
> -	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.
> +	 */
As Bjorn Helgaas mentioned, please wrap this comment.
> +	const char *driver_override;
>   } __attribute__ ((aligned(8)));
>   
>   DECLARE_PER_CPU_ALIGNED(struct irb, cio_irb);
> diff --git a/drivers/s390/cio/css.c b/drivers/s390/cio/css.c
> index fa8293335077..913b6ddd040b 100644
> --- a/drivers/s390/cio/css.c
> +++ b/drivers/s390/cio/css.c
> @@ -338,31 +338,11 @@ static ssize_t driver_override_store(struct device *dev,
>   				     const char *buf, size_t count)
>   {
>   	struct subchannel *sch = to_subchannel(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 = sch->driver_override;
> -	if (strlen(driver_override)) {
> -		sch->driver_override = driver_override;
> -	} else {
> -		kfree(driver_override);
> -		sch->driver_override = NULL;
> -	}
> -	device_unlock(dev);
> +	int ret;
>   
> -	kfree(old);
> +	ret = driver_set_override(dev, &sch->driver_override, buf, count);
> +	if (ret)
> +		return ret;
>   
>   	return count;
>   }

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH v3 10/11] slimbus: qcom-ngd: Fix kfree() of static memory on setting driver_override
  2022-02-27 13:53 ` [PATCH v3 10/11] slimbus: qcom-ngd: " Krzysztof Kozlowski
@ 2022-03-01 17:28   ` Srinivas Kandagatla
  0 siblings, 0 replies; 26+ messages in thread
From: Srinivas Kandagatla @ 2022-03-01 17:28 UTC (permalink / raw)
  To: Krzysztof Kozlowski, Greg Kroah-Hartman, Rafael J. Wysocki,
	Stuart Yoder, Laurentiu Tudor, Abel Vesa, Shawn Guo,
	Sascha Hauer, Fabio Estevam, K. Y. Srinivasan, Haiyang Zhang,
	Stephen Hemminger, Wei Liu, Dexuan Cui, Bjorn Helgaas,
	Bjorn Andersson, Mathieu Poirier, Vineeth Vijayan,
	Peter Oberparleiter, Heiko Carstens, Vasily Gorbik,
	Alexander Gordeev, Christian Borntraeger, Sven Schnelle,
	Andy Gross, Mark Brown, Michael S. Tsirkin, Jason Wang,
	linux-kernel, linux-clk, NXP Linux Team, linux-arm-kernel,
	linux-hyperv, linux-pci, linux-remoteproc, linux-s390,
	linux-arm-msm, alsa-devel, linux-spi, virtualization,
	Linus Torvalds
  Cc: Rasmus Villemoes, stable



On 27/02/2022 13:53, Krzysztof Kozlowski wrote:
> 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.
> 
> Fixes: 917809e2280b ("slimbus: ngd: Add qcom SLIMBus NGD driver")
> Cc: <stable@vger.kernel.org>
> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com>

LGTM,

Reviewed-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>

> ---
>   drivers/slimbus/qcom-ngd-ctrl.c | 13 ++++++++++++-
>   1 file changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/slimbus/qcom-ngd-ctrl.c b/drivers/slimbus/qcom-ngd-ctrl.c
> index 7040293c2ee8..e5d9fdb81eb0 100644
> --- a/drivers/slimbus/qcom-ngd-ctrl.c
> +++ b/drivers/slimbus/qcom-ngd-ctrl.c
> @@ -1434,6 +1434,7 @@ static int of_qcom_slim_ngd_register(struct device *parent,
>   	const struct of_device_id *match;
>   	struct device_node *node;
>   	u32 id;
> +	int ret;
>   
>   	match = of_match_node(qcom_slim_ngd_dt_match, parent->of_node);
>   	data = match->data;
> @@ -1455,7 +1456,17 @@ static int of_qcom_slim_ngd_register(struct device *parent,
>   		}
>   		ngd->id = id;
>   		ngd->pdev->dev.parent = parent;
> -		ngd->pdev->driver_override = QCOM_SLIM_NGD_DRV_NAME;
> +
> +		ret = driver_set_override(&ngd->pdev->dev,
> +					  &ngd->pdev->driver_override,
> +					  QCOM_SLIM_NGD_DRV_NAME,
> +					  strlen(QCOM_SLIM_NGD_DRV_NAME));
> +		if (ret) {
> +			platform_device_put(ngd->pdev);
> +			kfree(ngd);
> +			of_node_put(node);
> +			return ret;
> +		}
>   		ngd->pdev->dev.of_node = node;
>   		ctrl->ngd = ngd;
>   

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH v3 06/11] s390: cio: Use driver_set_override() instead of open-coding
  2022-03-01 16:01   ` Vineeth Vijayan
@ 2022-03-02 11:01     ` Krzysztof Kozlowski
  0 siblings, 0 replies; 26+ messages in thread
From: Krzysztof Kozlowski @ 2022-03-02 11:01 UTC (permalink / raw)
  To: Vineeth Vijayan, Greg Kroah-Hartman, Rafael J. Wysocki,
	Stuart Yoder, Laurentiu Tudor, Abel Vesa, Shawn Guo,
	Sascha Hauer, Fabio Estevam, K. Y. Srinivasan, Haiyang Zhang,
	Stephen Hemminger, Wei Liu, Dexuan Cui, Bjorn Helgaas,
	Bjorn Andersson, Mathieu Poirier, Peter Oberparleiter,
	Heiko Carstens, Vasily Gorbik, Alexander Gordeev,
	Christian Borntraeger, Sven Schnelle, Andy Gross,
	Srinivas Kandagatla, Mark Brown, Michael S. Tsirkin, Jason Wang,
	linux-kernel, linux-clk, NXP Linux Team, linux-arm-kernel,
	linux-hyperv, linux-pci, linux-remoteproc, linux-s390,
	linux-arm-msm, alsa-devel, linux-spi, virtualization,
	Linus Torvalds
  Cc: Rasmus Villemoes

On 01/03/2022 17:01, Vineeth Vijayan wrote:
> 
> On 2/27/22 14:52, Krzysztof Kozlowski wrote:
>> Use a helper for seting driver_override to reduce amount of duplicated
>> code. Make the driver_override field const char, because it is not
>> modified by the core and it matches other subsystems.
> s/seting/setting/
> 
> Also could you please change the title to start with "s390/cio:"
> instead of "s390 : cio"
> 

Sure, thanks for review!


Best regards,
Krzysztof

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH v3 11/11] rpmsg: Fix kfree() of static memory on setting driver_override
  2022-02-27 13:53 ` [PATCH v3 11/11] rpmsg: " Krzysztof Kozlowski
@ 2022-03-11 20:19   ` Bjorn Andersson
  0 siblings, 0 replies; 26+ messages in thread
From: Bjorn Andersson @ 2022-03-11 20:19 UTC (permalink / raw)
  To: Krzysztof Kozlowski
  Cc: Greg Kroah-Hartman, Rafael J. Wysocki, Stuart Yoder,
	Laurentiu Tudor, Abel Vesa, Shawn Guo, Sascha Hauer,
	Fabio Estevam, K. Y. Srinivasan, Haiyang Zhang,
	Stephen Hemminger, Wei Liu, Dexuan Cui, Bjorn Helgaas,
	Mathieu Poirier, Vineeth Vijayan, Peter Oberparleiter,
	Heiko Carstens, Vasily Gorbik, Alexander Gordeev,
	Christian Borntraeger, Sven Schnelle, Andy Gross,
	Srinivas Kandagatla, Mark Brown, Michael S. Tsirkin, Jason Wang,
	linux-kernel, linux-clk, NXP Linux Team, linux-arm-kernel,
	linux-hyperv, linux-pci, linux-remoteproc, linux-s390,
	linux-arm-msm, alsa-devel, linux-spi, virtualization,
	Linus Torvalds, Rasmus Villemoes, stable

On Sun 27 Feb 07:53 CST 2022, Krzysztof Kozlowski wrote:

> 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.
> 
> Fixes: 950a7388f02b ("rpmsg: Turn name service into a stand alone driver")
> Fixes: c0cdc19f84a4 ("rpmsg: Driver for user space endpoint interface")
> Cc: <stable@vger.kernel.org>
> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com>

Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>

Regards,
Bjorn

> ---
>  drivers/rpmsg/rpmsg_core.c     |  3 ++-
>  drivers/rpmsg/rpmsg_internal.h | 13 +++++++++++--
>  drivers/rpmsg/rpmsg_ns.c       | 14 ++++++++++++--
>  include/linux/rpmsg.h          |  6 ++++--
>  4 files changed, 29 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/rpmsg/rpmsg_core.c b/drivers/rpmsg/rpmsg_core.c
> index d9e612f4f0f2..6e2bf2742973 100644
> --- a/drivers/rpmsg/rpmsg_core.c
> +++ b/drivers/rpmsg/rpmsg_core.c
> @@ -397,7 +397,8 @@ field##_store(struct device *dev, struct device_attribute *attr,	\
>  	      const char *buf, size_t sz)				\
>  {									\
>  	struct rpmsg_device *rpdev = to_rpmsg_device(dev);		\
> -	char *new, *old;						\
> +	const char *old;						\
> +	char *new;							\
>  									\
>  	new = kstrndup(buf, sz, GFP_KERNEL);				\
>  	if (!new)							\
> diff --git a/drivers/rpmsg/rpmsg_internal.h b/drivers/rpmsg/rpmsg_internal.h
> index b1245d3ed7c6..31345d6e9a7e 100644
> --- a/drivers/rpmsg/rpmsg_internal.h
> +++ b/drivers/rpmsg/rpmsg_internal.h
> @@ -92,10 +92,19 @@ int rpmsg_release_channel(struct rpmsg_device *rpdev,
>   */
>  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,
> +				  "rpmsg_chrdev", strlen("rpmsg_chrdev"));
> +	if (ret)
> +		return ret;
> +
> +	ret = rpmsg_register_device(rpdev);
> +	if (ret)
> +		kfree(rpdev->driver_override);
>  
> -	return rpmsg_register_device(rpdev);
> +	return ret;
>  }
>  
>  #endif
> diff --git a/drivers/rpmsg/rpmsg_ns.c b/drivers/rpmsg/rpmsg_ns.c
> index 762ff1ae279f..95a51543f5ad 100644
> --- a/drivers/rpmsg/rpmsg_ns.c
> +++ b/drivers/rpmsg/rpmsg_ns.c
> @@ -20,12 +20,22 @@
>   */
>  int rpmsg_ns_register_device(struct rpmsg_device *rpdev)
>  {
> +	int ret;
> +
>  	strcpy(rpdev->id.name, "rpmsg_ns");
> -	rpdev->driver_override = "rpmsg_ns";
> +	ret = driver_set_override(&rpdev->dev, &rpdev->driver_override,
> +				  "rpmsg_ns", strlen("rpmsg_ns"));
> +	if (ret)
> +		return ret;
> +
>  	rpdev->src = RPMSG_NS_ADDR;
>  	rpdev->dst = RPMSG_NS_ADDR;
>  
> -	return rpmsg_register_device(rpdev);
> +	ret = rpmsg_register_device(rpdev);
> +	if (ret)
> +		kfree(rpdev->driver_override);
> +
> +	return ret;
>  }
>  EXPORT_SYMBOL(rpmsg_ns_register_device);
>  
> diff --git a/include/linux/rpmsg.h b/include/linux/rpmsg.h
> index 02fa9116cd60..20c8cd1cde21 100644
> --- a/include/linux/rpmsg.h
> +++ b/include/linux/rpmsg.h
> @@ -41,7 +41,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
> @@ -51,7 +53,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;
> -- 
> 2.32.0
> 

^ permalink raw reply	[flat|nested] 26+ messages in thread

end of thread, other threads:[~2022-03-11 20:19 UTC | newest]

Thread overview: 26+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-02-27 13:52 [PATCH v3 00/11] Fix broken usage of driver_override (and kfree of static memory) Krzysztof Kozlowski
2022-02-27 13:52 ` [PATCH v3 01/11] driver: platform: Add helper for safer setting of driver_override Krzysztof Kozlowski
2022-02-28 10:51   ` Abel Vesa
2022-02-28 11:30     ` Krzysztof Kozlowski
2022-02-28 11:56       ` Abel Vesa
2022-02-28 20:03   ` Bjorn Helgaas
2022-03-01  8:49     ` Krzysztof Kozlowski
2022-02-27 13:52 ` [PATCH v3 02/11] amba: Use driver_set_override() instead of open-coding Krzysztof Kozlowski
2022-02-27 13:52 ` [PATCH v3 03/11] fsl-mc: " Krzysztof Kozlowski
2022-02-27 13:52 ` [PATCH v3 04/11] hv: " Krzysztof Kozlowski
2022-02-27 15:53   ` Michael Kelley (LINUX)
2022-02-27 13:52 ` [PATCH v3 05/11] PCI: " Krzysztof Kozlowski
2022-02-28 20:06   ` Bjorn Helgaas
2022-03-01  8:41     ` Krzysztof Kozlowski
2022-02-27 13:52 ` [PATCH v3 06/11] s390: cio: " Krzysztof Kozlowski
2022-03-01 16:01   ` Vineeth Vijayan
2022-03-02 11:01     ` Krzysztof Kozlowski
2022-02-27 13:53 ` [PATCH v3 07/11] spi: Use helper for safer setting of driver_override Krzysztof Kozlowski
2022-02-28 13:07   ` Mark Brown
2022-02-27 13:53 ` [PATCH v3 08/11] vdpa: " Krzysztof Kozlowski
2022-02-27 13:53 ` [PATCH v3 09/11] clk: imx: scu: Fix kfree() of static memory on setting driver_override Krzysztof Kozlowski
2022-02-28 10:55   ` Abel Vesa
2022-02-27 13:53 ` [PATCH v3 10/11] slimbus: qcom-ngd: " Krzysztof Kozlowski
2022-03-01 17:28   ` Srinivas Kandagatla
2022-02-27 13:53 ` [PATCH v3 11/11] rpmsg: " Krzysztof Kozlowski
2022-03-11 20:19   ` Bjorn Andersson

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).