[PATCH 0/2] rpmsg: fix after driver_override patchset

[PATCH 0/2] rpmsg: fix after driver_override patchset

[Krzysztof Kozlowski]

Hi,

Marek reported issue introduced by commit in Greg's tree:
2cd402b8fd4 ("rpmsg: Fix kfree() of static memory on setting driver_override")

Therefore this should go via Greg's tree as well.

Marek,
Could you test if it fixes your issue? It worked for my case which used
different RPMSG driver.

Best regards,
Krzysztof

Krzysztof Kozlowski (2):
  rpmsg: Fix calling device_lock() on non-initialized device
  rpmsg: use local 'dev' variable

 drivers/rpmsg/rpmsg_core.c     | 39 ++++++++++++++++++++++++++++------
 drivers/rpmsg/rpmsg_internal.h | 14 +-----------
 drivers/rpmsg/rpmsg_ns.c       | 14 +-----------
 include/linux/rpmsg.h          |  8 +++++++
 4 files changed, 43 insertions(+), 32 deletions(-)

-- 
2.32.0

[PATCH 1/2] rpmsg: Fix calling device_lock() on non-initialized device

[Krzysztof Kozlowski]

driver_set_override() helper uses device_lock() so it should not be
called before rpmsg_register_device() (which calls device_register()).
Effect can be seen with CONFIG_DEBUG_MUTEXES:

  DEBUG_LOCKS_WARN_ON(lock->magic != lock)
  WARNING: CPU: 3 PID: 57 at kernel/locking/mutex.c:582 __mutex_lock+0x1ec/0x430
  ...
  Call trace:
   __mutex_lock+0x1ec/0x430
   mutex_lock_nested+0x44/0x50
   driver_set_override+0x124/0x150
   qcom_glink_native_probe+0x30c/0x3b0
   glink_rpm_probe+0x274/0x350
   platform_probe+0x6c/0xe0
   really_probe+0x17c/0x3d0
   __driver_probe_device+0x114/0x190
   driver_probe_device+0x3c/0xf0
   ...

Refactor the rpmsg_register_device() function to use two-step device
registering (initialization + add) and call driver_set_override() in
proper moment.

This moves the code around, so while at it also NULL-ify the
rpdev->driver_override in error path to be sure it won't be kfree()
second time.

Fixes: 42cd402b8fd4 ("rpmsg: Fix kfree() of static memory on setting driver_override")
Reported-by: Marek Szyprowski <m.szyprowski@samsung.com>
Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

---

Commit SHA from linux-next - Greg's tree.
---
 drivers/rpmsg/rpmsg_core.c     | 33 ++++++++++++++++++++++++++++++---
 drivers/rpmsg/rpmsg_internal.h | 14 +-------------
 drivers/rpmsg/rpmsg_ns.c       | 14 +-------------
 include/linux/rpmsg.h          |  8 ++++++++
 4 files changed, 40 insertions(+), 29 deletions(-)

diff --git a/drivers/rpmsg/rpmsg_core.c b/drivers/rpmsg/rpmsg_core.c
index 95fc283f6af7..4938fc4eff00 100644
--- a/drivers/rpmsg/rpmsg_core.c
+++ b/drivers/rpmsg/rpmsg_core.c
@@ -593,24 +593,51 @@ static struct bus_type rpmsg_bus = {
 	.remove		= rpmsg_dev_remove,
 };
 
-int rpmsg_register_device(struct rpmsg_device *rpdev)
+/*
+ * A helper for registering rpmsg device with driver override and name.
+ * Drivers should not be using it, but instead rpmsg_register_device().
+ */
+int rpmsg_register_device_override(struct rpmsg_device *rpdev,
+				   const char *driver_override)
 {
 	struct device *dev = &rpdev->dev;
 	int ret;
 
+	if (driver_override)
+		strcpy(rpdev->id.name, driver_override);
+
 	dev_set_name(&rpdev->dev, "%s.%s.%d.%d", dev_name(dev->parent),
 		     rpdev->id.name, rpdev->src, rpdev->dst);
 
 	rpdev->dev.bus = &rpmsg_bus;
 
-	ret = device_register(&rpdev->dev);
+	device_initialize(dev);
+	if (driver_override) {
+		ret = driver_set_override(dev, &rpdev->driver_override,
+					  driver_override,
+					  strlen(driver_override));
+		if (ret) {
+			dev_err(dev, "device_set_override failed: %d\n", ret);
+			return ret;
+		}
+	}
+
+	ret = device_add(dev);
 	if (ret) {
-		dev_err(dev, "device_register failed: %d\n", ret);
+		dev_err(dev, "device_add failed: %d\n", ret);
+		kfree(rpdev->driver_override);
+		rpdev->driver_override = NULL;
 		put_device(&rpdev->dev);
 	}
 
 	return ret;
 }
