Re: [PATCH] vringh: fix __vringh_iov() when riov and wiov are different

[Stefano Garzarella]

On Thu, Oct 08, 2020 at 04:00:51PM -0400, Michael S. Tsirkin wrote:
> On Thu, Oct 08, 2020 at 06:13:11PM +0200, Stefano Garzarella wrote:
> > If riov and wiov are both defined and they point to different
> > objects, only riov is initialized. If the wiov is not initialized
> > by the caller, the function fails returning -EINVAL and printing
> > "Readable desc 0x... after writable" error message.
> > 
> > Let's replace the 'else if' clause with 'if' to initialize both
> > riov and wiov if they are not NULL.
> > 
> > As checkpatch pointed out, we also avoid crashing the kernel
> > when riov and wiov are both NULL, replacing BUG() with WARN_ON()
> > and returning -EINVAL.
> > 
> > Fixes: f87d0fbb5798 ("vringh: host-side implementation of virtio rings.")
> > Cc: stable@xxxxxxxxxxxxxxx
> > Signed-off-by: Stefano Garzarella <sgarzare@xxxxxxxxxx>
> 
> Can you add more detail please? when does this trigger?

I'm developing vdpa_sim_blk and I'm using vringh_getdesc_iotlb()
to get readable and writable buffers.

With virtio-blk devices a descriptors has both readable and writable
buffers (eg. virtio_blk_outhdr in the readable buffer and status as last byte
of writable buffer).
So, I'm calling vringh_getdesc_iotlb() one time to get both type of buffer
and put them in 2 iovecs:

	ret = vringh_getdesc_iotlb(&vq->vring, &vq->riov, &vq->wiov,
				   &vq->head, GFP_ATOMIC);

With this patch applied it works well, without the function fails
returning -EINVAL and printing "Readable desc 0x... after writable".

Am I using vringh_getdesc_iotlb() in the wrong way?

Thanks,
Stefano

> 
> > ---
> >  drivers/vhost/vringh.c | 9 +++++----
> >  1 file changed, 5 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/vhost/vringh.c b/drivers/vhost/vringh.c
> > index e059a9a47cdf..8bd8b403f087 100644
> > --- a/drivers/vhost/vringh.c
> > +++ b/drivers/vhost/vringh.c
> > @@ -284,13 +284,14 @@ __vringh_iov(struct vringh *vrh, u16 i,
> >  	desc_max = vrh->vring.num;
> >  	up_next = -1;
> >  
> > +	/* You must want something! */
> > +	if (WARN_ON(!riov && !wiov))
> > +		return -EINVAL;
> > +
> >  	if (riov)
> >  		riov->i = riov->used = 0;
> > -	else if (wiov)
> > +	if (wiov)
> >  		wiov->i = wiov->used = 0;
> > -	else
> > -		/* You must want something! */
> > -		BUG();
> >  
> >  	for (;;) {
> >  		void *addr;
> > -- 
> > 2.26.2
>