Commit 9b7f5933 authored by Joel Cunningham's avatar Joel Cunningham Committed by Tomasz Zawadzki
Browse files

lib/nvmf: clear all flags during req re-use 



struct spdk_nvmf_request is re-used by the transport layer for subsequent
requests. The transports have inconsistent clearing of these flags,
which can cause some dangerous bugs when the request structure is reused.

The recent nvmf reservation serialization changes rely on the
reservation_queued flag and if that was set due to reuse, the
reservation would not be queued and will be dropped if an existing
reservation request was in-progross. If nothing is in-progress, the
request would continue without being enqueued.

The flags have a uint8_t union'd with them, so we can clear that for
each transport.  tcp clears in the get request while rdma and fc clear
in the free request function.

Add unit test to validate tcp requests have cleared flags.

Change-Id: I66bcc2ce7cc4bcdd590336160e8a7aa6629e2c5b
Signed-off-by: default avatarJoel Cunningham <joel.cunningham@oracle.com>
Reviewed-on: https://review.spdk.io/c/spdk/spdk/+/26697


Community-CI: Mellanox Build Bot
Reviewed-by: default avatarAleksey Marchuk <alexeymar@nvidia.com>
Reviewed-by: default avatarJacek Kalwas <jacek.kalwas@nutanix.com>
Tested-by: default avatarSPDK Automated Test System <spdkbot@gmail.com>
parent 70049bbc

lib/nvmf/fc.c

+1 −0
Original line number Diff line number Diff line
@@ -1583,6 +1583,7 @@ _nvmf_fc_request_free(struct spdk_nvmf_fc_request *fc_req) 
					       group->transport);

	}

	fc_req->req.iovcnt = 0;

	fc_req->req.raw = 0; /* clear all flags */



	/* Free Fc request */

	nvmf_fc_conn_free_fc_request(fc_req->fc_conn, fc_req);

lib/nvmf/rdma.c

+1 −1
Original line number Diff line number Diff line
@@ -2001,8 +2001,8 @@ _nvmf_rdma_request_free(struct spdk_nvmf_rdma_request *rdma_req, 
	nvmf_rdma_request_free_data(rdma_req, rtransport);

	rdma_req->req.length = 0;

	rdma_req->req.iovcnt = 0;

	rdma_req->req.raw = 0; /* clear all flags */

	rdma_req->offset = 0;

	rdma_req->req.dif_enabled = false;

	rdma_req->fused_failed = false;

	rdma_req->transfer_wr = NULL;

	if (rdma_req->fused_pair) {

lib/nvmf/tcp.c

+1 −1
Original line number Diff line number Diff line
@@ -457,7 +457,7 @@ nvmf_tcp_req_get(struct spdk_nvmf_tcp_qpair *tqpair) 
	memset(&tcp_req->rsp, 0, sizeof(tcp_req->rsp));

	tcp_req->h2c_offset = 0;

	tcp_req->has_in_capsule_data = false;

	tcp_req->req.dif_enabled = false;

	tcp_req->req.raw = 0; /* clear all flags */

	tcp_req->req.zcopy_phase = NVMF_ZCOPY_PHASE_NONE;



	TAILQ_REMOVE(&tqpair->tcp_req_free_queue, tcp_req, state_link);

test/unit/lib/nvmf/tcp.c/tcp_ut.c

+40 −0
Original line number Diff line number Diff line
@@ -1592,6 +1592,45 @@ test_nvmf_tcp_tls_generate_tls_psk(void) 
					  NVME_TCP_CIPHER_AES_128_GCM_SHA256) < 0);

}



static void

test_nvmf_tcp_get_request_resuse_flags(void)

{

	struct spdk_nvmf_tcp_qpair tqpair = {};

	struct spdk_nvmf_tcp_req tcp_req1 = {};

	union nvmf_c2h_msg rsp = {};

	struct spdk_nvmf_tcp_req *tcp_req;



	TAILQ_INIT(&tqpair.tcp_req_free_queue);

	TAILQ_INIT(&tqpair.tcp_req_working_queue);



	tcp_req1.req.qpair = &tqpair.qpair;

	tcp_req1.req.cmd = (union nvmf_h2c_msg *)&tcp_req1.cmd;

	tcp_req1.req.rsp = &rsp;



	/* Insert back into free queue with req flags */

	tcp_req1.req.raw = 0xFF;

	SPDK_CU_ASSERT_FATAL(tcp_req1.req.data_from_pool);

	SPDK_CU_ASSERT_FATAL(tcp_req1.req.dif_enabled);

	SPDK_CU_ASSERT_FATAL(tcp_req1.req.first_fused);

	SPDK_CU_ASSERT_FATAL(tcp_req1.req.reservation_queued);



	TAILQ_INSERT_TAIL(&tqpair.tcp_req_free_queue, &tcp_req1, state_link);

	tqpair.state_cntr[TCP_REQUEST_STATE_FREE]++;

	tqpair.state = NVMF_TCP_QPAIR_STATE_RUNNING;



	tcp_req = nvmf_tcp_req_get(&tqpair);

	SPDK_CU_ASSERT_FATAL(tcp_req != NULL);

	/* Validate flags are not set */

	SPDK_CU_ASSERT_FATAL(tcp_req->req.raw == 0);

	SPDK_CU_ASSERT_FATAL(!tcp_req->req.data_from_pool);

	SPDK_CU_ASSERT_FATAL(!tcp_req->req.dif_enabled);

	SPDK_CU_ASSERT_FATAL(!tcp_req->req.first_fused);

	SPDK_CU_ASSERT_FATAL(!tcp_req->req.reservation_queued);

	/* Validate additional fields */

	SPDK_CU_ASSERT_FATAL(tcp_req->req.zcopy_phase == NVMF_ZCOPY_PHASE_NONE);

	SPDK_CU_ASSERT_FATAL(tcp_req->state == TCP_REQUEST_STATE_NEW);

}



int

main(int argc, char **argv)

{
@@ -1619,6 +1658,7 @@ main(int argc, char **argv) 
	CU_ADD_TEST(suite, test_nvmf_tcp_tls_generate_psk_id);

	CU_ADD_TEST(suite, test_nvmf_tcp_tls_generate_retained_psk);

	CU_ADD_TEST(suite, test_nvmf_tcp_tls_generate_tls_psk);

	CU_ADD_TEST(suite, test_nvmf_tcp_get_request_resuse_flags);



	num_failures = spdk_ut_run_tests(argc, argv, NULL);

	CU_cleanup_registry();