| Message ID | 20200415032826.16701-5-fengli@smartx.com |
|---|---|
| State | New |
| Headers | show |
| Series | fix crashes when inject errors to vhost-user-blk chardev | expand |
Mostly looks good - just a few superficial notes. On Wed, Apr 15, 2020 at 11:28:26AM +0800, Li Feng wrote: > 1. set s->connected to true after vhost_dev_init; > 2. call vhost_dev_get_config when s->connected is true, otherwise the > hdev->host_ops will be nullptr. You mean hdev->vhost_ops, right? > > Signed-off-by: Li Feng <fengli@smartx.com> > --- > hw/block/vhost-user-blk.c | 47 +++++++++++++++++++++++++---------------------- > 1 file changed, 25 insertions(+), 22 deletions(-) > + /* > + * set true util vhost_dev_init return ok, because CLOSE event may happen > + * in vhost_dev_init routine. > + */ I'm a little confused by this comment. Do you mean to say “wait until vhost_dev_init succeeds to set connected to true, because a close event may happen while vhost_dev_init is executing”?
Add mail group list. Thank you, Raphael . Raphael Norwitz <raphael.norwitz@nutanix.com> 于2020年4月17日周五 下午12:10写道： > > Mostly looks good - just a few superficial notes. > > On Wed, Apr 15, 2020 at 11:28:26AM +0800, Li Feng wrote: > > 1. set s->connected to true after vhost_dev_init; > > 2. call vhost_dev_get_config when s->connected is true, otherwise the > > hdev->host_ops will be nullptr. > > You mean hdev->vhost_ops, right? > Yes. > > > > Signed-off-by: Li Feng <fengli@smartx.com> > > --- > > hw/block/vhost-user-blk.c | 47 +++++++++++++++++++++++++---------------------- > > 1 file changed, 25 insertions(+), 22 deletions(-) > > + /* > > + * set true util vhost_dev_init return ok, because CLOSE event may happen > > + * in vhost_dev_init routine. > > + */ > > I'm a little confused by this comment. Do you mean to say “wait until vhost_dev_init > succeeds to set connected to true, because a close event may happen while > vhost_dev_init is executing”? Yes. This is the exception path: qemu_chr_fe_set_handlers -> vhost_user_blk_event(OPEN) -> vhost_user_blk_connect -> vhost_dev_init -> vhost_user_blk_event(CLOSE) -> vhost_dev_cleanup We should set connected to true only when vhost_dev_init returns OK. If a close event is triggered, we shouldn't set connected to true.
diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c index 19e79b96e4..35386b7cb7 100644 --- a/hw/block/vhost-user-blk.c +++ b/hw/block/vhost-user-blk.c @@ -303,8 +303,6 @@ static int vhost_user_blk_connect(DeviceState *dev) if (s->connected) { return 0; } - s->connected = true; - s->dev.nvqs = s->num_queues; s->dev.vqs = s->vhost_vqs; s->dev.vq_index = 0; @@ -318,6 +316,11 @@ static int vhost_user_blk_connect(DeviceState *dev) strerror(-ret)); return ret; } + /* + * set true util vhost_dev_init return ok, because CLOSE event may happen + * in vhost_dev_init routine. + */ + s->connected = true; /* restore vhost state */ if (virtio_device_started(vdev, vdev->status)) { @@ -401,6 +404,7 @@ static void vhost_user_blk_device_realize(DeviceState *dev, Error **errp) VHostUserBlk *s = VHOST_USER_BLK(vdev); Error *err = NULL; int i, ret; + bool reconnect; if (!s->chardev.chr) { error_setg(errp, "vhost-user-blk: chardev is mandatory"); @@ -433,27 +437,26 @@ static void vhost_user_blk_device_realize(DeviceState *dev, Error **errp) s->inflight = g_new0(struct vhost_inflight, 1); s->vhost_vqs = g_new0(struct vhost_virtqueue, s->num_queues); s->connected = false; + reconnect = false; - qemu_chr_fe_set_handlers(&s->chardev, NULL, NULL, vhost_user_blk_event, - NULL, (void *)dev, NULL, true); - -reconnect: - if (qemu_chr_fe_wait_connected(&s->chardev, &err) < 0) { - error_report_err(err); - goto virtio_err; - } - - /* check whether vhost_user_blk_connect() failed or not */ - if (!s->connected) { - goto reconnect; - } - - ret = vhost_dev_get_config(&s->dev, (uint8_t *)&s->blkcfg, - sizeof(struct virtio_blk_config)); - if (ret < 0) { - error_report("vhost-user-blk: get block config failed"); - goto reconnect; - } + do { + if (qemu_chr_fe_wait_connected(&s->chardev, &err) < 0) { + error_report_err(err); + goto virtio_err; + } + qemu_chr_fe_set_handlers(&s->chardev, NULL, NULL, vhost_user_blk_event, + NULL, (void *)dev, NULL, true); + if (s->connected) { + ret = vhost_dev_get_config(&s->dev, (uint8_t *)&s->blkcfg, + sizeof(struct virtio_blk_config)); + if (ret < 0) { + error_report("vhost-user-blk: get block config failed"); + reconnect = true; + } else { + reconnect = false; + } + } + } while (!s->connected || reconnect); if (s->blkcfg.num_queues != s->num_queues) { s->blkcfg.num_queues = s->num_queues;
The crash could be reproduced like this: 1. break vhost_user_write; 2. kill the vhost-user-blk target; 3. let qemu continue running; 4. start vhost-user-blk; 5. see crash! This fix makes changes: 1. set s->connected to true after vhost_dev_init; 2. call vhost_dev_get_config when s->connected is true, otherwise the hdev->host_ops will be nullptr. Signed-off-by: Li Feng <fengli@smartx.com> --- hw/block/vhost-user-blk.c | 47 +++++++++++++++++++++++++---------------------- 1 file changed, 25 insertions(+), 22 deletions(-)