Commit 85a61ea6 authored by Pawel Wodkowski's avatar Pawel Wodkowski Committed by Jim Harris
Browse files

scsi: fix LUN removal/hotremoval path 



bdev hotremove event is send directly to hotremove callback given
during spdk_scsi_dev_construct() call. So any bdev hotremove might be
promoted to whole SCSI device removal (like in vhost) wich will trigger
LUN removal. But after returning from hotremove callback LUN and device
might be still referenced (eg to register poller or by outstanding IO).

Even worse: spdk_scsi_dev object is not dynamicaly allocated but
returned from static array which mean there is no way to detect
use-after-free error on spdk_scsi_dev by any tool. This might lead to
using SCSI device that is freed or assigned to different device.

To fix this:
- always delete LUN using hotremove path
- defer spdk_scsi_dev delete/removal after all LUNS are really
deleted.

Change-Id: I65598bf42cd507f620095dff5d32509a0424d060
Signed-off-by: default avatarPawel Wodkowski <pawelx.wodkowski@intel.com>
Reviewed-on: https://review.gerrithub.io/393674


Tested-by: default avatarSPDK Automated Test System <sys_sgsw@intel.com>
Reviewed-by: default avatarDariusz Stojaczyk <dariuszx.stojaczyk@intel.com>
Reviewed-by: default avatarJim Harris <james.r.harris@intel.com>
Reviewed-by: default avatarBen Walker <benjamin.walker@intel.com>
parent 5c0c104b

lib/scsi/dev.c

+26 −3
Original line number Diff line number Diff line
@@ -64,28 +64,42 @@ allocate_dev(void) 
static void

free_dev(struct spdk_scsi_dev *dev)

{

	assert(dev->is_allocated == 1);

	assert(dev->removed == true);



	dev->is_allocated = 0;

}



void

spdk_scsi_dev_destruct(struct spdk_scsi_dev *dev)

{

	int lun_cnt;

	int i;



	if (dev == NULL) {

	if (dev == NULL || dev->removed) {

		return;

	}



	dev->removed = true;

	lun_cnt = 0;



	for (i = 0; i < SPDK_SCSI_DEV_MAX_LUN; i++) {

		if (dev->lun[i] == NULL) {

			continue;

		}



		/*

		 * LUN will remove itself from this dev when all outstanding IO

		 * is done. When no more LUNs, dev will be deleted.

		 */

		spdk_scsi_lun_destruct(dev->lun[i]);

		dev->lun[i] = NULL;

		lun_cnt++;

	}



	if (lun_cnt == 0) {

		free_dev(dev);

		return;

	}

}



static void
@@ -101,12 +115,21 @@ void 
spdk_scsi_dev_delete_lun(struct spdk_scsi_dev *dev,

			 struct spdk_scsi_lun *lun)

{

	int lun_cnt = 0;

	int i;



	for (i = 0; i < SPDK_SCSI_DEV_MAX_LUN; i++) {

		if (dev->lun[i] == lun) {

			dev->lun[i] = NULL;

		}



		if (dev->lun[i]) {

			lun_cnt++;

		}

	}



	if (dev->removed == true && lun_cnt == 0) {

		free_dev(dev);

	}

}

lib/scsi/lun.c

+18 −30
Original line number Diff line number Diff line
@@ -188,7 +188,12 @@ spdk_scsi_lun_hotplug(void *arg) 


	if (!spdk_scsi_lun_has_pending_tasks(lun)) {

		spdk_scsi_lun_free_io_channel(lun);

		spdk_scsi_lun_delete(lun);



		spdk_bdev_close(lun->bdev_desc);

		spdk_poller_unregister(&lun->hotplug_poller);



		spdk_scsi_dev_delete_lun(lun->dev, lun);

		free(lun);

	}

}
@@ -197,12 +202,15 @@ _spdk_scsi_lun_hot_remove(void *arg1) 
{

	struct spdk_scsi_lun *lun = arg1;



	lun->removed = true;

	if (lun->hotremove_cb) {

		lun->hotremove_cb(lun, lun->hotremove_ctx);

	}



	lun->hotplug_poller = spdk_poller_register(spdk_scsi_lun_hotplug, lun, 0);

	if (spdk_scsi_lun_has_pending_tasks(lun)) {

		lun->hotplug_poller = spdk_poller_register(spdk_scsi_lun_hotplug, lun, 10);

	} else {

		spdk_scsi_lun_hotplug(lun);

	}

}



