From 95a04949024c5ddada1c3425669df820183f3e79 Mon Sep 17 00:00:00 2001 From: Artur Paszkiewicz Date: Thu, 3 Feb 2022 16:43:25 +0100 Subject: [PATCH] module/raid: fix device destruction Move device cleanup to spdk_io_device_unregister() callback. This fixes a case when the device would be freed before its last io channel was closed, leading to use after free condition. Repurpose raid_bdev_free() to actually free the bdev. Signed-off-by: Artur Paszkiewicz Change-Id: Ib667b4d5ac1b34a0f2dda69f6b0775d9363dbfee Reviewed-on: https://review.spdk.io/gerrit/c/spdk/spdk/+/11398 Tested-by: SPDK CI Jenkins Community-CI: Mellanox Build Bot Reviewed-by: Ben Walker Reviewed-by: Tomasz Zawadzki Reviewed-by: Jim Harris --- module/bdev/raid/bdev_raid.c | 75 ++++++++++--------- .../lib/bdev/raid/bdev_raid.c/bdev_raid_ut.c | 22 ++++-- 2 files changed, 57 insertions(+), 40 deletions(-) diff --git a/module/bdev/raid/bdev_raid.c b/module/bdev/raid/bdev_raid.c index e08ebfbd0..e88418520 100644 --- a/module/bdev/raid/bdev_raid.c +++ b/module/bdev/raid/bdev_raid.c @@ -160,7 +160,7 @@ raid_bdev_destroy_cb(void *io_device, void *ctx_buf) /* * brief: - * raid_bdev_cleanup is used to cleanup and free raid_bdev related data + * raid_bdev_cleanup is used to cleanup raid_bdev related data * structures. * params: * raid_bdev - pointer to raid_bdev @@ -181,14 +181,26 @@ raid_bdev_cleanup(struct raid_bdev *raid_bdev) assert(0); } TAILQ_REMOVE(&g_raid_bdev_list, raid_bdev, global_link); - free(raid_bdev->bdev.name); free(raid_bdev->base_bdev_info); if (raid_bdev->config) { raid_bdev->config->raid_bdev = NULL; } +} + +static void +raid_bdev_free(struct raid_bdev *raid_bdev) +{ + free(raid_bdev->bdev.name); free(raid_bdev); } +static void +raid_bdev_cleanup_and_free(struct raid_bdev *raid_bdev) +{ + raid_bdev_cleanup(raid_bdev); + raid_bdev_free(raid_bdev); +} + /* * brief: * wrapper for the bdev close operation @@ -232,14 +244,29 @@ raid_bdev_free_base_bdev_resource(struct raid_bdev *raid_bdev, raid_bdev->num_base_bdevs_discovered--; } +static void +raid_bdev_io_device_unregister_cb(void *io_device) +{ + struct raid_bdev *raid_bdev = io_device; + + if (raid_bdev->num_base_bdevs_discovered == 0) { + /* Free raid_bdev when there are no base bdevs left */ + SPDK_DEBUGLOG(bdev_raid, "raid bdev base bdevs is 0, going to free all in destruct\n"); + raid_bdev_cleanup(raid_bdev); + spdk_bdev_destruct_done(&raid_bdev->bdev, 0); + raid_bdev_free(raid_bdev); + } else { + spdk_bdev_destruct_done(&raid_bdev->bdev, 0); + } +} + /* * brief: * raid_bdev_destruct is the destruct function table pointer for raid bdev * params: * ctxt - pointer to raid_bdev * returns: - * 0 - success - * non zero - failure + * 1 - success (deferred completion) */ static int raid_bdev_destruct(void *ctxt) @@ -272,15 +299,9 @@ raid_bdev_destruct(void *ctxt) raid_bdev->module->stop(raid_bdev); } - spdk_io_device_unregister(raid_bdev, NULL); + spdk_io_device_unregister(raid_bdev, raid_bdev_io_device_unregister_cb); - if (raid_bdev->num_base_bdevs_discovered == 0) { - /* Free raid_bdev when there are no base bdevs left */ - SPDK_DEBUGLOG(bdev_raid, "raid bdev base bdevs is 0, going to free all in destruct\n"); - raid_bdev_cleanup(raid_bdev); - } - - return 0; + return 1; } void @@ -726,26 +747,6 @@ raid_bdev_config_cleanup(struct raid_bdev_config *raid_cfg) free(raid_cfg); } -/* - * brief: - * raid_bdev_free is the raid bdev function table function pointer. This is - * called on bdev free path - * params: - * none - * returns: - * none - */ -static void -raid_bdev_free(void) -{ - struct raid_bdev_config *raid_cfg, *tmp; - - SPDK_DEBUGLOG(bdev_raid, "raid_bdev_free\n"); - TAILQ_FOREACH_SAFE(raid_cfg, &g_raid_config.raid_bdev_config_head, link, tmp) { - raid_bdev_config_cleanup(raid_cfg); - } -} - /* brief * raid_bdev_config_find_by_name is a helper function to find raid bdev config * by name as key. @@ -945,8 +946,12 @@ raid_bdev_fini_start(void) static void raid_bdev_exit(void) { + struct raid_bdev_config *raid_cfg, *tmp; + SPDK_DEBUGLOG(bdev_raid, "raid_bdev_exit\n"); - raid_bdev_free(); + TAILQ_FOREACH_SAFE(raid_cfg, &g_raid_config.raid_bdev_config_head, link, tmp) { + raid_bdev_config_cleanup(raid_cfg); + } } /* @@ -1339,7 +1344,7 @@ raid_bdev_remove_base_bdev(struct spdk_bdev *base_bdev) raid_bdev_free_base_bdev_resource(raid_bdev, base_info); if (raid_bdev->num_base_bdevs_discovered == 0) { /* There is no base bdev for this raid, so free the raid device. */ - raid_bdev_cleanup(raid_bdev); + raid_bdev_cleanup_and_free(raid_bdev); return; } } @@ -1430,7 +1435,7 @@ raid_bdev_remove_base_devices(struct raid_bdev_config *raid_cfg, if (raid_bdev->num_base_bdevs_discovered == 0) { /* There is no base bdev for this raid, so free the raid device. */ - raid_bdev_cleanup(raid_bdev); + raid_bdev_cleanup_and_free(raid_bdev); if (cb_fn) { cb_fn(cb_arg, 0); } diff --git a/test/unit/lib/bdev/raid/bdev_raid.c/bdev_raid_ut.c b/test/unit/lib/bdev/raid/bdev_raid.c/bdev_raid_ut.c index 45222d0bc..f098ff233 100644 --- a/test/unit/lib/bdev/raid/bdev_raid.c/bdev_raid_ut.c +++ b/test/unit/lib/bdev/raid/bdev_raid.c/bdev_raid_ut.c @@ -210,7 +210,7 @@ check_and_remove_raid_bdev(struct raid_bdev_config *raid_cfg) } } assert(raid_bdev->num_base_bdevs_discovered == 0); - raid_bdev_cleanup(raid_bdev); + raid_bdev_cleanup_and_free(raid_bdev); } /* Reset globals */ @@ -345,14 +345,26 @@ spdk_bdev_unmap_blocks(struct spdk_bdev_desc *desc, struct spdk_io_channel *ch, return g_bdev_io_submit_status; } +void +spdk_bdev_destruct_done(struct spdk_bdev *bdev, int bdeverrno) +{ + CU_ASSERT(bdeverrno == 0); + SPDK_CU_ASSERT_FATAL(bdev->internal.unregister_cb != NULL); + bdev->internal.unregister_cb(bdev->internal.unregister_ctx, bdeverrno); +} + void spdk_bdev_unregister(struct spdk_bdev *bdev, spdk_bdev_unregister_cb cb_fn, void *cb_arg) { - bdev->fn_table->destruct(bdev->ctxt); + int ret; - if (cb_fn) { - cb_fn(cb_arg, 0); - } + bdev->internal.unregister_cb = cb_fn; + bdev->internal.unregister_ctx = cb_arg; + + ret = bdev->fn_table->destruct(bdev->ctxt); + CU_ASSERT(ret == 1); + + poll_threads(); } int