Fixes and rework of rpmsg_eptdev_add()

put_device() is called on error path of rpmsg_eptdev_add() to cleanup resource attached to eptdev->dev, unfortunately it's bogus cause dev->release() is not set yet.

When a struct device instance is destroyed, driver core framework checks the possible release() callback from candidates below:
- struct device::release()
- dev->type->release()
- dev->class->dev_release()

Rpmsg eptdev owns none of them so WARN() will complain the absence of release().

Fix it by:
- Pre-assign dev->release() before potential error path.
- Check before ida_free() in dev->release().

By fixing error path of rpmsg_eptdev_add() and fixing potential memory leak in rpmsg_anonymous_eptdev_create(), this work paves the way of rework of rpmsg_eptdev_add() and its callers.

Fixes c0cdc19f84a4 ("rpmsg: Driver for user space endpoint interface")

Signed-off-by Dawei Li <[email protected]>

---
  drivers/rpmsg/rpmsg_char.c | 26 +++++++++++++-------------
  1 file changed, 13 insertions(+), 13 deletions(-)

drivers/rpmsg/rpmsg_char.c +11 -7

index 34b35ea74aab..373b627581e8 100644
@@ -408,8 +408,13 @@ static void rpmsg_eptdev_release_device(struct device *dev)
  {
  	struct rpmsg_eptdev *eptdev = dev_to_eptdev(dev);
 
-	ida_free(&rpmsg_ept_ida, dev->id);
-	if (eptdev->dev.devt)
+	/*
+	 * release() can be invoked from error path of rpmsg_eptdev_add(),
+	 * WARN() will be fired if ida_free() is feed with invalid ID.
+	 */
+	if (likely(ida_exists(&rpmsg_ept_ida, dev->id)))
+		ida_free(&rpmsg_ept_ida, dev->id);
+	if (eptdev->dev.devt && likely(ida_exists(&rpmsg_minor_ida, MINOR(eptdev->dev.devt))))
  		ida_free(&rpmsg_minor_ida, MINOR(eptdev->dev.devt));
  	kfree(eptdev);
  }

