linux-kernel - Re: [PATCH v3 3/3] rpmsg: char: Rework exception handling of rpmsg_eptdev

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <59e84220-801c-45e3-a8de-f0536e79ffb9@oss.qualcomm.com>
Date: Fri, 14 Nov 2025 17:53:26 +0800
From: Zhongqiu Han <zhongqiu.han@....qualcomm.com>
To: Dawei Li <dawei.li@...ux.dev>, andersson@...nel.org,
        mathieu.poirier@...aro.org
Cc: linux-remoteproc@...r.kernel.org, linux-kernel@...r.kernel.org,
        set_pte_at@...look.com, Dan Carpenter <dan.carpenter@...aro.org>
Subject: Re: [PATCH v3 3/3] rpmsg: char: Rework exception handling of
 rpmsg_eptdev_add()

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@...aro.org>
> Closes: https://lore.kernel.org/all/aPi6gPZE2_ztOjIW@stanley.mountain/
> 
> Signed-off-by: Dawei Li <dawei.li@...ux.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