Commit a066f0c3 authored by Shuhei Matsumoto's avatar Shuhei Matsumoto Committed by Tomasz Zawadzki
Browse files

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: default avatarShuhei Matsumoto <shuhei.matsumoto.xt@hitachi.com>
Change-Id: Idaa610544dc5cb659c387fcd38a2b4b97cbd06e5
Reviewed-on: https://review.spdk.io/gerrit/c/spdk/spdk/+/8398


Reviewed-by: default avatarBen Walker <benjamin.walker@intel.com>
Reviewed-by: default avatarZiye Yang <ziye.yang@intel.com>
Reviewed-by: default avatarAleksey Marchuk <alexeymar@mellanox.com>
Reviewed-by: default avatarMonica Kenguva <monica.kenguva@intel.com>
Tested-by: default avatarSPDK CI Jenkins <sys_sgci@intel.com>
Community-CI: Mellanox Build Bot
parent 793119f0

lib/nvme/nvme_ctrlr.c

+22 −6
Original line number Diff line number Diff line
@@ -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);

lib/nvme/nvme_internal.h

+3 −2
Original line number Diff line number Diff line
@@ -880,6 +880,7 @@ struct spdk_nvme_ctrlr { 
	STAILQ_HEAD(, nvme_io_msg_producer) io_producers;



	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 */

test/unit/lib/nvme/nvme_ctrlr.c/nvme_ctrlr_ut.c

+78 −0
Original line number Diff line number Diff line
@@ -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();