@@ -458,6 +463,8 @@ static int rpmsg_eptdev_add(struct rpmsg_eptdev *eptdev,
  	struct device *dev = &eptdev->dev;
  	int ret;
 
+	dev->release = rpmsg_eptdev_release_device;
+
  	eptdev->chinfo = chinfo;
 
  	if (cdev) {

@@ -471,7 +478,7 @@ static int rpmsg_eptdev_add(struct rpmsg_eptdev *eptdev,
  	/* Anonymous inode device still need device name for dev_err() and friends */
  	ret = ida_alloc(&rpmsg_ept_ida, GFP_KERNEL);
  	if (ret < 0)
-		goto free_minor_ida;
+		goto free_eptdev;
  	dev->id = ret;
  	dev_set_name(dev, "rpmsg%d", ret);
 

@@ -480,22 +487,13 @@ static int rpmsg_eptdev_add(struct rpmsg_eptdev *eptdev,
  	if (cdev) {
  		ret = cdev_device_add(&eptdev->cdev, &eptdev->dev);
  		if (ret)
-			goto free_ept_ida;
+			goto free_eptdev;
  	}
 
-	/* We can now rely on the release function for cleanup */
-	dev->release = rpmsg_eptdev_release_device;
-

drivers/rpmsg/rpmsg_char.c +2 -0

  	return ret;
  }

@@ -561,6 +559,8 @@ int rpmsg_anonymous_eptdev_create(struct rpmsg_device *rpdev, struct device *par
 
  	if (!ret)
  		*pfd = fd;
+	else
+		put_device(&eptdev->dev);
 
  	return ret;
  }

Quoted 70 lines · Dawei Li

put_device() is called on error path of rpmsg_eptdev_add() to cleanup resource attached to eptdev->dev, unfortunately it's bogus cause dev->release() is not set yet.

When a struct device instance is destroyed, driver core framework checks the possible release() callback from candidates below:
- struct device::release()
- dev->type->release()
- dev->class->dev_release()

Rpmsg eptdev owns none of them so WARN() will complain the absence of release().

Fix it by:
- Pre-assign dev->release() before potential error path.
- Check before ida_free() in dev->release().

By fixing error path of rpmsg_eptdev_add() and fixing potential memory leak in rpmsg_anonymous_eptdev_create(), this work paves the way of rework of rpmsg_eptdev_add() and its callers.

Fixes c0cdc19f84a4 ("rpmsg: Driver for user space endpoint interface")

Signed-off-by Dawei Li <[email protected]>

---
  drivers/rpmsg/rpmsg_char.c | 26 +++++++++++++-------------
  1 file changed, 13 insertions(+), 13 deletions(-)

drivers/rpmsg/rpmsg_char.c +11 -6

index 34b35ea74aab..373b627581e8 100644
@@ -408,8 +408,13 @@ static void rpmsg_eptdev_release_device(struct device *dev)
  {
  	struct rpmsg_eptdev *eptdev = dev_to_eptdev(dev);
-	ida_free(&rpmsg_ept_ida, dev->id);
-	if (eptdev->dev.devt)
+	/*
+	 * release() can be invoked from error path of rpmsg_eptdev_add(),
+	 * WARN() will be fired if ida_free() is feed with invalid ID.
+	 */
+	if (likely(ida_exists(&rpmsg_ept_ida, dev->id)))
+		ida_free(&rpmsg_ept_ida, dev->id);
+	if (eptdev->dev.devt && likely(ida_exists(&rpmsg_minor_ida, MINOR(eptdev->dev.devt))))
  		ida_free(&rpmsg_minor_ida, MINOR(eptdev->dev.devt));
  	kfree(eptdev);
  }

@@ -458,6 +463,8 @@ static int rpmsg_eptdev_add(struct rpmsg_eptdev *eptdev,
  	struct device *dev = &eptdev->dev;
  	int ret;
+	dev->release = rpmsg_eptdev_release_device;
+
  	eptdev->chinfo = chinfo;
  	if (cdev) {

@@ -471,7 +478,7 @@ static int rpmsg_eptdev_add(struct rpmsg_eptdev *eptdev,
  	/* Anonymous inode device still need device name for dev_err() and friends */
  	ret = ida_alloc(&rpmsg_ept_ida, GFP_KERNEL);
  	if (ret < 0)
-		goto free_minor_ida;
+		goto free_eptdev;
  	dev->id = ret;
  	dev_set_name(dev, "rpmsg%d", ret);

@@ -480,22 +487,13 @@ static int rpmsg_eptdev_add(struct rpmsg_eptdev *eptdev,
  	if (cdev) {
  		ret = cdev_device_add(&eptdev->cdev, &eptdev->dev);
  		if (ret)
-			goto free_ept_ida;
+			goto free_eptdev;
  	}
-	/* We can now rely on the release function for cleanup */
-	dev->release = rpmsg_eptdev_release_device;

drivers/rpmsg/rpmsg_char.c static int rpmsg_eptdev_add(struct rpmsg_eptdev *eptdev, · line 496

Quoted 10 lines

drivers/rpmsg/rpmsg_char.c +0 -7

-
  	return ret;
-free_ept_ida:
-	ida_free(&rpmsg_ept_ida, dev->id);
-free_minor_ida:
-	if (cdev)
-		ida_free(&rpmsg_minor_ida, MINOR(dev->devt));
  free_eptdev:
  	put_device(dev);
-	kfree(eptdev);

Hi Dawei,

Thanks for your new version~

Patch 1/3 will introduce a use-after-free of eptdev in func rpmsg_anonymous_eptdev_create(),

https://git.kernel.org/pub/scm/linux/kernel/git/remoteproc/linux.git/tree/drivers/rpmsg/rpmsg_char.c?h=for-next#n548

even though this issue will be resolved in 2/3. However, 1/3, as an independent commit, should not introduce a new bug.

Quoted 11 lines

drivers/rpmsg/rpmsg_char.c +2 -0

  	return ret;
  }

@@ -561,6 +559,8 @@ int rpmsg_anonymous_eptdev_create(struct rpmsg_device *rpdev, struct device *par
  	if (!ret)
  		*pfd = fd;
+	else
+		put_device(&eptdev->dev);
  	return ret;
  }

Signature · 2 lines

Thx and BRs,
Zhongqiu Han

Quoted 90 lines · Zhongqiu Han

Quoted 70 lines · Dawei Li

put_device() is called on error path of rpmsg_eptdev_add() to cleanup resource attached to eptdev->dev, unfortunately it's bogus cause dev->release() is not set yet.

When a struct device instance is destroyed, driver core framework checks the possible release() callback from candidates below:
- struct device::release()
- dev->type->release()
- dev->class->dev_release()

Rpmsg eptdev owns none of them so WARN() will complain the absence of release().

Fix it by:
- Pre-assign dev->release() before potential error path.
- Check before ida_free() in dev->release().

By fixing error path of rpmsg_eptdev_add() and fixing potential memory leak in rpmsg_anonymous_eptdev_create(), this work paves the way of rework of rpmsg_eptdev_add() and its callers.

Fixes c0cdc19f84a4 ("rpmsg: Driver for user space endpoint interface")

Signed-off-by Dawei Li <[email protected]>

---
   drivers/rpmsg/rpmsg_char.c | 26 +++++++++++++-------------
   1 file changed, 13 insertions(+), 13 deletions(-)

drivers/rpmsg/rpmsg_char.c +11 -6

index 34b35ea74aab..373b627581e8 100644
@@ -408,8 +408,13 @@ static void rpmsg_eptdev_release_device(struct device *dev)
   {
   	struct rpmsg_eptdev *eptdev = dev_to_eptdev(dev);
-	ida_free(&rpmsg_ept_ida, dev->id);
-	if (eptdev->dev.devt)
+	/*
+	 * release() can be invoked from error path of rpmsg_eptdev_add(),
+	 * WARN() will be fired if ida_free() is feed with invalid ID.
+	 */
+	if (likely(ida_exists(&rpmsg_ept_ida, dev->id)))
+		ida_free(&rpmsg_ept_ida, dev->id);
+	if (eptdev->dev.devt && likely(ida_exists(&rpmsg_minor_ida, MINOR(eptdev->dev.devt))))
   		ida_free(&rpmsg_minor_ida, MINOR(eptdev->dev.devt));
   	kfree(eptdev);
   }

@@ -458,6 +463,8 @@ static int rpmsg_eptdev_add(struct rpmsg_eptdev *eptdev,
   	struct device *dev = &eptdev->dev;
   	int ret;
+	dev->release = rpmsg_eptdev_release_device;
+
   	eptdev->chinfo = chinfo;
   	if (cdev) {

@@ -471,7 +478,7 @@ static int rpmsg_eptdev_add(struct rpmsg_eptdev *eptdev,
   	/* Anonymous inode device still need device name for dev_err() and friends */
   	ret = ida_alloc(&rpmsg_ept_ida, GFP_KERNEL);
   	if (ret < 0)
-		goto free_minor_ida;
+		goto free_eptdev;
   	dev->id = ret;
   	dev_set_name(dev, "rpmsg%d", ret);

@@ -480,22 +487,13 @@ static int rpmsg_eptdev_add(struct rpmsg_eptdev *eptdev,
   	if (cdev) {
   		ret = cdev_device_add(&eptdev->cdev, &eptdev->dev);
   		if (ret)
-			goto free_ept_ida;
+			goto free_eptdev;
   	}
-	/* We can now rely on the release function for cleanup */
-	dev->release = rpmsg_eptdev_release_device;

drivers/rpmsg/rpmsg_char.c static int rpmsg_eptdev_add(struct rpmsg_eptdev *eptdev, · line 496

Quoted 10 lines

drivers/rpmsg/rpmsg_char.c +0 -7

-
   	return ret;
-free_ept_ida:
-	ida_free(&rpmsg_ept_ida, dev->id);
-free_minor_ida:
-	if (cdev)
-		ida_free(&rpmsg_minor_ida, MINOR(dev->devt));
   free_eptdev:
   	put_device(dev);
-	kfree(eptdev);

Hi Dawei,

Thanks for your new version~

Patch 1/3 will introduce a use-after-free of eptdev in func rpmsg_anonymous_eptdev_create(),

Quoted 4 lines

https://git.kernel.org/pub/scm/linux/kernel/git/remoteproc/linux.git/tree/drivers/rpmsg/rpmsg_char.c?h=for-next#n548

even though this issue will be resolved in 2/3. However, 1/3, as an independent commit, should not introduce a new bug.

FWIW, it's not new bug introduced by this commit, it's introduced by 2410558f5f11, which is supposed to be fixed in patch[2/3].

And new reorganize of series is trying to address the comments from Mathieu[1], If I understand it correctly.

Bjorn, Mathieu,
What's your inputs on this? I will respin v5 if you find it necessary.

Diff between v5 from v4:
patch[1/3]: Remove dev_err(&eptdev->dev) from rpmsg_anonymous_eptdev_create();

patch[2/3]: Bring back dev_err(&eptdev->dev) to rpmsg_anonymous_eptdev_create();

[1] https://lore.kernel.org/all/aRd8mDzQWXtEFnmt@p14s/

Quoted 16 lines

Quoted 11 lines

drivers/rpmsg/rpmsg_char.c +2 -0

   	return ret;
   }

@@ -561,6 +559,8 @@ int rpmsg_anonymous_eptdev_create(struct rpmsg_device *rpdev, struct device *par
   	if (!ret)
   		*pfd = fd;
+	else
+		put_device(&eptdev->dev);
   	return ret;
   }

Signature · 2 lines

Thx and BRs,
Zhongqiu Han

Thanks,

Dawei

For every caller of rpmsg_eptdev_add(), eptdev is allocated in rpmsg_eptdev_alloc(). Resource should be freed where it's allocated, In this case, eptdev should be freed in caller of rpmsg_eptdev_add() rather than rpmsg_eptdev_add() itself.

Move the error handling from rpmsg_eptdev_add() to its callers.

Fixes 2410558f5f11 ("rpmsg: char: Implement eptdev based on anonymous inode")

Signed-off-by Dawei Li <[email protected]>

---
  drivers/rpmsg/rpmsg_char.c | 26 +++++++++++++++-----------
  1 file changed, 15 insertions(+), 11 deletions(-)

drivers/rpmsg/rpmsg_char.c +15 -11

index 373b627581e8..56371899212f 100644
@@ -470,7 +470,7 @@ static int rpmsg_eptdev_add(struct rpmsg_eptdev *eptdev,
  	if (cdev) {
  		ret = ida_alloc_max(&rpmsg_minor_ida, RPMSG_DEV_MAX - 1, GFP_KERNEL);
  		if (ret < 0)
-			goto free_eptdev;
+			return ret;
 
  		dev->devt = MKDEV(MAJOR(rpmsg_major), ret);
  	}

@@ -478,7 +478,7 @@ static int rpmsg_eptdev_add(struct rpmsg_eptdev *eptdev,
  	/* Anonymous inode device still need device name for dev_err() and friends */
  	ret = ida_alloc(&rpmsg_ept_ida, GFP_KERNEL);
  	if (ret < 0)
-		goto free_eptdev;
+		return ret;
  	dev->id = ret;
  	dev_set_name(dev, "rpmsg%d", ret);
 

@@ -486,15 +486,8 @@ static int rpmsg_eptdev_add(struct rpmsg_eptdev *eptdev,
 
  	if (cdev) {
  		ret = cdev_device_add(&eptdev->cdev, &eptdev->dev);
-		if (ret)
-			goto free_eptdev;
  	}
 
-	return ret;
-
-free_eptdev:
-	put_device(dev);
-
  	return ret;
  }
 

@@ -507,12 +500,17 @@ int rpmsg_chrdev_eptdev_create(struct rpmsg_device *rpdev, struct device *parent
  			       struct rpmsg_channel_info chinfo)
  {
  	struct rpmsg_eptdev *eptdev;
+	int ret;
 
  	eptdev = rpmsg_chrdev_eptdev_alloc(rpdev, parent);
  	if (IS_ERR(eptdev))
  		return PTR_ERR(eptdev);
 
-	return rpmsg_chrdev_eptdev_add(eptdev, chinfo);
+	ret = rpmsg_chrdev_eptdev_add(eptdev, chinfo);
+	if (ret)
+		put_device(&eptdev->dev);
+
+	return ret;
  }
  EXPORT_SYMBOL(rpmsg_chrdev_eptdev_create);
 

@@ -544,6 +542,7 @@ int rpmsg_anonymous_eptdev_create(struct rpmsg_device *rpdev, struct device *par
  	ret =  rpmsg_eptdev_add(eptdev, chinfo, false);
  	if (ret) {
  		dev_err(&eptdev->dev, "failed to add %s\n", eptdev->chinfo.name);
+		put_device(&eptdev->dev);
  		return ret;
  	}
 

@@ -571,6 +570,7 @@ static int rpmsg_chrdev_probe(struct rpmsg_device *rpdev)
  	struct rpmsg_channel_info chinfo;
  	struct rpmsg_eptdev *eptdev;
  	struct device *dev = &rpdev->dev;
+	int ret;
 
  	memcpy(chinfo.name, rpdev->id.name, RPMSG_NAME_SIZE);
  	chinfo.src = rpdev->src;

@@ -589,7 +589,11 @@ static int rpmsg_chrdev_probe(struct rpmsg_device *rpdev)
  	 */
  	eptdev->default_ept->priv = eptdev;
 
-	return rpmsg_chrdev_eptdev_add(eptdev, chinfo);
+	ret = rpmsg_chrdev_eptdev_add(eptdev, chinfo);
+	if (ret)
+		put_device(&eptdev->dev);
+
+	return ret;
  }
 
  static void rpmsg_chrdev_remove(struct rpmsg_device *rpdev)

put_device() is called on error path of rpmsg_eptdev_add() to cleanup resource attached to eptdev->dev, unfortunately it's bogus cause dev->release() is not set yet.

When a struct device instance is destroyed, driver core framework checks the possible release() callback from candidates below:
- struct device::release()
- dev->type->release()
- dev->class->dev_release()

Rpmsg eptdev owns none of them so WARN() will complain the absence of release().

Fix it by:
- Pre-assign dev->release() before potential error path.
- Check before ida_free() in dev->release().

By fixing error path of rpmsg_eptdev_add() and fixing potential memory leak in rpmsg_anonymous_eptdev_create(), this work paves the way of rework of rpmsg_eptdev_add() and its callers.

Fixes c0cdc19f84a4 ("rpmsg: Driver for user space endpoint interface")

Signed-off-by Dawei Li <[email protected]>

---
 drivers/rpmsg/rpmsg_char.c | 26 +++++++++++++-------------
 1 file changed, 13 insertions(+), 13 deletions(-)

drivers/rpmsg/rpmsg_char.c +7 -2

index 34b35ea74aab..373b627581e8 100644
@@ -408,8 +408,13 @@ static void rpmsg_eptdev_release_device(struct device *dev)
 {
 	struct rpmsg_eptdev *eptdev = dev_to_eptdev(dev);
 
-	ida_free(&rpmsg_ept_ida, dev->id);
-	if (eptdev->dev.devt)
+	/*
+	 * release() can be invoked from error path of rpmsg_eptdev_add(),
+	 * WARN() will be fired if ida_free() is feed with invalid ID.
+	 */
+	if (likely(ida_exists(&rpmsg_ept_ida, dev->id)))
+		ida_free(&rpmsg_ept_ida, dev->id);
+	if (eptdev->dev.devt && likely(ida_exists(&rpmsg_minor_ida, MINOR(eptdev->dev.devt))))
 		ida_free(&rpmsg_minor_ida, MINOR(eptdev->dev.devt));
 	kfree(eptdev);
 }

drivers/rpmsg/rpmsg_char.c +2 -5

 	eptdev->chinfo = chinfo;
 
 	if (cdev) {

@@ -471,7 +478,7 @@ static int rpmsg_eptdev_add(struct rpmsg_eptdev *eptdev,
 	/* Anonymous inode device still need device name for dev_err() and friends */
 	ret = ida_alloc(&rpmsg_ept_ida, GFP_KERNEL);
 	if (ret < 0)
-		goto free_minor_ida;
+		goto free_eptdev;
 	dev->id = ret;
 	dev_set_name(dev, "rpmsg%d", ret);
 

@@ -480,22 +487,13 @@ static int rpmsg_eptdev_add(struct rpmsg_eptdev *eptdev,
 	if (cdev) {
 		ret = cdev_device_add(&eptdev->cdev, &eptdev->dev);
 		if (ret)
-			goto free_ept_ida;
+			goto free_eptdev;
 	}
 
-	/* We can now rely on the release function for cleanup */
-	dev->release = rpmsg_eptdev_release_device;
-

drivers/rpmsg/rpmsg_char.c +2 -0

 	return ret;
 }

@@ -561,6 +559,8 @@ int rpmsg_anonymous_eptdev_create(struct rpmsg_device *rpdev, struct device *par
 
 	if (!ret)
 		*pfd = fd;
+	else
+		put_device(&eptdev->dev);
 
 	return ret;
 }

Signature · 1 line

2.25.1