Fix for different race conditions observed in rpmsg_char driver
for reboot test of device.
Deepak Kumar Singh (3):
rpmsg: glink: Free device context only when cdev not in use
rpmsg: glink: Add lock to avoid race when rpmsg device is released
rpmsg: glink: Add lock for ctrl device
drivers/rpmsg/rpmsg_char.c | 25 ++++++++++++++++++++++++-
1 file changed, 24 insertions(+), 1 deletion(-)
--
2.7.4
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(+)
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 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
>>