[PATCH v3 0/3] Fix and rework of rpmsg_eptdev_add()

[PATCH v3 0/3] Fix and rework of rpmsg_eptdev_add()

[Dawei Li]

Hi,

This series fixes bug introduced by anonymous inode eptdev code and
rework the exception handing paths. 

And it tries to address a long suffering issue which may be backported
to stable kernels. 

Change for v3:
- Split it into 3 patches:
  1/3: Fix legacy bug.
  2/3: Fix new bug introduced by anonymous eptdev code.
  3/3: Rework error handling code.
Link to v2:
https://lore.kernel.org/all/20251112150108.49017-1-dawei.li@linux.dev/

Change for v2:
- Add put_device() when __rpmsg_eptdev_open() failed.
Link to v1:
https://lore.kernel.org/all/20251112142813.33708-1-dawei.li@linux.dev/

Dawei Li (3):
  rpmsg: char: Remove put_device() in rpmsg_eptdev_add()
  rpmsg: char: Fix UAF and memory leak in
    rpmsg_anonymous_eptdev_create()
  rpmsg: char: Rework exception handling of rpmsg_eptdev_add()

 drivers/rpmsg/rpmsg_char.c | 61 +++++++++++++++++++++-----------------
 1 file changed, 34 insertions(+), 27 deletions(-)

-- 
2.25.1

[PATCH v3 1/3] rpmsg: char: Remove put_device() in rpmsg_eptdev_add()