+EXPORT_SYMBOL(rpmsg_register_device_override);
+
+int rpmsg_register_device(struct rpmsg_device *rpdev)
+{
+	return rpmsg_register_device_override(rpdev, NULL);
+}
 EXPORT_SYMBOL(rpmsg_register_device);
 
 /*
diff --git a/drivers/rpmsg/rpmsg_internal.h b/drivers/rpmsg/rpmsg_internal.h
index 3e81642238d2..a22cd4abe7d1 100644
--- a/drivers/rpmsg/rpmsg_internal.h
+++ b/drivers/rpmsg/rpmsg_internal.h
@@ -94,19 +94,7 @@ int rpmsg_release_channel(struct rpmsg_device *rpdev,
  */
 static inline int rpmsg_ctrldev_register_device(struct rpmsg_device *rpdev)
 {
-	int ret;
-
-	strcpy(rpdev->id.name, "rpmsg_ctrl");
-	ret = driver_set_override(&rpdev->dev, &rpdev->driver_override,
-				  rpdev->id.name, strlen(rpdev->id.name));
-	if (ret)
-		return ret;
-
-	ret = rpmsg_register_device(rpdev);
-	if (ret)
-		kfree(rpdev->driver_override);
-
-	return ret;
+	return rpmsg_register_device_override(rpdev, "rpmsg_ctrl");
 }
 
 #endif
diff --git a/drivers/rpmsg/rpmsg_ns.c b/drivers/rpmsg/rpmsg_ns.c
index 8eb8f328237e..c70ad03ff2e9 100644
--- a/drivers/rpmsg/rpmsg_ns.c
+++ b/drivers/rpmsg/rpmsg_ns.c
@@ -20,22 +20,10 @@
  */
 int rpmsg_ns_register_device(struct rpmsg_device *rpdev)
 {
-	int ret;
-
-	strcpy(rpdev->id.name, "rpmsg_ns");
-	ret = driver_set_override(&rpdev->dev, &rpdev->driver_override,
-				  rpdev->id.name, strlen(rpdev->id.name));
-	if (ret)
-		return ret;
-
 	rpdev->src = RPMSG_NS_ADDR;
 	rpdev->dst = RPMSG_NS_ADDR;
 
-	ret = rpmsg_register_device(rpdev);
-	if (ret)
-		kfree(rpdev->driver_override);
-
-	return ret;
+	return rpmsg_register_device_override(rpdev, "rpmsg_ns");
 }
 EXPORT_SYMBOL(rpmsg_ns_register_device);
 
diff --git a/include/linux/rpmsg.h b/include/linux/rpmsg.h
index 20c8cd1cde21..523c98b96cb4 100644
--- a/include/linux/rpmsg.h
+++ b/include/linux/rpmsg.h
@@ -165,6 +165,8 @@ static inline __rpmsg64 cpu_to_rpmsg64(struct rpmsg_device *rpdev, u64 val)
 
 #if IS_ENABLED(CONFIG_RPMSG)
 
+int rpmsg_register_device_override(struct rpmsg_device *rpdev,
+				   const char *driver_override);
 int rpmsg_register_device(struct rpmsg_device *rpdev);
 int rpmsg_unregister_device(struct device *parent,
 			    struct rpmsg_channel_info *chinfo);
@@ -192,6 +194,12 @@ ssize_t rpmsg_get_mtu(struct rpmsg_endpoint *ept);
 
 #else
 
