Commit 7241a075 authored by Mike Gerdts's avatar Mike Gerdts Committed by Jim Harris
Browse files

bdev: hold spinlock while changing claim_module 



This closes races between concurrent spdk_bdev_module_claim_bdev()
and/or spdk_bdev_module_release_bdev() calls affecting the same bdev by
holding bdev->internal.spinlock while claiming and releasing a bdev. It
also closes a potential TOCTOU bug in that optimizing compilers probably
already eliminate in bdev_finish_unregister_bdevs_iter() and documents
that bdev->internal.claim_module is protected by
bdev->internal.spinlock.

This can be removed when the bdev_register_examine_thread deprecation
is removed.

Signed-off-by: default avatarMike Gerdts <mgerdts@nvidia.com>
Change-Id: Ib48552df065d5172139a61bbc00b391f36552c0c
Reviewed-on: https://review.spdk.io/gerrit/c/spdk/spdk/+/15282


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

include/spdk/bdev_module.h

+2 −1
Original line number Diff line number Diff line
@@ -524,7 +524,8 @@ struct spdk_bdev { 


		/**

		 * Pointer to the module that has claimed this bdev for purposes of creating virtual

		 *  bdevs on top of it.  Set to NULL if the bdev has not been claimed.

		 * bdevs on top of it. Set to NULL if the bdev has not been claimed. Must hold

		 * spinlock on all updates.

		 */

		struct spdk_bdev_module *claim_module;

lib/bdev/bdev.c

+12 −0
Original line number Diff line number Diff line
@@ -1818,11 +1818,14 @@ bdev_finish_unregister_bdevs_iter(void *cb_arg, int bdeverrno) 
	 */

	for (bdev = TAILQ_LAST(&g_bdev_mgr.bdevs, spdk_bdev_list);

	     bdev; bdev = TAILQ_PREV(bdev, spdk_bdev_list, internal.link)) {

		spdk_spin_lock(&bdev->internal.spinlock);

		if (bdev->internal.claim_module != NULL) {

			SPDK_DEBUGLOG(bdev, "Skipping claimed bdev '%s'(<-'%s').\n",

				      bdev->name, bdev->internal.claim_module->name);

			spdk_spin_unlock(&bdev->internal.spinlock);

			continue;

		}

		spdk_spin_unlock(&bdev->internal.spinlock);



		SPDK_DEBUGLOG(bdev, "Unregistering bdev '%s'\n", bdev->name);

		spdk_bdev_unregister(bdev, bdev_finish_unregister_bdevs_iter, bdev);
@@ -7276,9 +7279,12 @@ int 
spdk_bdev_module_claim_bdev(struct spdk_bdev *bdev, struct spdk_bdev_desc *desc,

			    struct spdk_bdev_module *module)

{

	spdk_spin_lock(&bdev->internal.spinlock);



	if (bdev->internal.claim_module != NULL) {

		SPDK_ERRLOG("bdev %s already claimed by module %s\n", bdev->name,

			    bdev->internal.claim_module->name);

		spdk_spin_unlock(&bdev->internal.spinlock);

		return -EPERM;

	}
@@ -7287,14 +7293,20 @@ spdk_bdev_module_claim_bdev(struct spdk_bdev *bdev, struct spdk_bdev_desc *desc, 
	}



	bdev->internal.claim_module = module;



	spdk_spin_unlock(&bdev->internal.spinlock);

	return 0;

}



void

spdk_bdev_module_release_bdev(struct spdk_bdev *bdev)

{

	spdk_spin_lock(&bdev->internal.spinlock);



	assert(bdev->internal.claim_module != NULL);

	bdev->internal.claim_module = NULL;



	spdk_spin_unlock(&bdev->internal.spinlock);

}



struct spdk_bdev *