Commit d0038b70 authored by Nathan Claudel's avatar Nathan Claudel Committed by Konrad Sztyber
Browse files

bdev: fix use-after-free in bdev registration 



When a bdev is registered, it is examined by the bdev modules before the
bdev register even is notified.

Examination may be asychronous, e.g. when the bdev module has to perform
I/O on the new bdev.

This causes a race condition where the bdev might be destroyed while
examination is not finished. Then, once all modules have signaled that
examination is done, `bdev_register_finished` makes an invalid access to
the freed bdev pointer.

To fix this, defer the unregistration until the examine is completed by
opening a descriptor on the bdev.

Change-Id: I79a2faa96c1c893fc1cee645fbe31f689b03ea4a
Signed-off-by: default avatarNathan Claudel <nclaudel@kalray.eu>
Reviewed-on: https://review.spdk.io/gerrit/c/spdk/spdk/+/13630


Community-CI: Mellanox Build Bot
Reviewed-by: default avatarShuhei Matsumoto <smatsumoto@nvidia.com>
Reviewed-by: default avatarJim Harris <james.r.harris@intel.com>
Reviewed-by: default avatarKonrad Sztyber <konrad.sztyber@intel.com>
Tested-by: default avatarSPDK CI Jenkins <sys_sgci@intel.com>
parent d974bad6

lib/bdev/bdev.c

+48 −23
Original line number Diff line number Diff line
@@ -6163,29 +6163,6 @@ bdev_destroy_cb(void *io_device) 
	}

}



static void

bdev_register_finished(void *arg)

{

	struct spdk_bdev *bdev = arg;



	spdk_notify_send("bdev_register", spdk_bdev_get_name(bdev));

}



int

spdk_bdev_register(struct spdk_bdev *bdev)

{

	int rc = bdev_register(bdev);



	if (rc == 0) {

		/* Examine configuration before initializing I/O */

		bdev_examine(bdev);



		spdk_bdev_wait_for_examine(bdev_register_finished, bdev);

	}



	return rc;

}



void

spdk_bdev_destruct_done(struct spdk_bdev *bdev, int bdeverrno)

{
@@ -6584,6 +6561,54 @@ spdk_bdev_close(struct spdk_bdev_desc *desc) 
	pthread_mutex_unlock(&g_bdev_mgr.mutex);

}



static void

bdev_register_finished(void *arg)

{

	struct spdk_bdev_desc *desc = arg;

	struct spdk_bdev *bdev = spdk_bdev_desc_get_bdev(desc);



	spdk_notify_send("bdev_register", spdk_bdev_get_name(bdev));



	bdev_close(bdev, desc);

}



int

spdk_bdev_register(struct spdk_bdev *bdev)

{

	struct spdk_bdev_desc *desc;

	int rc;



	rc = bdev_register(bdev);

	if (rc != 0) {

		return rc;

	}



	/* A descriptor is opened to prevent bdev deletion during examination */

	rc = bdev_desc_alloc(bdev, _tmp_bdev_event_cb, NULL, &desc);

	if (rc != 0) {

		spdk_bdev_unregister(bdev, NULL, NULL);

		return rc;

	}



	rc = bdev_open(bdev, false, desc);

	if (rc != 0) {

		bdev_desc_free(desc);

		spdk_bdev_unregister(bdev, NULL, NULL);

		return rc;

	}



	/* Examine configuration before initializing I/O */

	bdev_examine(bdev);



	rc = spdk_bdev_wait_for_examine(bdev_register_finished, desc);

	if (rc != 0) {

		bdev_close(bdev, desc);

		spdk_bdev_unregister(bdev, NULL, NULL);

	}



	return rc;

}



int

spdk_bdev_module_claim_bdev(struct spdk_bdev *bdev, struct spdk_bdev_desc *desc,

			    struct spdk_bdev_module *module)

test/unit/lib/bdev/bdev.c/bdev_ut.c

+3 −0
Original line number Diff line number Diff line
@@ -451,6 +451,7 @@ allocate_bdev(char *name) 
	bdev->blocklen = 512;



	rc = spdk_bdev_register(bdev);

	poll_threads();

	CU_ASSERT(rc == 0);



	return bdev;
@@ -470,6 +471,7 @@ allocate_vbdev(char *name) 
	bdev->module = &vbdev_ut_if;



	rc = spdk_bdev_register(bdev);

	poll_threads();

	CU_ASSERT(rc == 0);



	return bdev;
@@ -807,6 +809,7 @@ num_blocks_test(void) 
	bdev.fn_table = &fn_table;

	bdev.module = &bdev_ut_if;

	spdk_bdev_register(&bdev);

	poll_threads();

	spdk_bdev_notify_blockcnt_change(&bdev, 50);



	/* Growing block number */