static void
@@ -211,6 +219,11 @@ spdk_scsi_lun_hot_remove(void *remove_ctx) 
	struct spdk_scsi_lun *lun = (struct spdk_scsi_lun *)remove_ctx;

	struct spdk_thread *thread;



	if (lun->removed) {

		return;

	}



	lun->removed = true;

	if (lun->io_channel == NULL) {

		_spdk_scsi_lun_hot_remove(lun);

		return;
@@ -269,35 +282,10 @@ spdk_scsi_lun_construct(struct spdk_bdev *bdev, 
	return lun;

}



int

void

spdk_scsi_lun_destruct(struct spdk_scsi_lun *lun)

{

	spdk_bdev_close(lun->bdev_desc);

	spdk_poller_unregister(&lun->hotplug_poller);



	free(lun);



	return 0;

}



int

spdk_scsi_lun_delete(struct spdk_scsi_lun *lun)

{

	struct spdk_scsi_dev *dev;



	pthread_mutex_lock(&g_spdk_scsi.mutex);



	dev = lun->dev;



	/* Remove the LUN from the device */

	if (dev != NULL) {

		spdk_scsi_dev_delete_lun(dev, lun);

	}



	/* Destroy this lun */

	spdk_scsi_lun_destruct(lun);

	pthread_mutex_unlock(&g_spdk_scsi.mutex);

	return 0;

	spdk_scsi_lun_hot_remove(lun);

}



int spdk_scsi_lun_allocate_io_channel(struct spdk_scsi_lun *lun)

lib/scsi/scsi_internal.h

+2 −2
Original line number Diff line number Diff line
@@ -60,6 +60,7 @@ struct spdk_scsi_port { 
struct spdk_scsi_dev {

	int			id;

	int			is_allocated;

	bool			removed;



	char			name[SPDK_SCSI_DEV_MAX_NAME + 1];
@@ -125,13 +126,12 @@ typedef struct spdk_scsi_lun _spdk_scsi_lun; 
_spdk_scsi_lun *spdk_scsi_lun_construct(struct spdk_bdev *bdev,

					void (*hotremove_cb)(const struct spdk_scsi_lun *, void *),

					void *hotremove_ctx);

int spdk_scsi_lun_destruct(struct spdk_scsi_lun *lun);

void spdk_scsi_lun_destruct(struct spdk_scsi_lun *lun);



void spdk_scsi_lun_execute_task(struct spdk_scsi_lun *lun, struct spdk_scsi_task *task);

int spdk_scsi_lun_task_mgmt_execute(struct spdk_scsi_task *task, enum spdk_scsi_task_func func);

void spdk_scsi_lun_complete_task(struct spdk_scsi_lun *lun, struct spdk_scsi_task *task);

void spdk_scsi_lun_complete_mgmt_task(struct spdk_scsi_lun *lun, struct spdk_scsi_task *task);

int spdk_scsi_lun_delete(struct spdk_scsi_lun *lun);

int spdk_scsi_lun_allocate_io_channel(struct spdk_scsi_lun *lun);

void spdk_scsi_lun_free_io_channel(struct spdk_scsi_lun *lun);

bool spdk_scsi_lun_has_pending_tasks(const struct spdk_scsi_lun *lun);

test/unit/lib/scsi/dev.c/dev_ut.c

+5 −6
Original line number Diff line number Diff line
@@ -91,11 +91,10 @@ spdk_scsi_lun_construct(struct spdk_bdev *bdev, 
	return lun;

}



int

void

spdk_scsi_lun_destruct(struct spdk_scsi_lun *lun)

{

	free(lun);

	return 0;

}



struct spdk_bdev *
@@ -150,7 +149,7 @@ dev_destruct_null_dev(void) 
static void

dev_destruct_zero_luns(void)

{

	struct spdk_scsi_dev dev = { 0 };

	struct spdk_scsi_dev dev = { .is_allocated = 1 };



	/* No luns attached to the dev */
@@ -161,7 +160,7 @@ dev_destruct_zero_luns(void) 
static void

dev_destruct_null_lun(void)

{

	struct spdk_scsi_dev dev = { 0 };

	struct spdk_scsi_dev dev = { .is_allocated = 1 };



	/* pass null for the lun */

	dev.lun[0] = NULL;
@@ -173,13 +172,13 @@ dev_destruct_null_lun(void) 
static void

dev_destruct_success(void)

{

	struct spdk_scsi_dev dev = { 0 };

	struct spdk_scsi_dev dev = { .is_allocated = 1 };

	struct spdk_scsi_lun *lun;



	lun = calloc(1, sizeof(struct spdk_scsi_lun));



	/* dev with a single lun */

	dev.lun[0] = lun;

	spdk_scsi_dev_add_lun(&dev, lun, 0);



	/* free the dev */

	spdk_scsi_dev_destruct(&dev);

test/unit/lib/scsi/lun.c/lun_ut.c

+22 −25
Original line number Diff line number Diff line
@@ -215,6 +215,9 @@ lun_construct(void) 
static void

lun_destruct(struct spdk_scsi_lun *lun)

{

	/* LUN will defer its removal if there are any unfinished tasks */

	SPDK_CU_ASSERT_FATAL(TAILQ_EMPTY(&lun->tasks));



	spdk_scsi_lun_destruct(lun);

}
@@ -282,11 +285,13 @@ lun_task_mgmt_execute_abort_task_not_supported(void) 
	CU_ASSERT_TRUE(rc < 0);

	CU_ASSERT(mgmt_task.response == SPDK_SCSI_TASK_MGMT_RESP_REJECT_FUNC_NOT_SUPPORTED);



	lun_destruct(lun);



	/* task is still on the tasks list */

	CU_ASSERT_EQUAL(g_task_count, 1);

	g_task_count = 0;



	spdk_scsi_lun_complete_task(lun, &task);

	CU_ASSERT_EQUAL(g_task_count, 0);



	lun_destruct(lun);

}



static void
@@ -343,11 +348,14 @@ lun_task_mgmt_execute_abort_task_all_not_supported(void) 
	CU_ASSERT_TRUE(rc < 0);

	CU_ASSERT(mgmt_task.response == SPDK_SCSI_TASK_MGMT_RESP_REJECT_FUNC_NOT_SUPPORTED);



	lun_destruct(lun);



	/* task is still on the tasks list */

	CU_ASSERT_EQUAL(g_task_count, 1);

	g_task_count = 0;



	spdk_scsi_lun_complete_task(lun, &task);



	CU_ASSERT_EQUAL(g_task_count, 0);



	lun_destruct(lun);

}



static void
@@ -511,11 +519,15 @@ lun_execute_scsi_task_pending(void) 
	/* Assert the task has been successfully added to the tasks queue */

	CU_ASSERT(!TAILQ_EMPTY(&lun->tasks));



	lun_destruct(lun);



	/* task is still on the tasks list */

	CU_ASSERT_EQUAL(g_task_count, 1);

	g_task_count = 0;



	/* Need to complete task so LUN might be removed right now */

	spdk_scsi_lun_complete_task(lun, &task);



	CU_ASSERT_EQUAL(g_task_count, 0);



	lun_destruct(lun);

}



static void
@@ -553,13 +565,11 @@ static void 
lun_destruct_success(void)

{

	struct spdk_scsi_lun *lun;

	int rc;



	lun = lun_construct();



	rc = spdk_scsi_lun_destruct(lun);

	spdk_scsi_lun_destruct(lun);



	CU_ASSERT_EQUAL(rc, 0);

	CU_ASSERT_EQUAL(g_task_count, 0);

}
@@ -585,18 +595,6 @@ lun_construct_success(void) 
	CU_ASSERT_EQUAL(g_task_count, 0);

}



static void

lun_delete(void)

{

	struct spdk_scsi_lun *lun;

	int rc;



	lun = lun_construct();



	rc = spdk_scsi_lun_delete(lun);

	CU_ASSERT_EQUAL(rc, 0);

}



int

main(int argc, char **argv)

{
@@ -644,7 +642,6 @@ main(int argc, char **argv) 
		|| CU_add_test(suite, "destruct task - success", lun_destruct_success) == NULL

		|| CU_add_test(suite, "construct - null ctx", lun_construct_null_ctx) == NULL

		|| CU_add_test(suite, "construct - success", lun_construct_success) == NULL

		|| CU_add_test(suite, "lun_delete", lun_delete) == NULL

	) {

		CU_cleanup_registry();

		return CU_get_error();