On Thu, Oct 08, 2020 at 10:24:36PM +0200, Stefano Garzarella wrote:
> 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: sta...@vger.kernel.org
> > > Signed-off-by: Stefano Garzarella <sgarz...@redhat.com>
> >
> > 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
>
I think it's ok, this info just needs to be in the commit log ...
> > > ---
> > > 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
> >
