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
>