[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 complaint the absence of
release():

[  159.112182] ------------[ cut here ]------------
[  159.112188] Device '(null)' does not have a release() function, it is broken and must be fixed. See Documentation/core-api/kobject.rst.
[  159.112205] WARNING: CPU: 2 PID: 1975 at drivers/base/core.c:2567 device_release+0x7a/0x90

Fixes: c0cdc19f84a4 ("rpmsg: Driver for user space endpoint interface")
Cc: stable@vger.kernel.org
Signed-off-by: Dawei Li <dawei.li@linux.dev>
---
 drivers/rpmsg/rpmsg_char.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/rpmsg/rpmsg_char.c b/drivers/rpmsg/rpmsg_char.c
index 34b35ea74aab..1b8297b373f0 100644
--- a/drivers/rpmsg/rpmsg_char.c
+++ b/drivers/rpmsg/rpmsg_char.c
@@ -494,7 +494,6 @@ static int rpmsg_eptdev_add(struct rpmsg_eptdev *eptdev,
 	if (cdev)
 		ida_free(&rpmsg_minor_ida, MINOR(dev->devt));
 free_eptdev:
-	put_device(dev);
 	kfree(eptdev);
 
 	return ret;
-- 
2.25.1

Re: [PATCH v3 1/3] rpmsg: char: Remove put_device() in rpmsg_eptdev_add()

[Zhongqiu Han]

On 11/13/2025 11:39 PM, Dawei Li wrote:
> 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 complaint the absence of
> release():

Hi Dawei,


> 
> [  159.112182] ------------[ cut here ]------------
> [  159.112188] Device '(null)' does not have a release() function, it is broken and must be fixed. See Documentation/core-api/kobject.rst.
> [  159.112205] WARNING: CPU: 2 PID: 1975 at drivers/base/core.c:2567 device_release+0x7a/0x90
> 


Although my local checkpatch.pl didn’t complain about this log line
exceeding 75 characters, could we simplify it or just provide a summary
instead?


> Fixes: c0cdc19f84a4 ("rpmsg: Driver for user space endpoint interface")
> Cc: stable@vger.kernel.org
> Signed-off-by: Dawei Li <dawei.li@linux.dev>
> ---
>   drivers/rpmsg/rpmsg_char.c | 1 -
>   1 file changed, 1 deletion(-)
> 
> diff --git a/drivers/rpmsg/rpmsg_char.c b/drivers/rpmsg/rpmsg_char.c
> index 34b35ea74aab..1b8297b373f0 100644
> --- a/drivers/rpmsg/rpmsg_char.c
> +++ b/drivers/rpmsg/rpmsg_char.c
> @@ -494,7 +494,6 @@ static int rpmsg_eptdev_add(struct rpmsg_eptdev *eptdev,
>   	if (cdev)
>   		ida_free(&rpmsg_minor_ida, MINOR(dev->devt));
>   free_eptdev:
> -	put_device(dev);


Yes, remove put_device can solve the warning issue, however it would
introduce one memleak issue of kobj->name.

https://git.kernel.org/pub/scm/linux/kernel/git/remoteproc/linux.git/tree/drivers/rpmsg/rpmsg_char.c#n381 


dev_set_name(dev, "rpmsg%d", ret); is already called, it depends on
put_device to free memory, right?


>   	kfree(eptdev);
>   
>   	return ret;


-- 
Thx and BRs,
Zhongqiu Han

Re: [PATCH v3 1/3] rpmsg: char: Remove put_device() in rpmsg_eptdev_add()

[Zhongqiu Han]

On 11/14/2025 5:53 PM, Zhongqiu Han wrote:
> On 11/13/2025 11:39 PM, Dawei Li wrote:
>> 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 complaint the absence of
>> release():
> 
> Hi Dawei,
> 
> 
>>
>> [  159.112182] ------------[ cut here ]------------
>> [  159.112188] Device '(null)' does not have a release() function, it 
>> is broken and must be fixed. See Documentation/core-api/kobject.rst.
>> [  159.112205] WARNING: CPU: 2 PID: 1975 at drivers/base/core.c:2567 
>> device_release+0x7a/0x90
>>
> 
> 
> Although my local checkpatch.pl didn’t complain about this log line
> exceeding 75 characters, could we simplify it or just provide a summary
> instead?
> 
> 
>> Fixes: c0cdc19f84a4 ("rpmsg: Driver for user space endpoint interface")
>> Cc: stable@vger.kernel.org
>> Signed-off-by: Dawei Li <dawei.li@linux.dev>
>> ---
>>   drivers/rpmsg/rpmsg_char.c | 1 -
>>   1 file changed, 1 deletion(-)
>>
>> diff --git a/drivers/rpmsg/rpmsg_char.c b/drivers/rpmsg/rpmsg_char.c
>> index 34b35ea74aab..1b8297b373f0 100644
>> --- a/drivers/rpmsg/rpmsg_char.c
>> +++ b/drivers/rpmsg/rpmsg_char.c
>> @@ -494,7 +494,6 @@ static int rpmsg_eptdev_add(struct rpmsg_eptdev 
>> *eptdev,
>>       if (cdev)
>>           ida_free(&rpmsg_minor_ida, MINOR(dev->devt));
>>   free_eptdev:
>> -    put_device(dev);
> 
> 
> Yes, remove put_device can solve the warning issue, however it would
> introduce one memleak issue of kobj->name.
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/remoteproc/linux.git/ 
> tree/drivers/rpmsg/rpmsg_char.c#n381
> 

The above link I arised was wrong; it’s now updated to the correct one.

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


> dev_set_name(dev, "rpmsg%d", ret); is already called, it depends on
> put_device to free memory, right?
> 
> 
>>       kfree(eptdev);
>>       return ret;
> 
> 


-- 
Thx and BRs,
Zhongqiu Han

Re: [PATCH v3 1/3] rpmsg: char: Remove put_device() in rpmsg_eptdev_add()

[Dawei Li]

Hi,

Thanks for the review.

On Fri, Nov 14, 2025 at 05:53:14PM +0800, Zhongqiu Han wrote:
> On 11/13/2025 11:39 PM, Dawei Li wrote:
> > 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 complaint the absence of
> > release():
> 
> Hi Dawei,
> 
> 
> > 
> > [  159.112182] ------------[ cut here ]------------
> > [  159.112188] Device '(null)' does not have a release() function, it is broken and must be fixed. See Documentation/core-api/kobject.rst.
> > [  159.112205] WARNING: CPU: 2 PID: 1975 at drivers/base/core.c:2567 device_release+0x7a/0x90
> > 
> 
> 
> Although my local checkpatch.pl didn’t complain about this log line
> exceeding 75 characters, could we simplify it or just provide a summary
> instead?
> 
> 
> > Fixes: c0cdc19f84a4 ("rpmsg: Driver for user space endpoint interface")
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Dawei Li <dawei.li@linux.dev>
> > ---
> >   drivers/rpmsg/rpmsg_char.c | 1 -
> >   1 file changed, 1 deletion(-)
> > 
> > diff --git a/drivers/rpmsg/rpmsg_char.c b/drivers/rpmsg/rpmsg_char.c
> > index 34b35ea74aab..1b8297b373f0 100644
> > --- a/drivers/rpmsg/rpmsg_char.c
> > +++ b/drivers/rpmsg/rpmsg_char.c
> > @@ -494,7 +494,6 @@ static int rpmsg_eptdev_add(struct rpmsg_eptdev *eptdev,
> >   	if (cdev)
> >   		ida_free(&rpmsg_minor_ida, MINOR(dev->devt));
> >   free_eptdev:
> > -	put_device(dev);
> 
> 
> Yes, remove put_device can solve the warning issue, however it would
> introduce one memleak issue of kobj->name.


> 
> https://git.kernel.org/pub/scm/linux/kernel/git/remoteproc/linux.git/tree/drivers/rpmsg/rpmsg_char.c#n381
> 
> 
> dev_set_name(dev, "rpmsg%d", ret); is already called, it depends on
> put_device to free memory, right?

Good catch.

If it's just device name being leaked, just postpone dev_set_name till
every resource was allocated successfully. 

[Copying your comment on patch3/3]

> As I mentioned about the potential memory leak issue in patch 1/3, we
> could consider still using put_device for management, as this better
> aligns with the driver model standards and avoids potential issue.
> However, this requires assigning the release function in advance and
> also handling the special case where ida allocation fails in
> rpmsg_eptdev_add (removing the manual ida release).

But I agree with you, every data structure embedding struct device
should bind its life cycle management to struct devcice, that's what
driver core is designed. But it's bit tricky to implement your proposed
approach, especially considering backing port to stable kernel. A
possible solution could be:

diff --git a/drivers/rpmsg/rpmsg_char.c b/drivers/rpmsg/rpmsg_char.c
index 34b35ea74aab..e223a5452a75 100644
--- a/drivers/rpmsg/rpmsg_char.c
+++ b/drivers/rpmsg/rpmsg_char.c
@@ -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 invaid 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;
-
        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);
 
        return ret;
 }

ida_exists() is introduced in 7fe6b987166b9, which is beyond the
coverage of every stable kernel, and the commit this patch is fixing
(c0cdc19f84a4) is contained in almost every stable kernel maintained.

Thanks,

	Dawei

> 
> 
> >   	kfree(eptdev);
> >   	return ret;
> 
> 
> -- 
> Thx and BRs,
> Zhongqiu Han

[PATCH v3 2/3] rpmsg: char: Fix UAF and memory leak in rpmsg_anonymous_eptdev_create()

[Dawei Li]

Potential UAF and memory leak exsit in exception handling paths for
rpmsg_anonymous_eptdev_create(), fix them.

- If rpmsg_add_eptdev() failes, eptdev is freed in it.
  Subsequent call of dev_err(&eptdev->device) triggers a UAF.
- If __rpmsg_eptdev_open() fails, eptdev is supposed to be freed by
  put_device().

Fixes: 2410558f5f11 ("rpmsg: char: Implement eptdev based on anonymous inode")
Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
Closes: https://lore.kernel.org/all/aPi6gPZE2_ztOjIW@stanley.mountain/
Signed-off-by: Dawei Li <dawei.li@linux.dev>
---
 drivers/rpmsg/rpmsg_char.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/rpmsg/rpmsg_char.c b/drivers/rpmsg/rpmsg_char.c
index 1b8297b373f0..0919ad0a19df 100644
--- a/drivers/rpmsg/rpmsg_char.c
+++ b/drivers/rpmsg/rpmsg_char.c
@@ -494,6 +494,7 @@ static int rpmsg_eptdev_add(struct rpmsg_eptdev *eptdev,
 	if (cdev)
 		ida_free(&rpmsg_minor_ida, MINOR(dev->devt));
 free_eptdev:
+	dev_err(&eptdev->dev, "failed to add %s\n", eptdev->chinfo.name);
 	kfree(eptdev);
 
 	return ret;
@@ -544,7 +545,6 @@ 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);
 		return ret;
 	}
 
@@ -560,6 +560,8 @@ int rpmsg_anonymous_eptdev_create(struct rpmsg_device *rpdev, struct device *par
 
 	if (!ret)
 		*pfd = fd;
+	else
+		put_device(&eptdev->dev);
 
 	return ret;
 }
-- 
2.25.1

Re: [PATCH v3 2/3] rpmsg: char: Fix UAF and memory leak in rpmsg_anonymous_eptdev_create()

[Mathieu Poirier]

On Thu, Nov 13, 2025 at 11:39:08PM +0800, Dawei Li wrote:
> Potential UAF and memory leak exsit in exception handling paths for
> rpmsg_anonymous_eptdev_create(), fix them.
> 
> - If rpmsg_add_eptdev() failes, eptdev is freed in it.
>   Subsequent call of dev_err(&eptdev->device) triggers a UAF.
> - If __rpmsg_eptdev_open() fails, eptdev is supposed to be freed by
>   put_device().
> 
> Fixes: 2410558f5f11 ("rpmsg: char: Implement eptdev based on anonymous inode")
> Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
> Closes: https://lore.kernel.org/all/aPi6gPZE2_ztOjIW@stanley.mountain/
> Signed-off-by: Dawei Li <dawei.li@linux.dev>
> ---
>  drivers/rpmsg/rpmsg_char.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/rpmsg/rpmsg_char.c b/drivers/rpmsg/rpmsg_char.c
> index 1b8297b373f0..0919ad0a19df 100644
> --- a/drivers/rpmsg/rpmsg_char.c
> +++ b/drivers/rpmsg/rpmsg_char.c
> @@ -494,6 +494,7 @@ static int rpmsg_eptdev_add(struct rpmsg_eptdev *eptdev,
>  	if (cdev)
>  		ida_free(&rpmsg_minor_ida, MINOR(dev->devt));
>  free_eptdev:
> +	dev_err(&eptdev->dev, "failed to add %s\n", eptdev->chinfo.name);
>  	kfree(eptdev);
>  
>  	return ret;
> @@ -544,7 +545,6 @@ 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);

This doesn't neet to be done if kfree(eptdev) is moved out of
rpmsg_eptdev_add().

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

This goes in the previous patch.

>  
>  	return ret;
>  }
> -- 
> 2.25.1
> 

[PATCH v3 3/3] rpmsg: char: Rework exception handling of rpmsg_eptdev_add()

[Dawei Li]

Rework  error handling of rpmsg_eptdev_add() and its callers, following
rule of "release resource where it's allocated".

Fixes: 2410558f5f11 ("rpmsg: char: Implement eptdev based on anonymous inode")
Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
Closes: https://lore.kernel.org/all/aPi6gPZE2_ztOjIW@stanley.mountain/

Signed-off-by: Dawei Li <dawei.li@linux.dev>
---
 drivers/rpmsg/rpmsg_char.c | 60 +++++++++++++++++++++-----------------
 1 file changed, 33 insertions(+), 27 deletions(-)

diff --git a/drivers/rpmsg/rpmsg_char.c b/drivers/rpmsg/rpmsg_char.c
index 0919ad0a19df..92c176e9b0e4 100644
--- a/drivers/rpmsg/rpmsg_char.c
+++ b/drivers/rpmsg/rpmsg_char.c
@@ -460,44 +460,34 @@ static int rpmsg_eptdev_add(struct rpmsg_eptdev *eptdev,
 
 	eptdev->chinfo = chinfo;
 
-	if (cdev) {
-		ret = ida_alloc_max(&rpmsg_minor_ida, RPMSG_DEV_MAX - 1, GFP_KERNEL);
-		if (ret < 0)
-			goto free_eptdev;
-
-		dev->devt = MKDEV(MAJOR(rpmsg_major), ret);
-	}
-
 	/* 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;
+		return ret;
 	dev->id = ret;
 	dev_set_name(dev, "rpmsg%d", ret);
 
-	ret = 0;
-
 	if (cdev) {
+		ret = ida_alloc_max(&rpmsg_minor_ida, RPMSG_DEV_MAX - 1, GFP_KERNEL);
+		if (ret < 0) {
+			ida_free(&rpmsg_ept_ida, dev->id);
+			return ret;
+		}
+
+		dev->devt = MKDEV(MAJOR(rpmsg_major), ret);
+
 		ret = cdev_device_add(&eptdev->cdev, &eptdev->dev);
-		if (ret)
-			goto free_ept_ida;
+		if (ret) {
+			ida_free(&rpmsg_ept_ida, dev->id);
+			ida_free(&rpmsg_minor_ida, MINOR(dev->devt));
+			return ret;
+		}
 	}
 
 	/* We can now rely on the release function for cleanup */
 	dev->release = rpmsg_eptdev_release_device;
 
-	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:
-	dev_err(&eptdev->dev, "failed to add %s\n", eptdev->chinfo.name);
-	kfree(eptdev);
-
-	return ret;
+	return 0;
 }
 
 static int rpmsg_chrdev_eptdev_add(struct rpmsg_eptdev *eptdev, struct rpmsg_channel_info chinfo)
@@ -509,12 +499,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)
+		kfree(eptdev);
+
+	return ret;
 }
 EXPORT_SYMBOL(rpmsg_chrdev_eptdev_create);
 
