Re: [PATCH v7 3/3] vhost: add an RPMsg API

[Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>]

Hi Vincent,

On Thu, Sep 17, 2020 at 10:55:59AM +0200, Vincent Whitchurch wrote:
> On Thu, Sep 10, 2020 at 01:13:51PM +0200, Guennadi Liakhovetski wrote:
> > +int vhost_rpmsg_start_lock(struct vhost_rpmsg *vr, struct vhost_rpmsg_iter *iter,
> > +			   unsigned int qid, ssize_t len)
> > +	__acquires(vq->mutex)
> > +{
> > +	struct vhost_virtqueue *vq = vr->vq + qid;
> > +	unsigned int cnt;
> > +	ssize_t ret;
> > +	size_t tmp;
> > +
> > +	if (qid >= VIRTIO_RPMSG_NUM_OF_VQS)
> > +		return -EINVAL;
> > +
> > +	iter->vq = vq;
> > +
> > +	mutex_lock(&vq->mutex);
> > +	vhost_disable_notify(&vr->dev, vq);
> > +
> > +	iter->head = vhost_rpmsg_get_msg(vq, &cnt);
> > +	if (iter->head == vq->num)
> > +		iter->head = -EAGAIN;
> > +
> > +	if (iter->head < 0) {
> > +		ret = iter->head;
> > +		goto unlock;
> > +	}
> > +
> [...]
> > +
> > +return_buf:
> > +	vhost_add_used(vq, iter->head, 0);
> > +unlock:
> > +	vhost_enable_notify(&vr->dev, vq);
> > +	mutex_unlock(&vq->mutex);
> > +
> > +	return ret;
> > +}
> 
> There is a race condition here.  New buffers could have been added while
> notifications were disabled (between vhost_disable_notify() and
> vhost_enable_notify()), so the other vhost drivers check the return
> value of vhost_enable_notify() and rerun their work loops if it returns
> true.  This driver doesn't do that so it stops processing requests if
> that condition hits.

You're right, thanks for spotting this!

> Something like the below seems to fix it but the correct fix could maybe
> involve changing this API to account for this case so that it looks more
> like the code in other vhost drivers.

I'll try to use your proposed code, we'll see if it turns out incorrect.

> diff --git a/drivers/vhost/rpmsg.c b/drivers/vhost/rpmsg.c
> index 7c753258d42..673dd4ec865 100644
> --- a/drivers/vhost/rpmsg.c
> +++ b/drivers/vhost/rpmsg.c
> @@ -302,8 +302,14 @@ static void handle_rpmsg_req_kick(struct vhost_work *work)
>  	struct vhost_virtqueue *vq = container_of(work, struct vhost_virtqueue,
>  						  poll.work);
>  	struct vhost_rpmsg *vr = container_of(vq->dev, struct vhost_rpmsg, dev);
> +	struct vhost_virtqueue *reqvq = vr->vq + VIRTIO_RPMSG_REQUEST;

This is only called on the request queue, so, we can just use vq here.

>  
> -	while (handle_rpmsg_req_single(vr, vq))
> +	/*
> +	 * The !vhost_vq_avail_empty() check is needed since the vhost_rpmsg*
> +	 * APIs don't check the return value of vhost_enable_notify() and retry
> +	 * if there were buffers added while notifications were disabled.
> +	 */
> +	while (handle_rpmsg_req_single(vr, vq) || !vhost_vq_avail_empty(reqvq->dev, reqvq))
>  		;
>  }

Thanks
Guennadi
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization