From 7241a075be6709fa3b702613a26ee656d1056a3b Mon Sep 17 00:00:00 2001 From: Mike Gerdts Date: Wed, 26 Oct 2022 23:13:59 -0500 Subject: [PATCH] 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: Mike Gerdts Change-Id: Ib48552df065d5172139a61bbc00b391f36552c0c Reviewed-on: https://review.spdk.io/gerrit/c/spdk/spdk/+/15282 Tested-by: SPDK CI Jenkins Reviewed-by: Shuhei Matsumoto Reviewed-by: Jim Harris Community-CI: Mellanox Build Bot --- include/spdk/bdev_module.h | 3 ++- lib/bdev/bdev.c | 12 ++++++++++++ 2 files changed, 14 insertions(+), 1 deletion(-) diff --git a/include/spdk/bdev_module.h b/include/spdk/bdev_module.h index 85e39becb..5831ff989 100644 --- a/include/spdk/bdev_module.h +++ b/include/spdk/bdev_module.h @@ -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; diff --git a/lib/bdev/bdev.c b/lib/bdev/bdev.c index 3ac9a9745..80e642531 100644 --- a/lib/bdev/bdev.c +++ b/lib/bdev/bdev.c @@ -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 *