@@ -545,6 +540,12 @@ 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);
+		/*
+		 * Avoid put_device() or WARN() will be triggered due to absence of
+		 * device::release(), refer to device_release().
+		 */
+		kfree(eptdev);
 		return ret;
 	}
 
@@ -572,6 +573,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;
@@ -590,7 +592,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)
+		kfree(eptdev);
+
+	return ret;
 }
 
 static void rpmsg_chrdev_remove(struct rpmsg_device *rpdev)
-- 
2.25.1

Re: [PATCH v3 3/3] rpmsg: char: Rework exception handling of rpmsg_eptdev_add()

[Zhongqiu Han]

On 11/13/2025 11:39 PM, Dawei Li wrote:
> Rework  error handling of rpmsg_eptdev_add() and its callers, following
> rule of "release resource where it's allocated".
> 
> Fixes: 2410558f5f11 ("rpmsg: char: Implement eptdev based on anonymous inode")
> Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
> Closes: https://lore.kernel.org/all/aPi6gPZE2_ztOjIW@stanley.mountain/
> 
> Signed-off-by: Dawei Li <dawei.li@linux.dev>
> ---
>   drivers/rpmsg/rpmsg_char.c | 60 +++++++++++++++++++++-----------------
>   1 file changed, 33 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/rpmsg/rpmsg_char.c b/drivers/rpmsg/rpmsg_char.c
> index 0919ad0a19df..92c176e9b0e4 100644
> --- a/drivers/rpmsg/rpmsg_char.c
> +++ b/drivers/rpmsg/rpmsg_char.c
> @@ -460,44 +460,34 @@ static int rpmsg_eptdev_add(struct rpmsg_eptdev *eptdev,
>   
>   	eptdev->chinfo = chinfo;
>   
> -	if (cdev) {
> -		ret = ida_alloc_max(&rpmsg_minor_ida, RPMSG_DEV_MAX - 1, GFP_KERNEL);
> -		if (ret < 0)
> -			goto free_eptdev;
> -
> -		dev->devt = MKDEV(MAJOR(rpmsg_major), ret);
> -	}
> -
>   	/* 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;
> +		return ret;
>   	dev->id = ret;
>   	dev_set_name(dev, "rpmsg%d", ret);
>   
> -	ret = 0;
> -
>   	if (cdev) {
> +		ret = ida_alloc_max(&rpmsg_minor_ida, RPMSG_DEV_MAX - 1, GFP_KERNEL);
> +		if (ret < 0) {
> +			ida_free(&rpmsg_ept_ida, dev->id);
> +			return ret;
> +		}
> +
> +		dev->devt = MKDEV(MAJOR(rpmsg_major), ret);
> +
>   		ret = cdev_device_add(&eptdev->cdev, &eptdev->dev);
> -		if (ret)
> -			goto free_ept_ida;
> +		if (ret) {
> +			ida_free(&rpmsg_ept_ida, dev->id);
> +			ida_free(&rpmsg_minor_ida, MINOR(dev->devt));
> +			return ret;
> +		}
>   	}
>   
>   	/* We can now rely on the release function for cleanup */
>   	dev->release = rpmsg_eptdev_release_device;
>   
> -	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:
> -	dev_err(&eptdev->dev, "failed to add %s\n", eptdev->chinfo.name);
> -	kfree(eptdev);
> -
> -	return ret;
> +	return 0;
>   }
>   
>   static int rpmsg_chrdev_eptdev_add(struct rpmsg_eptdev *eptdev, struct rpmsg_channel_info chinfo)
> @@ -509,12 +499,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)
> +		kfree(eptdev);
> +
> +	return ret;
>   }
>   EXPORT_SYMBOL(rpmsg_chrdev_eptdev_create);
>   
> @@ -545,6 +540,12 @@ 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);
> +		/*
> +		 * Avoid put_device() or WARN() will be triggered due to absence of
> +		 * device::release(), refer to device_release().

Hi Dawei,

As I mentioned about the potential memory leak issue in patch 1/3, we
could consider still using put_device for management, as this better
aligns with the driver model standards and avoids potential issue.
However, this requires assigning the release function in advance and
also handling the special case where ida allocation fails in
rpmsg_eptdev_add (removing the manual ida release).



> +		 */
> +		kfree(eptdev);
>   		return ret;
>   	}
>   
> @@ -572,6 +573,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;
> @@ -590,7 +592,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)
> +		kfree(eptdev);
> +
> +	return ret;
>   }
>   
>   static void rpmsg_chrdev_remove(struct rpmsg_device *rpdev)


