* [PATCH] vringh: fix __vringh_iov() when riov and wiov are different @ 2020-10-08 16:13 Stefano Garzarella 2020-10-08 20:00 ` Michael S. Tsirkin 0 siblings, 1 reply; 5+ messages in thread From: Stefano Garzarella @ 2020-10-08 16:13 UTC (permalink / raw) To: mst Cc: netdev, virtualization, linux-kernel, kvm, Rusty Russell, stable, Jason Wang 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 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] vringh: fix __vringh_iov() when riov and wiov are different 2020-10-08 16:13 [PATCH] vringh: fix __vringh_iov() when riov and wiov are different Stefano Garzarella @ 2020-10-08 20:00 ` Michael S. Tsirkin 2020-10-08 20:24 ` Stefano Garzarella 0 siblings, 1 reply; 5+ messages in thread From: Michael S. Tsirkin @ 2020-10-08 20:00 UTC (permalink / raw) To: Stefano Garzarella Cc: netdev, virtualization, linux-kernel, kvm, Rusty Russell, stable, Jason Wang 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] vringh: fix __vringh_iov() when riov and wiov are different 2020-10-08 20:00 ` Michael S. Tsirkin @ 2020-10-08 20:24 ` Stefano Garzarella 2020-10-08 20:28 ` Michael S. Tsirkin 0 siblings, 1 reply; 5+ messages in thread From: Stefano Garzarella @ 2020-10-08 20:24 UTC (permalink / raw) To: Michael S. Tsirkin Cc: netdev, virtualization, linux-kernel, kvm, Rusty Russell, stable, Jason Wang 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 > ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] vringh: fix __vringh_iov() when riov and wiov are different 2020-10-08 20:24 ` Stefano Garzarella @ 2020-10-08 20:28 ` Michael S. Tsirkin 2020-10-08 20:32 ` Stefano Garzarella 0 siblings, 1 reply; 5+ messages in thread From: Michael S. Tsirkin @ 2020-10-08 20:28 UTC (permalink / raw) To: Stefano Garzarella Cc: netdev, virtualization, linux-kernel, kvm, Rusty Russell, stable, Jason Wang 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 > > ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] vringh: fix __vringh_iov() when riov and wiov are different 2020-10-08 20:28 ` Michael S. Tsirkin @ 2020-10-08 20:32 ` Stefano Garzarella 0 siblings, 0 replies; 5+ messages in thread From: Stefano Garzarella @ 2020-10-08 20:32 UTC (permalink / raw) To: Michael S. Tsirkin Cc: netdev, virtualization, linux-kernel, kvm, Rusty Russell, stable, Jason Wang 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2020-10-08 20:32 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-10-08 16:13 [PATCH] vringh: fix __vringh_iov() when riov and wiov are different Stefano Garzarella 2020-10-08 20:00 ` Michael S. Tsirkin 2020-10-08 20:24 ` Stefano Garzarella 2020-10-08 20:28 ` Michael S. Tsirkin 2020-10-08 20:32 ` Stefano Garzarella
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).