Re: [PATCH] vhost/vsock: don't check owner in vhost_vsock_stop() while releasing

[Stefano Garzarella]

On Mon, Feb 21, 2022 at 10:03:39AM -0500, Michael S. Tsirkin wrote:
> On Mon, Feb 21, 2022 at 12:49:16PM +0100, Stefano Garzarella wrote:
> > vhost_vsock_stop() calls vhost_dev_check_owner() to check the device
> > ownership. It expects current->mm to be valid.
> >
> > vhost_vsock_stop() is also called by vhost_vsock_dev_release() when
> > the user has not done close(), so when we are in do_exit(). In this
> > case current->mm is invalid and we're releasing the device, so we
> > should clean it anyway.
> >
> > Let's check the owner only when vhost_vsock_stop() is called
> > by an ioctl.
>
>
>
>
> > Fixes: 433fc58e6bf2 ("VSOCK: Introduce vhost_vsock.ko")
> > Cc: stable@xxxxxxxxxxxxxxx
> > Reported-by: syzbot+1e3ea63db39f2b4440e0@xxxxxxxxxxxxxxxxxxxxxxxxx
> > Signed-off-by: Stefano Garzarella <sgarzare@xxxxxxxxxx>
> > ---
> >  drivers/vhost/vsock.c | 14 ++++++++------
> >  1 file changed, 8 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/vhost/vsock.c b/drivers/vhost/vsock.c
> > index d6ca1c7ad513..f00d2dfd72b7 100644
> > --- a/drivers/vhost/vsock.c
> > +++ b/drivers/vhost/vsock.c
> > @@ -629,16 +629,18 @@ static int vhost_vsock_start(struct vhost_vsock *vsock)
> >  	return ret;
> >  }
> >
> > -static int vhost_vsock_stop(struct vhost_vsock *vsock)
> > +static int vhost_vsock_stop(struct vhost_vsock *vsock, bool check_owner)
>
> >  {
> >  	size_t i;
> >  	int ret;
> >
> >  	mutex_lock(&vsock->dev.mutex);
> >
> > -	ret = vhost_dev_check_owner(&vsock->dev);
> > -	if (ret)
> > -		goto err;
> > +	if (check_owner) {
> > +		ret = vhost_dev_check_owner(&vsock->dev);
> > +		if (ret)
> > +			goto err;
> > +	}
> >
> >  	for (i = 0; i < ARRAY_SIZE(vsock->vqs); i++) {
> >  		struct vhost_virtqueue *vq = &vsock->vqs[i];
> > @@ -753,7 +755,7 @@ static int vhost_vsock_dev_release(struct inode *inode, struct file *file)
> >  	 * inefficient.  Room for improvement here. */
> >  	vsock_for_each_connected_socket(vhost_vsock_reset_orphans);
> >
> > -	vhost_vsock_stop(vsock);
>
> Let's add an explanation:
>
> When invoked from release we can not fail so we don't
> check return code of vhost_vsock_stop.
> We need to stop vsock even if it's not the owner.

Do you want me to send a v2 by adding this as a comment in the code?

Thanks,
Stefano