-- 
Thx and BRs,
Zhongqiu Han

Re: [PATCH v3 3/3] rpmsg: char: Rework exception handling of rpmsg_eptdev_add()

[Mathieu Poirier]

On Thu, Nov 13, 2025 at 11:39:09PM +0800, Dawei Li wrote:
> Rework  error handling of rpmsg_eptdev_add() and its callers, following
> rule of "release resource where it's allocated".
> 
> Fixes: 2410558f5f11 ("rpmsg: char: Implement eptdev based on anonymous inode")
> Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
> Closes: https://lore.kernel.org/all/aPi6gPZE2_ztOjIW@stanley.mountain/
> 
> Signed-off-by: Dawei Li <dawei.li@linux.dev>
> ---
>  drivers/rpmsg/rpmsg_char.c | 60 +++++++++++++++++++++-----------------
>  1 file changed, 33 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/rpmsg/rpmsg_char.c b/drivers/rpmsg/rpmsg_char.c
> index 0919ad0a19df..92c176e9b0e4 100644
> --- a/drivers/rpmsg/rpmsg_char.c
> +++ b/drivers/rpmsg/rpmsg_char.c
> @@ -460,44 +460,34 @@ static int rpmsg_eptdev_add(struct rpmsg_eptdev *eptdev,
>  
>  	eptdev->chinfo = chinfo;
>  
> -	if (cdev) {
> -		ret = ida_alloc_max(&rpmsg_minor_ida, RPMSG_DEV_MAX - 1, GFP_KERNEL);
> -		if (ret < 0)
> -			goto free_eptdev;
> -
> -		dev->devt = MKDEV(MAJOR(rpmsg_major), ret);
> -	}
> -
>  	/* 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;
> +		return ret;
>  	dev->id = ret;
>  	dev_set_name(dev, "rpmsg%d", ret);
>  
> -	ret = 0;
> -
>  	if (cdev) {
> +		ret = ida_alloc_max(&rpmsg_minor_ida, RPMSG_DEV_MAX - 1, GFP_KERNEL);
> +		if (ret < 0) {
> +			ida_free(&rpmsg_ept_ida, dev->id);
> +			return ret;
> +		}
> +
> +		dev->devt = MKDEV(MAJOR(rpmsg_major), ret);
> +
>  		ret = cdev_device_add(&eptdev->cdev, &eptdev->dev);
> -		if (ret)
> -			goto free_ept_ida;
> +		if (ret) {
> +			ida_free(&rpmsg_ept_ida, dev->id);
> +			ida_free(&rpmsg_minor_ida, MINOR(dev->devt));
> +			return ret;
> +		}
>  	}
>  
>  	/* We can now rely on the release function for cleanup */
>  	dev->release = rpmsg_eptdev_release_device;
>  
> -	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:
> -	dev_err(&eptdev->dev, "failed to add %s\n", eptdev->chinfo.name);
> -	kfree(eptdev);
> -
> -	return ret;
> +	return 0;
>  }
>  
>  static int rpmsg_chrdev_eptdev_add(struct rpmsg_eptdev *eptdev, struct rpmsg_channel_info chinfo)
> @@ -509,12 +499,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)
> +		kfree(eptdev);

Previous patch.

> +
> +	return ret;
>  }
>  EXPORT_SYMBOL(rpmsg_chrdev_eptdev_create);
>  
> @@ -545,6 +540,12 @@ 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);
> +		/*
> +		 * Avoid put_device() or WARN() will be triggered due to absence of
> +		 * device::release(), refer to device_release().
> +		 */
> +		kfree(eptdev);

Previous patch.

>  		return ret;
>  	}
>  
> @@ -572,6 +573,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;
> @@ -590,7 +592,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)
> +		kfree(eptdev);

Previous patch.

> +
> +	return ret;
>  }
>  
>  static void rpmsg_chrdev_remove(struct rpmsg_device *rpdev)
> -- 
> 2.25.1
>