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

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(+)

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
>

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
>>