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

[Deepak Kumar Singh <quic_deesin@xxxxxxxxxxx>]

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