From 7a9ecf8284accbb14e99cf8533e561d5f9820f76 Mon Sep 17 00:00:00 2001 From: Shuhei Matsumoto Date: Fri, 18 Oct 2019 08:17:03 +0900 Subject: [PATCH] lib/scsi: Free bdev_io just after getting completion from bdev for non-read I/O Previously, for iSCSI target, freeing bdev_io of SCSI task was deferred until the reference count of the SCSI task becomes zero. But this will cause the use-after-free issue when doing LUN hotplug during large write I/O workload. The scenario is the following: - Large iSCSI write I/O is split into multiple I/Os, the first I/O is from immediate, and subsetquent I/Os are from R2T. 1. The first I/O allocates iSCSI task as primary, and is submitted to the bdev layer. The first I/O is pending in the bdev layer. 2. The second I/O allocates iSCSI task as secondary (secondary is associated with primary by incrementing reference count). 3. Before submitting the second I/O to the bdev layer, LUN hotplug is started. LUN hotplug waits for getting completion of the first write I/O from the bdev layer. 4. The bdev layer completes the first I/O. The primary iSCSI task is tried to free, but reference count is still one, and is not done yet. 5. LUN hotplug detects completion of the first write I/O, and returns LUN I/O channel to the bdev layer. 6. The second I/O is tried to submit to the bdev layer, but LUN is already removed, and so free the secondary iSCSI task. 7. Then the reference count of the primary iSCSI task becomes zero, and its bdev_io is freed. However, LUN I/O channel is already freed and freeing bdev_io fails. This issue is caused by separating iSCSI task allocation and submission. For write I/O, we don't have to keep bdev_io after getting completion of it from the bdev layer. This applies to other non-read I/O types. So for non-read I/O, free bdev_io after getting SCSI status in bdev_scsi_task_complete_cmd(), and for read I/O, set bdev_io to task as same as before. The next patch will do the same for management task. Signed-off-by: Shuhei Matsumoto Change-Id: I530fb491514880ce41858e1bea55d422d606dfc4 Reviewed-on: https://review.gerrithub.io/c/spdk/spdk/+/471695 Tested-by: SPDK CI Jenkins Reviewed-by: Changpeng Liu Reviewed-by: Jim Harris Reviewed-by: Ben Walker Community-CI: Broadcom SPDK FC-NVMe CI --- lib/scsi/scsi_bdev.c | 18 +++++++++++++++++- test/unit/lib/scsi/scsi_bdev.c/scsi_bdev_ut.c | 6 +----- 2 files changed, 18 insertions(+), 6 deletions(-) diff --git a/lib/scsi/scsi_bdev.c b/lib/scsi/scsi_bdev.c index e4488dd9c..2c9c05cef 100644 --- a/lib/scsi/scsi_bdev.c +++ b/lib/scsi/scsi_bdev.c @@ -1166,9 +1166,25 @@ bdev_scsi_task_complete_cmd(struct spdk_bdev_io *bdev_io, bool success, struct spdk_scsi_task *task = cb_arg; int sc, sk, asc, ascq; + spdk_bdev_io_get_scsi_status(bdev_io, &sc, &sk, &asc, &ascq); + + spdk_bdev_free_io(bdev_io); + + spdk_scsi_task_set_status(task, sc, sk, asc, ascq); + spdk_scsi_lun_complete_task(task->lun, task); +} + +static void +bdev_scsi_read_task_complete_cmd(struct spdk_bdev_io *bdev_io, bool success, + void *cb_arg) +{ + struct spdk_scsi_task *task = cb_arg; + int sc, sk, asc, ascq; + task->bdev_io = bdev_io; spdk_bdev_io_get_scsi_status(bdev_io, &sc, &sk, &asc, &ascq); + spdk_scsi_task_set_status(task, sc, sk, asc, ascq); spdk_scsi_lun_complete_task(task->lun, task); } @@ -1354,7 +1370,7 @@ bdev_scsi_readwrite(struct spdk_scsi_task *task, if (is_read) { rc = spdk_bdev_readv_blocks(bdev_desc, bdev_ch, task->iovs, task->iovcnt, offset_blocks, num_blocks, - bdev_scsi_task_complete_cmd, task); + bdev_scsi_read_task_complete_cmd, task); } else { rc = spdk_bdev_writev_blocks(bdev_desc, bdev_ch, task->iovs, task->iovcnt, offset_blocks, num_blocks, diff --git a/test/unit/lib/scsi/scsi_bdev.c/scsi_bdev_ut.c b/test/unit/lib/scsi/scsi_bdev.c/scsi_bdev_ut.c index 8c7c1018c..84310375a 100644 --- a/test/unit/lib/scsi/scsi_bdev.c/scsi_bdev_ut.c +++ b/test/unit/lib/scsi/scsi_bdev.c/scsi_bdev_ut.c @@ -60,11 +60,7 @@ spdk_bdev_io_type_supported(struct spdk_bdev *bdev, enum spdk_bdev_io_type io_ty return false; } -void -spdk_bdev_free_io(struct spdk_bdev_io *bdev_io) -{ - CU_ASSERT(0); -} +DEFINE_STUB_V(spdk_bdev_free_io, (struct spdk_bdev_io *bdev_io)); DEFINE_STUB(spdk_bdev_get_name, const char *, (const struct spdk_bdev *bdev), "test");