+static inline int rpmsg_register_device_override(struct rpmsg_device *rpdev,
+						 const char *driver_override)
+{
+	return -ENXIO;
+}
+
 static inline int rpmsg_register_device(struct rpmsg_device *rpdev)
 {
 	return -ENXIO;
-- 
2.32.0

[PATCH 2/2] rpmsg: use local 'dev' variable

[Krzysztof Kozlowski]

'&rpdev->dev' is already cached as local variable, so use it to simplify
the code.

Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
---
 drivers/rpmsg/rpmsg_core.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/rpmsg/rpmsg_core.c b/drivers/rpmsg/rpmsg_core.c
index 4938fc4eff00..290c1f02da10 100644
--- a/drivers/rpmsg/rpmsg_core.c
+++ b/drivers/rpmsg/rpmsg_core.c
@@ -606,10 +606,10 @@ int rpmsg_register_device_override(struct rpmsg_device *rpdev,
 	if (driver_override)
 		strcpy(rpdev->id.name, driver_override);
 
-	dev_set_name(&rpdev->dev, "%s.%s.%d.%d", dev_name(dev->parent),
+	dev_set_name(dev, "%s.%s.%d.%d", dev_name(dev->parent),
 		     rpdev->id.name, rpdev->src, rpdev->dst);
 
-	rpdev->dev.bus = &rpmsg_bus;
+	dev->bus = &rpmsg_bus;
 
 	device_initialize(dev);
 	if (driver_override) {
@@ -627,7 +627,7 @@ int rpmsg_register_device_override(struct rpmsg_device *rpdev,
 		dev_err(dev, "device_add failed: %d\n", ret);
 		kfree(rpdev->driver_override);
 		rpdev->driver_override = NULL;
-		put_device(&rpdev->dev);
+		put_device(dev);
 	}
 
 	return ret;
-- 
2.32.0

Re: [PATCH 1/2] rpmsg: Fix calling device_lock() on non-initialized device

[Marek Szyprowski]

On 29.04.2022 21:59, Krzysztof Kozlowski wrote:
> driver_set_override() helper uses device_lock() so it should not be
> called before rpmsg_register_device() (which calls device_register()).
> Effect can be seen with CONFIG_DEBUG_MUTEXES:
>
>    DEBUG_LOCKS_WARN_ON(lock->magic != lock)
>    WARNING: CPU: 3 PID: 57 at kernel/locking/mutex.c:582 __mutex_lock+0x1ec/0x430
>    ...
>    Call trace:
>     __mutex_lock+0x1ec/0x430
>     mutex_lock_nested+0x44/0x50
>     driver_set_override+0x124/0x150
>     qcom_glink_native_probe+0x30c/0x3b0
>     glink_rpm_probe+0x274/0x350
>     platform_probe+0x6c/0xe0
>     really_probe+0x17c/0x3d0
>     __driver_probe_device+0x114/0x190
>     driver_probe_device+0x3c/0xf0
>     ...
>
> Refactor the rpmsg_register_device() function to use two-step device
> registering (initialization + add) and call driver_set_override() in
> proper moment.
>
> This moves the code around, so while at it also NULL-ify the
> rpdev->driver_override in error path to be sure it won't be kfree()
> second time.
>
> Fixes: 42cd402b8fd4 ("rpmsg: Fix kfree() of static memory on setting driver_override")
> Reported-by: Marek Szyprowski <m.szyprowski@samsung.com>
> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Tested-by: Marek Szyprowski <m.szyprowski@samsung.com>
> ---
>
> Commit SHA from linux-next - Greg's tree.
> ---
>   drivers/rpmsg/rpmsg_core.c     | 33 ++++++++++++++++++++++++++++++---
>   drivers/rpmsg/rpmsg_internal.h | 14 +-------------
>   drivers/rpmsg/rpmsg_ns.c       | 14 +-------------
>   include/linux/rpmsg.h          |  8 ++++++++
>   4 files changed, 40 insertions(+), 29 deletions(-)
>
> diff --git a/drivers/rpmsg/rpmsg_core.c b/drivers/rpmsg/rpmsg_core.c
> index 95fc283f6af7..4938fc4eff00 100644
> --- a/drivers/rpmsg/rpmsg_core.c
> +++ b/drivers/rpmsg/rpmsg_core.c
> @@ -593,24 +593,51 @@ static struct bus_type rpmsg_bus = {
>   	.remove		= rpmsg_dev_remove,
>   };
>   
> -int rpmsg_register_device(struct rpmsg_device *rpdev)
> +/*
> + * A helper for registering rpmsg device with driver override and name.
> + * Drivers should not be using it, but instead rpmsg_register_device().
> + */
> +int rpmsg_register_device_override(struct rpmsg_device *rpdev,
> +				   const char *driver_override)
>   {
>   	struct device *dev = &rpdev->dev;
>   	int ret;
>   
> +	if (driver_override)
> +		strcpy(rpdev->id.name, driver_override);
> +
>   	dev_set_name(&rpdev->dev, "%s.%s.%d.%d", dev_name(dev->parent),
>   		     rpdev->id.name, rpdev->src, rpdev->dst);
>   
>   	rpdev->dev.bus = &rpmsg_bus;
>   
> -	ret = device_register(&rpdev->dev);
> +	device_initialize(dev);
> +	if (driver_override) {
> +		ret = driver_set_override(dev, &rpdev->driver_override,
> +					  driver_override,
> +					  strlen(driver_override));
> +		if (ret) {
> +			dev_err(dev, "device_set_override failed: %d\n", ret);
> +			return ret;
> +		}
> +	}
> +
> +	ret = device_add(dev);
>   	if (ret) {
> -		dev_err(dev, "device_register failed: %d\n", ret);
> +		dev_err(dev, "device_add failed: %d\n", ret);
> +		kfree(rpdev->driver_override);
> +		rpdev->driver_override = NULL;
>   		put_device(&rpdev->dev);
>   	}
>   
>   	return ret;
>   }
> +EXPORT_SYMBOL(rpmsg_register_device_override);
> +
> +int rpmsg_register_device(struct rpmsg_device *rpdev)
> +{
> +	return rpmsg_register_device_override(rpdev, NULL);
> +}
>   EXPORT_SYMBOL(rpmsg_register_device);
>   
>   /*
> diff --git a/drivers/rpmsg/rpmsg_internal.h b/drivers/rpmsg/rpmsg_internal.h
> index 3e81642238d2..a22cd4abe7d1 100644
> --- a/drivers/rpmsg/rpmsg_internal.h
> +++ b/drivers/rpmsg/rpmsg_internal.h
> @@ -94,19 +94,7 @@ int rpmsg_release_channel(struct rpmsg_device *rpdev,
>    */
>   static inline int rpmsg_ctrldev_register_device(struct rpmsg_device *rpdev)
>   {
> -	int ret;
> -
> -	strcpy(rpdev->id.name, "rpmsg_ctrl");
> -	ret = driver_set_override(&rpdev->dev, &rpdev->driver_override,
> -				  rpdev->id.name, strlen(rpdev->id.name));
> -	if (ret)
> -		return ret;
> -
> -	ret = rpmsg_register_device(rpdev);
> -	if (ret)
> -		kfree(rpdev->driver_override);
> -
> -	return ret;
> +	return rpmsg_register_device_override(rpdev, "rpmsg_ctrl");
>   }
>   
>   #endif
> diff --git a/drivers/rpmsg/rpmsg_ns.c b/drivers/rpmsg/rpmsg_ns.c
> index 8eb8f328237e..c70ad03ff2e9 100644
> --- a/drivers/rpmsg/rpmsg_ns.c
> +++ b/drivers/rpmsg/rpmsg_ns.c
> @@ -20,22 +20,10 @@
>    */
>   int rpmsg_ns_register_device(struct rpmsg_device *rpdev)
>   {
> -	int ret;
> -
> -	strcpy(rpdev->id.name, "rpmsg_ns");
> -	ret = driver_set_override(&rpdev->dev, &rpdev->driver_override,
> -				  rpdev->id.name, strlen(rpdev->id.name));
> -	if (ret)
> -		return ret;
> -
>   	rpdev->src = RPMSG_NS_ADDR;
>   	rpdev->dst = RPMSG_NS_ADDR;
>   
> -	ret = rpmsg_register_device(rpdev);
> -	if (ret)
> -		kfree(rpdev->driver_override);
> -
> -	return ret;
> +	return rpmsg_register_device_override(rpdev, "rpmsg_ns");
>   }
>   EXPORT_SYMBOL(rpmsg_ns_register_device);
>   
> diff --git a/include/linux/rpmsg.h b/include/linux/rpmsg.h
> index 20c8cd1cde21..523c98b96cb4 100644
> --- a/include/linux/rpmsg.h
> +++ b/include/linux/rpmsg.h
> @@ -165,6 +165,8 @@ static inline __rpmsg64 cpu_to_rpmsg64(struct rpmsg_device *rpdev, u64 val)
>   
>   #if IS_ENABLED(CONFIG_RPMSG)
>   
> +int rpmsg_register_device_override(struct rpmsg_device *rpdev,
> +				   const char *driver_override);
>   int rpmsg_register_device(struct rpmsg_device *rpdev);
>   int rpmsg_unregister_device(struct device *parent,
>   			    struct rpmsg_channel_info *chinfo);
> @@ -192,6 +194,12 @@ ssize_t rpmsg_get_mtu(struct rpmsg_endpoint *ept);
>   
>   #else
>   
> +static inline int rpmsg_register_device_override(struct rpmsg_device *rpdev,
> +						 const char *driver_override)
> +{
> +	return -ENXIO;
> +}
> +
>   static inline int rpmsg_register_device(struct rpmsg_device *rpdev)
>   {
>   	return -ENXIO;

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland