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

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

[Stefano Garzarella]

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@vger.kernel.org
Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
---
 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

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

[Michael S. Tsirkin]

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@vger.kernel.org
> Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>

Can you add more detail please? when does this trigger?

> ---
>  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

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@vger.kernel.org
> > Signed-off-by: Stefano Garzarella <sgarzare@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

> 
> > ---
> >  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
> 

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

[Michael S. Tsirkin]

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: stable@vger.kernel.org
> > > Signed-off-by: Stefano Garzarella <sgarzare@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
> > 

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

[Stefano Garzarella]

On Thu, Oct 08, 2020 at 04:28:40PM -0400, Michael S. Tsirkin wrote:
> 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: stable@vger.kernel.org
> > > > Signed-off-by: Stefano Garzarella <sgarzare@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 ...

Sure, I'll send a v2 adding this info.
Sorry for not adding it earlier!

Thanks,
Stefano