diff mbox series

[V1,2/3] rpmsg: glink: Add lock to avoid race when rpmsg device is released

Message ID 1643223886-28170-3-git-send-email-quic_deesin@quicinc.com (mailing list archive)
State Not Applicable
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 
When remote host goes down glink char device channel is freed,
At the same time user space apps can still try to open rpmsg_char
device which will result in calling rpmsg_create_ept. This may cause
reference to already freed context of glink chardev channel.

Use per ept lock to avoid race between rpmsg_destroy_ept and
rpmsg_destory_ept.
---
 drivers/rpmsg/rpmsg_char.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

Comments

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

> When remote host goes down glink char device channel is freed,
> At the same time user space apps can still try to open rpmsg_char
> device which will result in calling rpmsg_create_ept. This may cause
> reference to already freed context of glink chardev channel.
> 

Hi Deepak,

Could you please be a little bit more specific on the details of where
you're seeing this race? Perhaps I'm just missing something obvious?

> Use per ept lock to avoid race between rpmsg_destroy_ept and
> rpmsg_destory_ept.

I presume one of these should say rpmsg_eptdev_open().

Regards,
Bjorn

> ---
>  drivers/rpmsg/rpmsg_char.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/drivers/rpmsg/rpmsg_char.c b/drivers/rpmsg/rpmsg_char.c
> index 72ee101..2108ef8 100644
> --- a/drivers/rpmsg/rpmsg_char.c
> +++ b/drivers/rpmsg/rpmsg_char.c
> @@ -85,6 +85,7 @@ static int rpmsg_eptdev_destroy(struct device *dev, void *data)
>  	struct rpmsg_eptdev *eptdev = dev_to_eptdev(dev);
>  
>  	mutex_lock(&eptdev->ept_lock);
> +	eptdev->rpdev = NULL;
>  	if (eptdev->ept) {
>  		rpmsg_destroy_ept(eptdev->ept);
>  		eptdev->ept = NULL;
> @@ -145,15 +146,24 @@ static int rpmsg_eptdev_open(struct inode *inode, struct file *filp)
>  
>  	get_device(dev);
>  
> +	mutex_lock(&eptdev->ept_lock);
> +	if (!eptdev->rpdev) {
> +		put_device(dev);
> +		mutex_unlock(&eptdev->ept_lock);
> +		return -ENETRESET;
> +	}
> +
>  	ept = rpmsg_create_ept(rpdev, rpmsg_ept_cb, eptdev, eptdev->chinfo);
>  	if (!ept) {
>  		dev_err(dev, "failed to open %s\n", eptdev->chinfo.name);
> +		mutex_unlock(&eptdev->ept_lock);
>  		put_device(dev);
>  		return -EINVAL;
>  	}
>  
>  	ept->sig_cb = rpmsg_sigs_cb;
>  	eptdev->ept = ept;
> +	mutex_unlock(&eptdev->ept_lock);
>  	filp->private_data = eptdev;
>  
>  	return 0;
> @@ -285,7 +295,9 @@ static __poll_t rpmsg_eptdev_poll(struct file *filp, poll_table *wait)
>  	if (eptdev->sig_pending)
>  		mask |= EPOLLPRI;
>  
> +	mutex_lock(&eptdev->ept_lock);
>  	mask |= rpmsg_poll(eptdev->ept, filp, wait);
> +	mutex_unlock(&eptdev->ept_lock);
>  
>  	return mask;
>  }
> -- 
> 2.7.4
>
Deepak Kumar Singh April 7, 2022, 4:58 p.m. UTC | #2 
On 3/12/2022 2:22 AM, Bjorn Andersson wrote:
> On Wed 26 Jan 13:04 CST 2022, Deepak Kumar Singh wrote:
>
>> When remote host goes down glink char device channel is freed,
>> At the same time user space apps can still try to open rpmsg_char
>> device which will result in calling rpmsg_create_ept. This may cause
>> reference to already freed context of glink chardev channel.
>>
> Hi Deepak,
>
> Could you please be a little bit more specific on the details of where
> you're seeing this race? Perhaps I'm just missing something obvious?

Crash is observed in reboot test case.

Log prints suggested that ept was destroyed just before crash in 
rpmsg_eptdev_create().

Below code was executed before crash -

static int rpmsg_eptdev_destroy(struct device *dev, void *data)
{
     struct rpmsg_eptdev *eptdev = dev_to_eptdev(dev);

     mutex_lock(&eptdev->ept_lock);
     if (eptdev->ept) {
         rpmsg_destroy_ept(eptdev->ept);
         eptdev->ept = NULL;
     }
     mutex_unlock(&eptdev->ept_lock);

     /* wake up any blocked readers */
     wake_up_interruptible(&eptdev->readq);

     device_del(&eptdev->dev);
     put_device(&eptdev->dev);

     return 0;
}

one crash was observed in rpmsg_eptdev_create() and other in 
rpmsg_eptdev_poll() -

1)

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

2)

