From a066f0c3fb583bd7a3c204f0f9af523d7a4b265a Mon Sep 17 00:00:00 2001 From: Shuhei Matsumoto Date: Thu, 17 Jun 2021 09:41:42 +0900 Subject: [PATCH] nvme: Fix the bug that assumed ANA group descriptor is 8-bytes aligned This fix is as same as for NVMe bdev module. If a ANA log page has two or more ANA group descriptors, the second or later of ANA group descriptors will not be 8-bytes aligned. Then runtime error would occur as follows: runtime error: member access within misaligned address 0x612000000074 for type 'const struct spdk_nvme_ana_group_descriptor', which requires 8 byte alignment nvmf_get_ana_log_page() in lib/nvmf/ctrlr.c creates a ANA log page data and processes 8 bytes alignment correctly because we got the same runtime error before. However, lib/nvme had been missed at that time. Signed-off-by: Shuhei Matsumoto Change-Id: Idaa610544dc5cb659c387fcd38a2b4b97cbd06e5 Reviewed-on: https://review.spdk.io/gerrit/c/spdk/spdk/+/8398 Reviewed-by: Ben Walker Reviewed-by: Ziye Yang Reviewed-by: Aleksey Marchuk Reviewed-by: Monica Kenguva Tested-by: SPDK CI Jenkins Community-CI: Mellanox Build Bot --- lib/nvme/nvme_ctrlr.c | 28 +++++-- lib/nvme/nvme_internal.h | 5 +- .../lib/nvme/nvme_ctrlr.c/nvme_ctrlr_ut.c | 78 +++++++++++++++++++ 3 files changed, 103 insertions(+), 8 deletions(-) diff --git a/lib/nvme/nvme_ctrlr.c b/lib/nvme/nvme_ctrlr.c index b05c46d1b..3bd63dacb 100644 --- a/lib/nvme/nvme_ctrlr.c +++ b/lib/nvme/nvme_ctrlr.c @@ -715,6 +715,11 @@ nvme_ctrlr_init_ana_log_page(struct spdk_nvme_ctrlr *ctrlr) NVME_CTRLR_ERRLOG(ctrlr, "could not allocate ANA log page buffer\n"); return -ENXIO; } + ctrlr->copied_ana_desc = calloc(1, ana_log_page_size); + if (ctrlr->copied_ana_desc == NULL) { + NVME_CTRLR_ERRLOG(ctrlr, "could not allocate a buffer to parse ANA descriptor\n"); + return -ENOMEM; + } ctrlr->ana_log_page_size = ana_log_page_size; ctrlr->log_page_supported[SPDK_NVME_LOG_ASYMMETRIC_NAMESPACE_ACCESS] = true; @@ -750,23 +755,32 @@ int nvme_ctrlr_parse_ana_log_page(struct spdk_nvme_ctrlr *ctrlr, spdk_nvme_parse_ana_log_page_cb cb_fn, void *cb_arg) { - struct spdk_nvme_ana_group_descriptor *desc; - uint32_t i; + struct spdk_nvme_ana_group_descriptor *copied_desc; + uint8_t *orig_desc; + uint32_t i, desc_size, copy_len; int rc = 0; if (ctrlr->ana_log_page == NULL) { return -EINVAL; } - desc = (void *)((uint8_t *)ctrlr->ana_log_page + sizeof(struct spdk_nvme_ana_page)); + copied_desc = ctrlr->copied_ana_desc; + + orig_desc = (uint8_t *)ctrlr->ana_log_page + sizeof(struct spdk_nvme_ana_page); + copy_len = ctrlr->ana_log_page_size - sizeof(struct spdk_nvme_ana_page); for (i = 0; i < ctrlr->ana_log_page->num_ana_group_desc; i++) { - rc = cb_fn(desc, cb_arg); + memcpy(copied_desc, orig_desc, copy_len); + + rc = cb_fn(copied_desc, cb_arg); if (rc != 0) { break; } - desc = (void *)((uint8_t *)desc + sizeof(struct spdk_nvme_ana_group_descriptor) + - desc->num_of_nsid * sizeof(uint32_t)); + + desc_size = sizeof(struct spdk_nvme_ana_group_descriptor) + + copied_desc->num_of_nsid * sizeof(uint32_t); + orig_desc += desc_size; + copy_len -= desc_size; } return rc; @@ -3574,7 +3588,9 @@ nvme_ctrlr_destruct_poll_async(struct spdk_nvme_ctrlr *ctrlr, spdk_bit_array_free(&ctrlr->free_io_qids); spdk_free(ctrlr->ana_log_page); + free(ctrlr->copied_ana_desc); ctrlr->ana_log_page = NULL; + ctrlr->copied_ana_desc = NULL; ctrlr->ana_log_page_size = 0; nvme_transport_ctrlr_destruct(ctrlr); diff --git a/lib/nvme/nvme_internal.h b/lib/nvme/nvme_internal.h index 20d1c4d50..c7301f161 100644 --- a/lib/nvme/nvme_internal.h +++ b/lib/nvme/nvme_internal.h @@ -879,8 +879,9 @@ struct spdk_nvme_ctrlr { STAILQ_HEAD(, nvme_io_msg_producer) io_producers; - struct spdk_nvme_ana_page *ana_log_page; - uint32_t ana_log_page_size; + struct spdk_nvme_ana_page *ana_log_page; + struct spdk_nvme_ana_group_descriptor *copied_ana_desc; + uint32_t ana_log_page_size; /* scratchpad pointer that can be used to send data between two NVME_CTRLR_STATEs */ void *tmp_ptr; diff --git a/test/unit/lib/nvme/nvme_ctrlr.c/nvme_ctrlr_ut.c b/test/unit/lib/nvme/nvme_ctrlr.c/nvme_ctrlr_ut.c index 4c3d5b3ec..f1694265b 100644 --- a/test/unit/lib/nvme/nvme_ctrlr.c/nvme_ctrlr_ut.c +++ b/test/unit/lib/nvme/nvme_ctrlr.c/nvme_ctrlr_ut.c @@ -2884,6 +2884,83 @@ test_nvme_ctrlr_set_supported_log_pages(void) sizeof(struct spdk_nvme_ana_group_descriptor) * 1 + sizeof(uint32_t) * 1); CU_ASSERT(ctrlr.log_page_supported[SPDK_NVME_LOG_ASYMMETRIC_NAMESPACE_ACCESS] == true); spdk_free(ctrlr.ana_log_page); + free(ctrlr.copied_ana_desc); +} + +#define UT_ANA_DESC_SIZE (sizeof(struct spdk_nvme_ana_group_descriptor) + \ + sizeof(uint32_t)) +static void +test_nvme_ctrlr_parse_ana_log_page(void) +{ + int rc; + struct spdk_nvme_ctrlr ctrlr = {}; + struct spdk_nvme_ns ns[3] = {}; + struct spdk_nvme_ana_page ana_hdr; + char _ana_desc[UT_ANA_DESC_SIZE]; + struct spdk_nvme_ana_group_descriptor *ana_desc; + uint32_t offset; + + ctrlr.ns = ns; + ctrlr.cdata.nn = 3; + ctrlr.cdata.nanagrpid = 3; + ctrlr.num_ns = 3; + + rc = nvme_ctrlr_init_ana_log_page(&ctrlr); + CU_ASSERT(rc == 0); + CU_ASSERT(ctrlr.ana_log_page != NULL); + CU_ASSERT(ctrlr.copied_ana_desc != NULL); + + /* + * Create ANA log page data - There are three ANA groups. + * Each ANA group has a namespace and has a different ANA state. + */ + memset(&ana_hdr, 0, sizeof(ana_hdr)); + ana_hdr.num_ana_group_desc = 3; + + SPDK_CU_ASSERT_FATAL(sizeof(ana_hdr) <= ctrlr.ana_log_page_size); + memcpy((char *)ctrlr.ana_log_page, (char *)&ana_hdr, sizeof(ana_hdr)); + offset = sizeof(ana_hdr); + + ana_desc = (struct spdk_nvme_ana_group_descriptor *)_ana_desc; + memset(ana_desc, 0, UT_ANA_DESC_SIZE); + ana_desc->num_of_nsid = 1; + + ana_desc->ana_group_id = 1; + ana_desc->ana_state = SPDK_NVME_ANA_OPTIMIZED_STATE; + ana_desc->nsid[0] = 3; + + SPDK_CU_ASSERT_FATAL(offset + UT_ANA_DESC_SIZE <= ctrlr.ana_log_page_size); + memcpy((char *)ctrlr.ana_log_page + offset, (char *)ana_desc, UT_ANA_DESC_SIZE); + offset += UT_ANA_DESC_SIZE; + + ana_desc->ana_group_id = 2; + ana_desc->ana_state = SPDK_NVME_ANA_NON_OPTIMIZED_STATE; + ana_desc->nsid[0] = 2; + + SPDK_CU_ASSERT_FATAL(offset + UT_ANA_DESC_SIZE <= ctrlr.ana_log_page_size); + memcpy((char *)ctrlr.ana_log_page + offset, (char *)ana_desc, UT_ANA_DESC_SIZE); + offset += UT_ANA_DESC_SIZE; + + ana_desc->ana_group_id = 3; + ana_desc->ana_state = SPDK_NVME_ANA_INACCESSIBLE_STATE; + ana_desc->nsid[0] = 1; + + SPDK_CU_ASSERT_FATAL(offset + UT_ANA_DESC_SIZE <= ctrlr.ana_log_page_size); + memcpy((char *)ctrlr.ana_log_page + offset, (char *)ana_desc, UT_ANA_DESC_SIZE); + + /* Parse the created ANA log page data, and update ANA states. */ + rc = nvme_ctrlr_parse_ana_log_page(&ctrlr, nvme_ctrlr_update_ns_ana_states, + &ctrlr); + CU_ASSERT(rc == 0); + CU_ASSERT(ns[0].ana_group_id == 3); + CU_ASSERT(ns[0].ana_state == SPDK_NVME_ANA_INACCESSIBLE_STATE); + CU_ASSERT(ns[1].ana_group_id == 2); + CU_ASSERT(ns[1].ana_state == SPDK_NVME_ANA_NON_OPTIMIZED_STATE); + CU_ASSERT(ns[2].ana_group_id == 1); + CU_ASSERT(ns[2].ana_state == SPDK_NVME_ANA_OPTIMIZED_STATE); + + spdk_free(ctrlr.ana_log_page); + free(ctrlr.copied_ana_desc); } int main(int argc, char **argv) @@ -2936,6 +3013,7 @@ int main(int argc, char **argv) CU_ADD_TEST(suite, test_nvme_ctrlr_ns_attr_changed); CU_ADD_TEST(suite, test_nvme_ctrlr_identify_namespaces_iocs_specific_next); CU_ADD_TEST(suite, test_nvme_ctrlr_set_supported_log_pages); + CU_ADD_TEST(suite, test_nvme_ctrlr_parse_ana_log_page); CU_basic_set_mode(CU_BRM_VERBOSE); CU_basic_run_tests();