Commit ffd9f746 authored by Allen Zhu's avatar Allen Zhu Committed by Konrad Sztyber
Browse files

bdev/nvme: Fix crash due to NULL io_path 



Before calling _bdev_nvme_reset_io(), bio->io_path will be set
to NULL. An assert has already been added to guarentee it.

But in _bdev_nvme_reset_io(), if error happens before setting
bio->io_path, the caller will keep it NULL.

For example, in bdev_nvme_freeze_bdev_channel_done, it will
call bdev_nvme_reset_io_continue with NULL io_path. In this
case, it will crash because prev_io_path is still NULL in
_bdev_nvme_reset_io_continue.

Change-Id: I97c28167322af5ad9df9c293f00d61b035e33268
Signed-off-by: default avatarAllen Zhu <allenzhu@nvidia.com>
Reviewed-on: https://review.spdk.io/gerrit/c/spdk/spdk/+/25222


Community-CI: Mellanox Build Bot
Community-CI: Broadcom CI <spdk-ci.pdl@broadcom.com>
Reviewed-by: default avatarShuhei Matsumoto <smatsumoto@nvidia.com>
Reviewed-by: default avatarKonrad Sztyber <konrad.sztyber@intel.com>
Tested-by: default avatarSPDK CI Jenkins <sys_sgci@intel.com>
Reviewed-by: default avatarAleksey Marchuk <alexeymar@nvidia.com>
parent ee513ce4

module/bdev/nvme/bdev_nvme.c

+3 −3
Original line number Diff line number Diff line
@@ -2941,15 +2941,15 @@ _bdev_nvme_reset_io(struct nvme_io_path *io_path, struct nvme_bdev_io *bio) 
	struct nvme_ctrlr_channel *ctrlr_ch;

	int rc;



	assert(bio->io_path == NULL);

	bio->io_path = io_path;



	rc = nvme_ctrlr_op(io_path->qpair->ctrlr, NVME_CTRLR_OP_RESET,

			   bdev_nvme_reset_io_continue, bio);

	if (rc != 0 && rc != -EBUSY) {

		return rc;

	}



	assert(bio->io_path == NULL);

	bio->io_path = io_path;



	if (rc == -EBUSY) {

		ctrlr_ch = io_path->qpair->ctrlr_ch;

		assert(ctrlr_ch != NULL);

test/unit/lib/bdev/nvme/bdev_nvme.c/bdev_nvme_ut.c

+33 −0
Original line number Diff line number Diff line
@@ -4517,6 +4517,39 @@ test_reset_bdev_ctrlr(void) 
	CU_ASSERT(ctrlr2->is_failed == false);

	CU_ASSERT(curr_path2->last_failed_tsc == 0);



	/* Reset of the first path failes, reset of the second path succeeds.

	 * Since we have at least one working path we should not fail RESET IO.

	 *

	 * Here, reset of the first path fails immediately because it is disabled.

	 *

	 * The purpose is to verify the fix. We had a bug that bdev_io did not

	 * hold io_path when reset of it failed immediately, and then continue

	 * operation caused NULL pointer access.

	 */

	nvme_ctrlr1->disabled = true;

	ctrlr1->is_failed = true;

	curr_path1->last_failed_tsc = spdk_get_ticks();

	ctrlr2->is_failed = true;

	curr_path2->last_failed_tsc = spdk_get_ticks();

	first_bdev_io->internal.status = SPDK_BDEV_IO_STATUS_FAILED;



	set_thread(0);

	bdev_nvme_submit_request(ch1, first_bdev_io);



	poll_threads();

	spdk_delay_us(g_opts.nvme_adminq_poll_period_us);

	poll_threads();



	CU_ASSERT(ctrlr1->is_failed == true);

	CU_ASSERT(curr_path1->last_failed_tsc != 0);

	CU_ASSERT(ctrlr2->is_failed == false);

	CU_ASSERT(curr_path2->last_failed_tsc == 0);

	CU_ASSERT(first_bdev_io->internal.status == SPDK_BDEV_IO_STATUS_SUCCESS);



	nvme_ctrlr1->disabled = false;

	ctrlr1->is_failed = false;

	curr_path1->last_failed_tsc = 0;



	set_thread(0);



	spdk_put_io_channel(ch1);