rpmsg_poll+0x5c/0x80
rpmsg_eptdev_poll+0x84/0xa4
do_sys_poll+0x22c/0x5c8

>> Use per ept lock to avoid race between rpmsg_destroy_ept and
>> rpmsg_destory_ept.
> I presume one of these should say rpmsg_eptdev_open().
yes, i will correct this in next patch.
> Regards,
> Bjorn
>
>> ---
>>   drivers/rpmsg/rpmsg_char.c | 12 ++++++++++++
>>   1 file changed, 12 insertions(+)
>>
>> diff --git a/drivers/rpmsg/rpmsg_char.c b/drivers/rpmsg/rpmsg_char.c
>> index 72ee101..2108ef8 100644
>> --- a/drivers/rpmsg/rpmsg_char.c
>> +++ b/drivers/rpmsg/rpmsg_char.c
>> @@ -85,6 +85,7 @@ static int rpmsg_eptdev_destroy(struct device *dev, void *data)
>>   	struct rpmsg_eptdev *eptdev = dev_to_eptdev(dev);
>>   
>>   	mutex_lock(&eptdev->ept_lock);
>> +	eptdev->rpdev = NULL;
>>   	if (eptdev->ept) {
>>   		rpmsg_destroy_ept(eptdev->ept);
>>   		eptdev->ept = NULL;
>> @@ -145,15 +146,24 @@ static int rpmsg_eptdev_open(struct inode *inode, struct file *filp)
>>   
>>   	get_device(dev);
>>   
>> +	mutex_lock(&eptdev->ept_lock);
>> +	if (!eptdev->rpdev) {
>> +		put_device(dev);
>> +		mutex_unlock(&eptdev->ept_lock);
>> +		return -ENETRESET;
>> +	}
>> +
>>   	ept = rpmsg_create_ept(rpdev, rpmsg_ept_cb, eptdev, eptdev->chinfo);
>>   	if (!ept) {
>>   		dev_err(dev, "failed to open %s\n", eptdev->chinfo.name);
>> +		mutex_unlock(&eptdev->ept_lock);
>>   		put_device(dev);
>>   		return -EINVAL;
>>   	}
>>   
>>   	ept->sig_cb = rpmsg_sigs_cb;
>>   	eptdev->ept = ept;
>> +	mutex_unlock(&eptdev->ept_lock);
>>   	filp->private_data = eptdev;
>>   
>>   	return 0;
>> @@ -285,7 +295,9 @@ static __poll_t rpmsg_eptdev_poll(struct file *filp, poll_table *wait)
>>   	if (eptdev->sig_pending)
>>   		mask |= EPOLLPRI;
>>   
>> +	mutex_lock(&eptdev->ept_lock);
>>   	mask |= rpmsg_poll(eptdev->ept, filp, wait);
>> +	mutex_unlock(&eptdev->ept_lock);
>>   
>>   	return mask;
>>   }
>> -- 
>> 2.7.4
>>
diff mbox series

Patch

diff --git a/drivers/rpmsg/rpmsg_char.c b/drivers/rpmsg/rpmsg_char.c
index 72ee101..2108ef8 100644
--- a/drivers/rpmsg/rpmsg_char.c
+++ b/drivers/rpmsg/rpmsg_char.c
@@ -85,6 +85,7 @@  static int rpmsg_eptdev_destroy(struct device *dev, void *data)
 	struct rpmsg_eptdev *eptdev = dev_to_eptdev(dev);
 
 	mutex_lock(&eptdev->ept_lock);
+	eptdev->rpdev = NULL;
 	if (eptdev->ept) {
 		rpmsg_destroy_ept(eptdev->ept);
 		eptdev->ept = NULL;
@@ -145,15 +146,24 @@  static int rpmsg_eptdev_open(struct inode *inode, struct file *filp)
 
 	get_device(dev);
 
+	mutex_lock(&eptdev->ept_lock);
+	if (!eptdev->rpdev) {
+		put_device(dev);
+		mutex_unlock(&eptdev->ept_lock);
+		return -ENETRESET;
+	}
+
 	ept = rpmsg_create_ept(rpdev, rpmsg_ept_cb, eptdev, eptdev->chinfo);
 	if (!ept) {
 		dev_err(dev, "failed to open %s\n", eptdev->chinfo.name);
+		mutex_unlock(&eptdev->ept_lock);
 		put_device(dev);
 		return -EINVAL;
 	}
 
 	ept->sig_cb = rpmsg_sigs_cb;
 	eptdev->ept = ept;
+	mutex_unlock(&eptdev->ept_lock);
 	filp->private_data = eptdev;
 
 	return 0;
@@ -285,7 +295,9 @@  static __poll_t rpmsg_eptdev_poll(struct file *filp, poll_table *wait)
 	if (eptdev->sig_pending)
 		mask |= EPOLLPRI;
 
+	mutex_lock(&eptdev->ept_lock);
 	mask |= rpmsg_poll(eptdev->ept, filp, wait);
+	mutex_unlock(&eptdev->ept_lock);
 
 	return mask;
 }