diff mbox series

[V1,3/3] rpmsg: glink: Add lock for ctrl device

Message ID 1643223886-28170-4-git-send-email-quic_deesin@quicinc.com (mailing list archive)
State Changes Requested
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 
Race between rpmsg_eptdev_create and rpmsg_chrdev_remove
can sometime casue crash while accessing rpdev while new
endpoint is being created. Using lock ensure no new eptdev
is created after rpmsg_chrdev_remove has been completed.
---
 drivers/rpmsg/rpmsg_char.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

Comments

Bjorn Andersson March 11, 2022, 8:54 p.m. UTC | #1 
On Wed 26 Jan 13:04 CST 2022, Deepak Kumar Singh wrote:

> Race between rpmsg_eptdev_create and rpmsg_chrdev_remove
> can sometime casue crash while accessing rpdev while new
> endpoint is being created. Using lock ensure no new eptdev
> is created after rpmsg_chrdev_remove has been completed.

This patch lacks a Signed-off-by.

Isn't this solving the same problem as the previous patch? Would be nice
with some more specifics on the race that you're seeing.

Thanks,
Bjorn

> ---
>  drivers/rpmsg/rpmsg_char.c | 11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/rpmsg/rpmsg_char.c b/drivers/rpmsg/rpmsg_char.c
> index 2108ef8..3e5b85d 100644
> --- a/drivers/rpmsg/rpmsg_char.c
> +++ b/drivers/rpmsg/rpmsg_char.c
> @@ -27,6 +27,7 @@
>  
>  static dev_t rpmsg_major;
>  static struct class *rpmsg_class;
> +struct mutex ctrl_lock;
>  
>  static DEFINE_IDA(rpmsg_ctrl_ida);
>  static DEFINE_IDA(rpmsg_ept_ida);
> @@ -396,9 +397,12 @@ static int rpmsg_eptdev_create(struct rpmsg_ctrldev *ctrldev,
>  	struct device *dev;
>  	int ret;
>  
> +	mutex_lock(&ctrl_lock);
>  	eptdev = kzalloc(sizeof(*eptdev), GFP_KERNEL);
> -	if (!eptdev)
> +	if (!eptdev) {
> +		mutex_unlock(&ctrl_lock);
>  		return -ENOMEM;
> +	}
>  
>  	dev = &eptdev->dev;
>  	eptdev->rpdev = rpdev;
> @@ -443,6 +447,7 @@ static int rpmsg_eptdev_create(struct rpmsg_ctrldev *ctrldev,
>  		put_device(dev);
>  	}
>  
> +	mutex_unlock(&ctrl_lock);
>  	return ret;
>  
>  free_ept_ida:
> @@ -453,6 +458,7 @@ static int rpmsg_eptdev_create(struct rpmsg_ctrldev *ctrldev,
>  	put_device(dev);
>  	kfree(eptdev);
>  
> +	mutex_unlock(&ctrl_lock);
>  	return ret;
>  }
>  
> @@ -525,6 +531,7 @@ static int rpmsg_chrdev_probe(struct rpmsg_device *rpdev)
>  	if (!ctrldev)
>  		return -ENOMEM;
>  
> +	mutex_init(&ctrl_lock);
>  	ctrldev->rpdev = rpdev;
>  
>  	dev = &ctrldev->dev;
> @@ -581,12 +588,14 @@ static void rpmsg_chrdev_remove(struct rpmsg_device *rpdev)
>  	int ret;
>  
>  	/* Destroy all endpoints */
> +	mutex_lock(&ctrl_lock);
>  	ret = device_for_each_child(&ctrldev->dev, NULL, rpmsg_eptdev_destroy);
>  	if (ret)
>  		dev_warn(&rpdev->dev, "failed to nuke endpoints: %d\n", ret);
>  
>  	device_del(&ctrldev->dev);
>  	put_device(&ctrldev->dev);
> +	mutex_unlock(&ctrl_lock);
>  }
>  
>  static struct rpmsg_driver rpmsg_chrdev_driver = {
> -- 
> 2.7.4
>
Deepak Kumar Singh April 6, 2022, 11:38 a.m. UTC | #2 
On 3/12/2022 2:24 AM, Bjorn Andersson wrote:
> On Wed 26 Jan 13:04 CST 2022, Deepak Kumar Singh wrote:
>
>> Race between rpmsg_eptdev_create and rpmsg_chrdev_remove
>> can sometime casue crash while accessing rpdev while new
>> endpoint is being created. Using lock ensure no new eptdev
>> is created after rpmsg_chrdev_remove has been completed.
> This patch lacks a Signed-off-by.
I will correct that in next patch.
> Isn't this solving the same problem as the previous patch? Would be nice
> with some more specifics on the race that you're seeing.
>
> Thanks,
> Bjorn

Issue was observed after having patch 2, in reboot test case.

Here observation was, user space daemon was able to create rpmsg0 device 
through

ctrl device and it was in process of rpmsg_eptdev_create() but as such 
ept creation was not yet done.

At the same time rpmsg_chrdev_remove() call happened which caused ctrl 
device to be freed.

backtrace of crash -

rpmsg_create_ept+0x40/0xa0
rpmsg_eptdev_open+0x88/0x138
chrdev_open+0xc4/0x1c8
do_dentry_open+0x230/0x378
vfs_open+0x3c/0x48
path_openat+0x93c/0xa78
do_filp_open+0x98/0x118
do_sys_openat2+0x90/0x220
do_sys_open+0x64/0x8c

>> ---
>>   drivers/rpmsg/rpmsg_char.c | 11 ++++++++++-
>>   1 file changed, 10 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/rpmsg/rpmsg_char.c b/drivers/rpmsg/rpmsg_char.c
>> index 2108ef8..3e5b85d 100644
>> --- a/drivers/rpmsg/rpmsg_char.c
>> +++ b/drivers/rpmsg/rpmsg_char.c
>> @@ -27,6 +27,7 @@
>>   
>>   static dev_t rpmsg_major;
>>   static struct class *rpmsg_class;
>> +struct mutex ctrl_lock;
>>   
>>   static DEFINE_IDA(rpmsg_ctrl_ida);
>>   static DEFINE_IDA(rpmsg_ept_ida);
>> @@ -396,9 +397,12 @@ static int rpmsg_eptdev_create(struct rpmsg_ctrldev *ctrldev,
>>   	struct device *dev;
>>   	int ret;
>>   
>> +	mutex_lock(&ctrl_lock);
>>   	eptdev = kzalloc(sizeof(*eptdev), GFP_KERNEL);
>> -	if (!eptdev)
>> +	if (!eptdev) {
>> +		mutex_unlock(&ctrl_lock);
>>   		return -ENOMEM;
>> +	}
>>   
>>   	dev = &eptdev->dev;
>>   	eptdev->rpdev = rpdev;
>> @@ -443,6 +447,7 @@ static int rpmsg_eptdev_create(struct rpmsg_ctrldev *ctrldev,
>>   		put_device(dev);
>>   	}
>>   
>> +	mutex_unlock(&ctrl_lock);
>>   	return ret;
>>   
>>   free_ept_ida:
>> @@ -453,6 +458,7 @@ static int rpmsg_eptdev_create(struct rpmsg_ctrldev *ctrldev,
>>   	put_device(dev);
>>   	kfree(eptdev);
>>   
>> +	mutex_unlock(&ctrl_lock);
>>   	return ret;
>>   }
>>   
>> @@ -525,6 +531,7 @@ static int rpmsg_chrdev_probe(struct rpmsg_device *rpdev)
>>   	if (!ctrldev)
>>   		return -ENOMEM;
>>   
>> +	mutex_init(&ctrl_lock);
>>   	ctrldev->rpdev = rpdev;
>>   
>>   	dev = &ctrldev->dev;
>> @@ -581,12 +588,14 @@ static void rpmsg_chrdev_remove(struct rpmsg_device *rpdev)
>>   	int ret;
>>   
>>   	/* Destroy all endpoints */
>> +	mutex_lock(&ctrl_lock);
>>   	ret = device_for_each_child(&ctrldev->dev, NULL, rpmsg_eptdev_destroy);
>>   	if (ret)
>>   		dev_warn(&rpdev->dev, "failed to nuke endpoints: %d\n", ret);
>>   
>>   	device_del(&ctrldev->dev);
>>   	put_device(&ctrldev->dev);
>> +	mutex_unlock(&ctrl_lock);
>>   }
>>   
>>   static struct rpmsg_driver rpmsg_chrdev_driver = {
>> -- 
>> 2.7.4
>>
diff mbox series

Patch

diff --git a/drivers/rpmsg/rpmsg_char.c b/drivers/rpmsg/rpmsg_char.c
index 2108ef8..3e5b85d 100644
--- a/drivers/rpmsg/rpmsg_char.c
+++ b/drivers/rpmsg/rpmsg_char.c
@@ -27,6 +27,7 @@ 
 
 static dev_t rpmsg_major;
 static struct class *rpmsg_class;
+struct mutex ctrl_lock;
 
 static DEFINE_IDA(rpmsg_ctrl_ida);
 static DEFINE_IDA(rpmsg_ept_ida);
@@ -396,9 +397,12 @@  static int rpmsg_eptdev_create(struct rpmsg_ctrldev *ctrldev,
 	struct device *dev;
 	int ret;
 
+	mutex_lock(&ctrl_lock);
 	eptdev = kzalloc(sizeof(*eptdev), GFP_KERNEL);
-	if (!eptdev)
+	if (!eptdev) {
+		mutex_unlock(&ctrl_lock);
 		return -ENOMEM;
+	}
 
 	dev = &eptdev->dev;
 	eptdev->rpdev = rpdev;
@@ -443,6 +447,7 @@  static int rpmsg_eptdev_create(struct rpmsg_ctrldev *ctrldev,
 		put_device(dev);
 	}
 
+	mutex_unlock(&ctrl_lock);
 	return ret;
 
 free_ept_ida:
@@ -453,6 +458,7 @@  static int rpmsg_eptdev_create(struct rpmsg_ctrldev *ctrldev,
 	put_device(dev);
 	kfree(eptdev);
 
+	mutex_unlock(&ctrl_lock);
 	return ret;
 }
 
@@ -525,6 +531,7 @@  static int rpmsg_chrdev_probe(struct rpmsg_device *rpdev)
 	if (!ctrldev)
 		return -ENOMEM;
 
+	mutex_init(&ctrl_lock);
 	ctrldev->rpdev = rpdev;
 
 	dev = &ctrldev->dev;
@@ -581,12 +588,14 @@  static void rpmsg_chrdev_remove(struct rpmsg_device *rpdev)
 	int ret;
 
 	/* Destroy all endpoints */
+	mutex_lock(&ctrl_lock);
 	ret = device_for_each_child(&ctrldev->dev, NULL, rpmsg_eptdev_destroy);
 	if (ret)
 		dev_warn(&rpdev->dev, "failed to nuke endpoints: %d\n", ret);
 
 	device_del(&ctrldev->dev);
 	put_device(&ctrldev->dev);
+	mutex_unlock(&ctrl_lock);
 }
 
 static struct rpmsg_driver rpmsg_chrdev_driver = {