| Message ID | 1662404120-24338-1-git-send-email-quic_deesin@quicinc.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | [V2,1/2] rpmsg: glink: Add lock to avoid race when rpmsg device is released | expand |
Quoting Deepak Kumar Singh (2022-09-05 11:55:19) > When remote host goes down glink char device channel is freed, > At the same time user space apps can still try to open/poll rpmsg > char device which will result in calling rpmsg_create_ept. This may > cause reference to already freed context of glink chardev channel and > result in below crash signatures - > > 1) > rpmsg_create_ept+0x40/0xa0 > rpmsg_eptdev_open+0x88/0x138 > chrdev_open+0xc4/0x1c8 > do_dentry_open+0x230/0x378 > > 2) > rpmsg_poll+0x5c/0x80 > rpmsg_eptdev_poll+0x84/0xa4 > do_sys_poll+0x22c/0x5c8 > > This patch adds proper lock and check condition to avoid such crash. Usually it's nice to have a two CPU diagram that shows the problem scenario with time flowing down and interleaving the calls with indentation, instead of some callstacks. Can you add that detail here? I guess it would look like this: CPU0 CPU1 ---- ---- do_sys_poll() chrdev_open() rpmsg_eptdev_poll() rpmsg_eptdev_open() rpmsg_create_ept() rpmsg_poll() <access old rpdev pointer?> but I don't immediately understand the problem. Probably showing the free path and wherever eptdev is set to NULL will help inform better than a parallel poll and chrdev open. > > Signed-off-by: Deepak Kumar Singh <quic_deesin@quicinc.com> > --- Any Fixes tag?
diff --git a/drivers/rpmsg/rpmsg_char.c b/drivers/rpmsg/rpmsg_char.c index 4f21891..5500dc0 100644 --- a/drivers/rpmsg/rpmsg_char.c +++ b/drivers/rpmsg/rpmsg_char.c @@ -75,6 +75,7 @@ int rpmsg_chrdev_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; @@ -126,6 +127,11 @@ static int rpmsg_eptdev_open(struct inode *inode, struct file *filp) return -EBUSY; } + if (!eptdev->rpdev) { + mutex_unlock(&eptdev->ept_lock); + return -ENETRESET; + } + get_device(dev); /* @@ -277,7 +283,9 @@ static __poll_t rpmsg_eptdev_poll(struct file *filp, poll_table *wait) if (!skb_queue_empty(&eptdev->queue)) mask |= EPOLLIN | EPOLLRDNORM; + mutex_lock(&eptdev->ept_lock); mask |= rpmsg_poll(eptdev->ept, filp, wait); + mutex_unlock(&eptdev->ept_lock); return mask; }
When remote host goes down glink char device channel is freed, At the same time user space apps can still try to open/poll rpmsg char device which will result in calling rpmsg_create_ept. This may cause reference to already freed context of glink chardev channel and result in below crash signatures - 1) rpmsg_create_ept+0x40/0xa0 rpmsg_eptdev_open+0x88/0x138 chrdev_open+0xc4/0x1c8 do_dentry_open+0x230/0x378 2) rpmsg_poll+0x5c/0x80 rpmsg_eptdev_poll+0x84/0xa4 do_sys_poll+0x22c/0x5c8 This patch adds proper lock and check condition to avoid such crash. Signed-off-by: Deepak Kumar Singh <quic_deesin@quicinc.com> --- drivers/rpmsg/rpmsg_char.c | 8 ++++++++ 1 file changed, 8 insertions(+)