search for: err_vqs

We flush under vq mutex when changing backends. This creates a deadlock as workqueue being flushed needs this lock as well. https://bugzilla.redhat.com/show_bug.cgi?id=612421 Drop the vq mutex before flush: we have the device mutex which is sufficient to prevent another ioctl from touching the vq. Signed-off-by: Michael S. Tsirkin <mst at redhat.com> --- drivers/vhost/net.c | 5 +++++

We flush under vq mutex when changing backends. This creates a deadlock as workqueue being flushed needs this lock as well. https://bugzilla.redhat.com/show_bug.cgi?id=612421 Drop the vq mutex before flush: we have the device mutex which is sufficient to prevent another ioctl from touching the vq. Signed-off-by: Michael S. Tsirkin <mst at redhat.com> --- drivers/vhost/net.c | 5 +++++

...;vqs) + return -ENOMEM; names = kmalloc(sizeof(*names) * num_vqs, GFP_KERNEL); - if (!names) - goto err_names; - callbacks = kmalloc(sizeof(*callbacks) * num_vqs, GFP_KERNEL); - if (!callbacks) - goto err_callbacks; - vqs = kmalloc(sizeof(*vqs) * num_vqs, GFP_KERNEL); - if (!vqs) - goto err_vqs; + if (!names || !callbacks || !vqs) { + err = -ENOMEM; + goto out; + } for (i = 0; i < num_vqs; i++) { callbacks[i] = virtblk_done; @@ -420,7 +414,7 @@ static int init_vq(struct virtio_blk *vblk) /* Discover virtqueues and write information to configuration. */ err = vdev->conf...

...;vqs) + return -ENOMEM; names = kmalloc(sizeof(*names) * num_vqs, GFP_KERNEL); - if (!names) - goto err_names; - callbacks = kmalloc(sizeof(*callbacks) * num_vqs, GFP_KERNEL); - if (!callbacks) - goto err_callbacks; - vqs = kmalloc(sizeof(*vqs) * num_vqs, GFP_KERNEL); - if (!vqs) - goto err_vqs; + if (!names || !callbacks || !vqs) { + err = -ENOMEM; + goto out; + } for (i = 0; i < num_vqs; i++) { callbacks[i] = virtblk_done; @@ -420,7 +414,7 @@ static int init_vq(struct virtio_blk *vblk) /* Discover virtqueues and write information to configuration. */ err = vdev->conf...

...-ENOMEM; + vs = kzalloc(sizeof(*vs), GFP_KERNEL | __GFP_NOWARN | __GFP_REPEAT); + if (!vs) { + vs = vzalloc(sizeof(*vs)); + if (!vs) + goto err_vs; + } vqs = kmalloc(VHOST_SCSI_MAX_VQ * sizeof(*vqs), GFP_KERNEL); - if (!vqs) { - kfree(vs); - return -ENOMEM; - } + if (!vqs) + goto err_vqs; vhost_work_init(&vs->vs_completion_work, vhost_scsi_complete_cmd_work); vhost_work_init(&vs->vs_event_work, tcm_vhost_evt_work); @@ -1407,14 +1416,18 @@ static int vhost_scsi_open(struct inode *inode, struct file *f) tcm_vhost_init_inflight(vs, NULL); - if (r < 0) { -...

...-ENOMEM; + vs = kzalloc(sizeof(*vs), GFP_KERNEL | __GFP_NOWARN | __GFP_REPEAT); + if (!vs) { + vs = vzalloc(sizeof(*vs)); + if (!vs) + goto err_vs; + } vqs = kmalloc(VHOST_SCSI_MAX_VQ * sizeof(*vqs), GFP_KERNEL); - if (!vqs) { - kfree(vs); - return -ENOMEM; - } + if (!vqs) + goto err_vqs; vhost_work_init(&vs->vs_completion_work, vhost_scsi_complete_cmd_work); vhost_work_init(&vs->vs_event_work, tcm_vhost_evt_work); @@ -1407,14 +1416,18 @@ static int vhost_scsi_open(struct inode *inode, struct file *f) tcm_vhost_init_inflight(vs, NULL); - if (r < 0) { -...

> > Add a new vq to report hints of guest free pages to the host. > > Signed-off-by: Wei Wang <wei.w.wang at intel.com> > Signed-off-by: Liang Li <liang.z.li at intel.com> > --- > drivers/virtio/virtio_balloon.c | 164 > ++++++++++++++++++++++++++++++------ > include/uapi/linux/virtio_balloon.h | 1 + > 2 files changed, 140 insertions(+), 25

> > Add a new vq to report hints of guest free pages to the host. > > Signed-off-by: Wei Wang <wei.w.wang at intel.com> > Signed-off-by: Liang Li <liang.z.li at intel.com> > --- > drivers/virtio/virtio_balloon.c | 164 > ++++++++++++++++++++++++++++++------ > include/uapi/linux/virtio_balloon.h | 1 + > 2 files changed, 140 insertions(+), 25

vhost_net_ubuf_put_and_wait has a confusing name: it will actually also free it's argument. Thus since commit 1280c27f8e29acf4af2da914e80ec27c3dbd5c01 vhost_net_flush tries to use the argument after passing it to vhost_net_ubuf_put_and_wait, this results in use after free. To fix, don't free the argument in vhost_net_ubuf_put_and_wait, add an new API for callers that want to free ubufs.

vhost_net_ubuf_put_and_wait has a confusing name: it will actually also free it's argument. Thus since commit 1280c27f8e29acf4af2da914e80ec27c3dbd5c01 vhost_net_flush tries to use the argument after passing it to vhost_net_ubuf_put_and_wait, this results in use after free. To fix, don't free the argument in vhost_net_ubuf_put_and_wait, add an new API for callers that want to free ubufs.

...;vqs) + return -ENOMEM; names = kmalloc(sizeof(*names) * num_vqs, GFP_KERNEL); - if (!names) - goto err_names; - callbacks = kmalloc(sizeof(*callbacks) * num_vqs, GFP_KERNEL); - if (!callbacks) - goto err_callbacks; - vqs = kmalloc(sizeof(*vqs) * num_vqs, GFP_KERNEL); - if (!vqs) - goto err_vqs; + if (!names || !callbacks || !vqs) { + err = -ENOMEM; + goto out; + } for (i = 0; i < num_vqs; i++) { callbacks[i] = virtblk_done; @@ -417,7 +411,7 @@ static int init_vq(struct virtio_blk *vblk) /* Discover virtqueues and write information to configuration. */ err = vdev->conf...

> > On 08/03/2017 04:13 PM, Pankaj Gupta wrote: > >> > >> + /* Allocate space for find_vqs parameters */ > >> + vqs = kcalloc(nvqs, sizeof(*vqs), GFP_KERNEL); > >> + if (!vqs) > >> + goto err_vq; > >> + callbacks = kmalloc_array(nvqs, sizeof(*callbacks), GFP_KERNEL); > >> + if

...;vqs) + return -ENOMEM; names = kmalloc(sizeof(*names) * num_vqs, GFP_KERNEL); - if (!names) - goto err_names; - callbacks = kmalloc(sizeof(*callbacks) * num_vqs, GFP_KERNEL); - if (!callbacks) - goto err_callbacks; - vqs = kmalloc(sizeof(*vqs) * num_vqs, GFP_KERNEL); - if (!vqs) - goto err_vqs; + if (!names || !callbacks || !vqs) { + err = -ENOMEM; + goto out; + } for (i = 0; i < num_vqs; i++) { callbacks[i] = virtblk_done; @@ -417,7 +411,7 @@ static int init_vq(struct virtio_blk *vblk) /* Discover virtqueues and write information to configuration. */ err = vdev->conf...

> > On 08/03/2017 04:13 PM, Pankaj Gupta wrote: > >> > >> + /* Allocate space for find_vqs parameters */ > >> + vqs = kcalloc(nvqs, sizeof(*vqs), GFP_KERNEL); > >> + if (!vqs) > >> + goto err_vq; > >> + callbacks = kmalloc_array(nvqs, sizeof(*callbacks), GFP_KERNEL); > >> + if

...!names) > > - goto err_names; > > - > > callbacks = kmalloc(sizeof(*callbacks) * num_vqs, GFP_KERNEL); > > - if (!callbacks) > > - goto err_callbacks; > > - > > vqs = kmalloc(sizeof(*vqs) * num_vqs, GFP_KERNEL); > > - if (!vqs) > > - goto err_vqs; > > + if (!names || !callbacks || !vqs) { > > + err = -ENOMEM; > > + goto out; > > + } > > ...you could use the > > foo = kmalloc(...); > if (!foo) > goto out; > > sequence in any case. This avoids trying again and again if e.g. the > names...

...!names) > > - goto err_names; > > - > > callbacks = kmalloc(sizeof(*callbacks) * num_vqs, GFP_KERNEL); > > - if (!callbacks) > > - goto err_callbacks; > > - > > vqs = kmalloc(sizeof(*vqs) * num_vqs, GFP_KERNEL); > > - if (!vqs) > > - goto err_vqs; > > + if (!names || !callbacks || !vqs) { > > + err = -ENOMEM; > > + goto out; > > + } > > ...you could use the > > foo = kmalloc(...); > if (!foo) > goto out; > > sequence in any case. This avoids trying again and again if e.g. the > names...

...st_scsi_free(struct vhost_scsi *vs) -{ - if (is_vmalloc_addr(vs)) - vfree(vs); - else - kfree(vs); -} - static int vhost_scsi_open(struct inode *inode, struct file *f) { struct vhost_scsi *vs; @@ -1443,7 +1435,7 @@ static int vhost_scsi_open(struct inode *inode, struct file *f) return 0; err_vqs: - vhost_scsi_free(vs); + kvfree(vs); err_vs: return r; } @@ -1462,7 +1454,7 @@ static int vhost_scsi_release(struct inode *inode, struct file *f) /* Jobs can re-queue themselves in evt kick handler. Do extra flush. */ vhost_scsi_flush(vs); kfree(vs->dev.vqs); - vhost_scsi_free(vs); +...

...st_scsi_free(struct vhost_scsi *vs) -{ - if (is_vmalloc_addr(vs)) - vfree(vs); - else - kfree(vs); -} - static int vhost_scsi_open(struct inode *inode, struct file *f) { struct vhost_scsi *vs; @@ -1443,7 +1435,7 @@ static int vhost_scsi_open(struct inode *inode, struct file *f) return 0; err_vqs: - vhost_scsi_free(vs); + kvfree(vs); err_vs: return r; } @@ -1462,7 +1454,7 @@ static int vhost_scsi_release(struct inode *inode, struct file *f) /* Jobs can re-queue themselves in evt kick handler. Do extra flush. */ vhost_scsi_flush(vs); kfree(vs->dev.vqs); - vhost_scsi_free(vs); +...

On 13.02.20 17:29, Eugenio P?rez wrote: > Can we try with this traces? Does not apply on eccb852f1fe6bede630e2e4f1a121a81e34354ab, can you double check? > > From b793b4106085ab1970bdedb340e49f37843ed585 Mon Sep 17 00:00:00 2001 > From: =?UTF-8?q?Eugenio=20P=C3=A9rez?= <eperezma at redhat.com> > Date: Thu, 13 Feb 2020 17:27:05 +0100 > Subject: [PATCH] vhost: Add debug in

...f(*names) * num_vqs, GFP_KERNEL); > + if (!names) > + goto err_names; > + > + callbacks = kmalloc(sizeof(*callbacks) * num_vqs, GFP_KERNEL); > + if (!callbacks) > + goto err_callbacks; > + > + vqs = kmalloc(sizeof(*vqs) * num_vqs, GFP_KERNEL); > + if (!vqs) > + goto err_vqs; > + > + for (i = 0; i < num_vqs; i++) { > + callbacks[i] = virtblk_done; > + snprintf(&name_array[i * 32], 32, "req.%d", i); Eschew abbreviation. Call it requests.%d. > + names[i] = &name_array[i * 32]; > + } That 32 and pointer math hurts. Please creat...