From 1188bdd70dd2fc2e426b36858360204cfa8433a3 Mon Sep 17 00:00:00 2001 From: Darek Stojaczyk Date: Wed, 27 Mar 2019 16:26:22 +0100 Subject: [PATCH] vhost/scsi: forbid removing targets that are still being added It is theoretically possible for an asynchronous hotremove request to be finished before the hotplug request that was started first. This is obviously not expected and will most likely result in a resource leak. For SCSI target hotplug, we immediately update the whole vhost device object and then asynchronously ask each vhost session to poll the changes. For hotremove, we see the device attached in the whole vhost device object, so we immediately mark it as "still being removed" and proceed aynchronously asking the sessions to hotremove. When session receives the hotremove event first, it will either fail an assertion (when debug is on), or do nothing. The subsequent hotplug event will attach the target again - and that target won't be ever freed. Change-Id: I784c979fb47127a4238038ad9fb5ed1cac3ced04 Signed-off-by: Darek Stojaczyk Reviewed-on: https://review.gerrithub.io/c/spdk/spdk/+/449391 Tested-by: SPDK CI Jenkins Reviewed-by: Pawel Wodkowski Reviewed-by: Changpeng Liu Reviewed-by: Jim Harris --- lib/vhost/vhost_scsi.c | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-) diff --git a/lib/vhost/vhost_scsi.c b/lib/vhost/vhost_scsi.c index bd5a08e23..235790b0d 100644 --- a/lib/vhost/vhost_scsi.c +++ b/lib/vhost/vhost_scsi.c @@ -71,6 +71,9 @@ enum spdk_scsi_dev_vhost_status { /* Target ID is empty. */ VHOST_SCSI_DEV_EMPTY, + /* Target is still being added. */ + VHOST_SCSI_DEV_ADDING, + /* Target ID occupied. */ VHOST_SCSI_DEV_PRESENT, @@ -933,7 +936,13 @@ spdk_vhost_scsi_session_add_tgt(struct spdk_vhost_dev *vdev, int rc; if (vsession == NULL) { - /* Nothing more to do */ + struct spdk_vhost_scsi_dev *svdev = SPDK_CONTAINEROF(vdev, + struct spdk_vhost_scsi_dev, vdev); + vhost_sdev = &svdev->scsi_dev_state[scsi_tgt_num]; + + /* All sessions have added the target */ + assert(vhost_sdev->status == VHOST_SCSI_DEV_ADDING); + vhost_sdev->status = VHOST_SCSI_DEV_PRESENT; return 0; } @@ -942,7 +951,7 @@ spdk_vhost_scsi_session_add_tgt(struct spdk_vhost_dev *vdev, session_sdev = &svsession->scsi_dev_state[scsi_tgt_num]; session_sdev->dev = vhost_sdev->dev; - session_sdev->status = vhost_sdev->status; + session_sdev->status = VHOST_SCSI_DEV_PRESENT; if (vsession->lcore == -1) { /* All done. */ @@ -1033,7 +1042,7 @@ spdk_vhost_scsi_dev_add_tgt(struct spdk_vhost_dev *vdev, int scsi_tgt_num, lun_id_list[0] = 0; bdev_names_list[0] = (char *)bdev_name; - state->status = VHOST_SCSI_DEV_PRESENT; + state->status = VHOST_SCSI_DEV_ADDING; state->dev = spdk_scsi_dev_construct(target_name, bdev_names_list, lun_id_list, 1, SPDK_SPC_PROTOCOL_IDENTIFIER_SAS, spdk_vhost_scsi_lun_hotremove, svdev); @@ -1116,7 +1125,7 @@ spdk_vhost_scsi_dev_remove_tgt(struct spdk_vhost_dev *vdev, unsigned scsi_tgt_nu } scsi_dev_state = &svdev->scsi_dev_state[scsi_tgt_num]; - if (scsi_dev_state->dev == NULL) { + if (scsi_dev_state->dev == NULL || scsi_dev_state->status == VHOST_SCSI_DEV_ADDING) { SPDK_ERRLOG("Controller %s target %u is not occupied\n", vdev->name, scsi_tgt_num); return -ENODEV; }