| Message ID | 20201008161311.114398-1-sgarzare@redhat.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | vringh: fix __vringh_iov() when riov and wiov are different | expand |
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
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 >
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 > >
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
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;
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(-)