diff mbox series

[V1,1/3] rpmsg: glink: Free device context only when cdev not in use

Message ID 1643223886-28170-2-git-send-email-quic_deesin@quicinc.com
State New
Headers show
Series rpmsg char fixes for race conditions in device reboot | expand

Commit Message

Deepak Kumar Singh Jan. 26, 2022, 7:04 p.m. UTC
Struct device holding cdev should not be freed unless cdev
is not in use. It is possible that user space has opened
char device while kernel has freed the associated struct
device context.

Mark dev kobj as parent of cdev, so that chardev_add gets
an extra reference to dev. This ensures device context is not
freed until cdev is is not in uses.
---
 drivers/rpmsg/rpmsg_char.c | 2 ++
 1 file changed, 2 insertions(+)

Comments

Mathieu Poirier Feb. 3, 2022, 5:35 p.m. UTC | #1
Hi Deepak,

On Thu, Jan 27, 2022 at 12:34:44AM +0530, Deepak Kumar Singh wrote:
> Struct device holding cdev should not be freed unless cdev
> is not in use. It is possible that user space has opened
> char device while kernel has freed the associated struct
> device context.
> 
> Mark dev kobj as parent of cdev, so that chardev_add gets
> an extra reference to dev. This ensures device context is not
> freed until cdev is is not in uses.
> ---
>  drivers/rpmsg/rpmsg_char.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/rpmsg/rpmsg_char.c b/drivers/rpmsg/rpmsg_char.c
> index c03a118..72ee101 100644
> --- a/drivers/rpmsg/rpmsg_char.c
> +++ b/drivers/rpmsg/rpmsg_char.c
> @@ -417,6 +417,7 @@ static int rpmsg_eptdev_create(struct rpmsg_ctrldev *ctrldev,
>  	dev->id = ret;
>  	dev_set_name(dev, "rpmsg%d", ret);
>  
> +	cdev_set_parent(&eptdev->cdev, &dev->kobj);
>  	ret = cdev_add(&eptdev->cdev, dev->devt, 1);

This issue should have been fixed when cdev_add() was replaced by
cdev_device_add(), something you will find on v5.17-rc2.

Also, this set is generating checkpatch warnings and as such I will not review
the other patches in it. 

Thanks,
Mathieu

>  	if (ret)
>  		goto free_ept_ida;
> @@ -533,6 +534,7 @@ static int rpmsg_chrdev_probe(struct rpmsg_device *rpdev)
>  	dev->id = ret;
>  	dev_set_name(&ctrldev->dev, "rpmsg_ctrl%d", ret);
>  
> +	cdev_set_parent(&ctrldev->cdev, &dev->kobj);
>  	ret = cdev_add(&ctrldev->cdev, dev->devt, 1);
>  	if (ret)
>  		goto free_ctrl_ida;
> -- 
> 2.7.4
>
Deepak Kumar Singh Feb. 14, 2022, 3:02 p.m. UTC | #2
On 2/3/2022 11:05 PM, Mathieu Poirier wrote:
> Hi Deepak,
>
> On Thu, Jan 27, 2022 at 12:34:44AM +0530, Deepak Kumar Singh wrote:
>> Struct device holding cdev should not be freed unless cdev
>> is not in use. It is possible that user space has opened
>> char device while kernel has freed the associated struct
>> device context.
>>
>> Mark dev kobj as parent of cdev, so that chardev_add gets
>> an extra reference to dev. This ensures device context is not
>> freed until cdev is is not in uses.
>> ---
>>   drivers/rpmsg/rpmsg_char.c | 2 ++
>>   1 file changed, 2 insertions(+)
>>
>> diff --git a/drivers/rpmsg/rpmsg_char.c b/drivers/rpmsg/rpmsg_char.c
>> index c03a118..72ee101 100644
>> --- a/drivers/rpmsg/rpmsg_char.c
>> +++ b/drivers/rpmsg/rpmsg_char.c
>> @@ -417,6 +417,7 @@ static int rpmsg_eptdev_create(struct rpmsg_ctrldev *ctrldev,
>>   	dev->id = ret;
>>   	dev_set_name(dev, "rpmsg%d", ret);
>>   
>> +	cdev_set_parent(&eptdev->cdev, &dev->kobj);
>>   	ret = cdev_add(&eptdev->cdev, dev->devt, 1);
> This issue should have been fixed when cdev_add() was replaced by
> cdev_device_add(), something you will find on v5.17-rc2.
>
> Also, this set is generating checkpatch warnings and as such I will not review
> the other patches in it.
>
> Thanks,
> Mathieu

Thank you Mathieu for info!! i will recheck other 2 patches for 
checkpatch warnings.

Thanks,

Deepak

>>   	if (ret)
>>   		goto free_ept_ida;
>> @@ -533,6 +534,7 @@ static int rpmsg_chrdev_probe(struct rpmsg_device *rpdev)
>>   	dev->id = ret;
>>   	dev_set_name(&ctrldev->dev, "rpmsg_ctrl%d", ret);
>>   
>> +	cdev_set_parent(&ctrldev->cdev, &dev->kobj);
>>   	ret = cdev_add(&ctrldev->cdev, dev->devt, 1);
>>   	if (ret)
>>   		goto free_ctrl_ida;
>> -- 
>> 2.7.4
>>
diff mbox series

Patch

diff --git a/drivers/rpmsg/rpmsg_char.c b/drivers/rpmsg/rpmsg_char.c
index c03a118..72ee101 100644
--- a/drivers/rpmsg/rpmsg_char.c
+++ b/drivers/rpmsg/rpmsg_char.c
@@ -417,6 +417,7 @@  static int rpmsg_eptdev_create(struct rpmsg_ctrldev *ctrldev,
 	dev->id = ret;
 	dev_set_name(dev, "rpmsg%d", ret);
 
+	cdev_set_parent(&eptdev->cdev, &dev->kobj);
 	ret = cdev_add(&eptdev->cdev, dev->devt, 1);
 	if (ret)
 		goto free_ept_ida;
@@ -533,6 +534,7 @@  static int rpmsg_chrdev_probe(struct rpmsg_device *rpdev)
 	dev->id = ret;
 	dev_set_name(&ctrldev->dev, "rpmsg_ctrl%d", ret);
 
+	cdev_set_parent(&ctrldev->cdev, &dev->kobj);
 	ret = cdev_add(&ctrldev->cdev, dev->devt, 1);
 	if (ret)
 		goto free_ctrl_ida;