From 8ed53eee32a2c3a96288fedc98aa5fe23b9d6b44 Mon Sep 17 00:00:00 2001 From: MengjinWu Date: Fri, 23 Sep 2022 05:41:47 +0000 Subject: [PATCH] nvmf/tcp: Fixed error handle in 'nvmf_tcp_req_parse_sgl' Fixed error handles which are violated with spec: 1. 'data length > MAXH2CDATA' is a fatal error. 2. 'ICDOFF != 0' should abort the IO. Other errors which are not defined in spec: 1. invalid sgl type 2. In-capsule Data length > In-capsule Data size Because this function runs before data part receiving, it is hard to skip the following data segment if we want to handle some error as non-fatal. Currently, we have to handle all undefined errors as fatal errors. I think after this release, we can change receving process. This will be helpful for error handling. But this work is not small. Signed-off-by: MengjinWu Change-Id: I8fc0d2d743505e49a93be19fd217e7ad6ca06622 Reviewed-on: https://review.spdk.io/gerrit/c/spdk/spdk/+/14580 Tested-by: SPDK CI Jenkins Reviewed-by: Jim Harris Reviewed-by: Ben Walker --- lib/nvmf/tcp.c | 51 +++++++++++++++---------------- test/unit/lib/nvmf/tcp.c/tcp_ut.c | 14 +++++---- 2 files changed, 32 insertions(+), 33 deletions(-) diff --git a/lib/nvmf/tcp.c b/lib/nvmf/tcp.c index aa634a892..6b08fadc8 100644 --- a/lib/nvmf/tcp.c +++ b/lib/nvmf/tcp.c @@ -2274,24 +2274,23 @@ nvmf_tcp_req_parse_sgl(struct spdk_nvmf_tcp_req *tcp_req, { struct spdk_nvmf_request *req = &tcp_req->req; struct spdk_nvme_cmd *cmd; - struct spdk_nvme_cpl *rsp; struct spdk_nvme_sgl_descriptor *sgl; struct spdk_nvmf_tcp_poll_group *tgroup; - uint32_t length; + enum spdk_nvme_tcp_term_req_fes fes; + uint32_t length, error_offset = 0; cmd = &req->cmd->nvme_cmd; - rsp = &req->rsp->nvme_cpl; sgl = &cmd->dptr.sgl1; length = sgl->unkeyed.length; if (sgl->generic.type == SPDK_NVME_SGL_TYPE_TRANSPORT_DATA_BLOCK && sgl->unkeyed.subtype == SPDK_NVME_SGL_SUBTYPE_TRANSPORT) { - if (length > transport->opts.max_io_size) { + if (spdk_unlikely(length > transport->opts.max_io_size)) { SPDK_ERRLOG("SGL length 0x%x exceeds max io size 0x%x\n", length, transport->opts.max_io_size); - rsp->status.sc = SPDK_NVME_SC_DATA_SGL_LENGTH_INVALID; - return -1; + fes = SPDK_NVME_TCP_TERM_REQ_FES_DATA_TRANSFER_LIMIT_EXCEEDED; + goto fatal_err; } /* fill request length and populate iovs */ @@ -2334,13 +2333,14 @@ nvmf_tcp_req_parse_sgl(struct spdk_nvmf_tcp_req *tcp_req, SPDK_DEBUGLOG(nvmf_tcp, "In-capsule data: offset 0x%" PRIx64 ", length 0x%x\n", offset, length); - if (offset > max_len) { - SPDK_ERRLOG("In-capsule offset 0x%" PRIx64 " exceeds capsule length 0x%x\n", - offset, max_len); - rsp->status.sc = SPDK_NVME_SC_INVALID_SGL_OFFSET; - return -1; + /* The NVMe/TCP transport does not use ICDOFF to control the in-capsule data offset. ICDOFF should be '0' */ + if (spdk_unlikely(offset != 0)) { + /* Not defined fatal error in NVMe/TCP spec, handle this error as a fatal error */ + SPDK_ERRLOG("In-capsule offset 0x%" PRIx64 " should be ZERO in NVMe/TCP\n", offset); + fes = SPDK_NVME_TCP_TERM_REQ_FES_INVALID_DATA_UNSUPPORTED_PARAMETER; + error_offset = offsetof(struct spdk_nvme_tcp_cmd, ccsqe.dptr.sgl1.address); + goto fatal_err; } - max_len -= (uint32_t)offset; if (spdk_unlikely(length > max_len)) { /* According to the SPEC we should support ICD up to 8192 bytes for admin and fabric commands */ @@ -2360,8 +2360,8 @@ nvmf_tcp_req_parse_sgl(struct spdk_nvmf_tcp_req *tcp_req, } else { SPDK_ERRLOG("In-capsule data length 0x%x exceeds capsule length 0x%x\n", length, max_len); - rsp->status.sc = SPDK_NVME_SC_DATA_SGL_LENGTH_INVALID; - return -1; + fes = SPDK_NVME_TCP_TERM_REQ_FES_DATA_TRANSFER_LIMIT_EXCEEDED; + goto fatal_err; } } else { req->data = tcp_req->buf; @@ -2381,10 +2381,14 @@ nvmf_tcp_req_parse_sgl(struct spdk_nvmf_tcp_req *tcp_req, return 0; } - + /* If we want to handle the problem here, then we can't skip the following data segment. + * Because this function runs before reading data part, now handle all errors as fatal errors. */ SPDK_ERRLOG("Invalid NVMf I/O Command SGL: Type 0x%x, Subtype 0x%x\n", sgl->generic.type, sgl->generic.subtype); - rsp->status.sc = SPDK_NVME_SC_SGL_DESCRIPTOR_TYPE_INVALID; + fes = SPDK_NVME_TCP_TERM_REQ_FES_INVALID_DATA_UNSUPPORTED_PARAMETER; + error_offset = offsetof(struct spdk_nvme_tcp_cmd, ccsqe.dptr.sgl1.generic); +fatal_err: + nvmf_tcp_send_c2h_term_req(tcp_req->pdu->qpair, tcp_req->pdu, fes, error_offset); return -1; } @@ -2629,7 +2633,6 @@ nvmf_tcp_req_process(struct spdk_nvmf_tcp_transport *ttransport, struct spdk_nvmf_tcp_req *tcp_req) { struct spdk_nvmf_tcp_qpair *tqpair; - int rc; uint32_t plen; struct nvme_tcp_pdu *pdu; enum spdk_nvmf_tcp_req_state prev_state; @@ -2726,14 +2729,7 @@ nvmf_tcp_req_process(struct spdk_nvmf_tcp_transport *ttransport, } /* Try to get a data buffer */ - rc = nvmf_tcp_req_parse_sgl(tcp_req, transport, group); - if (rc < 0) { - STAILQ_REMOVE_HEAD(&group->pending_buf_queue, buf_link); - /* Reset the tqpair receiving pdu state */ - nvmf_tcp_qpair_set_recv_state(tqpair, NVME_TCP_PDU_RECV_STATE_ERROR); - nvmf_tcp_req_set_state(tcp_req, TCP_REQUEST_STATE_READY_TO_COMPLETE); - tcp_req->req.rsp->nvme_cpl.cid = tcp_req->req.cmd->nvme_cmd.cid; - nvmf_tcp_qpair_set_recv_state(tqpair, NVME_TCP_PDU_RECV_STATE_AWAIT_PDU_READY); + if (nvmf_tcp_req_parse_sgl(tcp_req, transport, group) < 0) { break; } @@ -2909,8 +2905,9 @@ nvmf_tcp_req_process(struct spdk_nvmf_tcp_transport *ttransport, case TCP_REQUEST_STATE_READY_TO_COMPLETE: spdk_trace_record(TRACE_TCP_REQUEST_STATE_READY_TO_COMPLETE, tqpair->qpair.qid, 0, (uintptr_t)tcp_req, tqpair); - rc = request_transfer_out(&tcp_req->req); - assert(rc == 0); /* No good way to handle this currently */ + if (request_transfer_out(&tcp_req->req) != 0) { + assert(0); /* No good way to handle this currently */ + } break; case TCP_REQUEST_STATE_TRANSFERRING_CONTROLLER_TO_HOST: spdk_trace_record(TRACE_TCP_REQUEST_STATE_TRANSFERRING_CONTROLLER_TO_HOST, tqpair->qpair.qid, 0, diff --git a/test/unit/lib/nvmf/tcp.c/tcp_ut.c b/test/unit/lib/nvmf/tcp.c/tcp_ut.c index c9ee23494..68104c5f6 100644 --- a/test/unit/lib/nvmf/tcp.c/tcp_ut.c +++ b/test/unit/lib/nvmf/tcp.c/tcp_ut.c @@ -1013,6 +1013,7 @@ test_nvmf_tcp_invalid_sgl(void) struct spdk_nvmf_tcp_req tcp_req = {}; struct nvme_tcp_pdu rsp_pdu = {}; + struct nvme_tcp_pdu mgmt_pdu = {}; struct spdk_nvme_tcp_cmd *capsule_data; struct spdk_nvme_sgl_descriptor *sgl; @@ -1043,6 +1044,9 @@ test_nvmf_tcp_invalid_sgl(void) /* init tcp_req */ tcp_req.req.qpair = &tqpair.qpair; tcp_req.pdu = &rsp_pdu; + tcp_req.pdu->qpair = &tqpair; + tqpair.mgmt_pdu = &mgmt_pdu; + tqpair.mgmt_pdu->qpair = &tqpair; tcp_req.req.cmd = (union nvmf_h2c_msg *)&tcp_req.cmd; tcp_req.req.rsp = &rsp0; tcp_req.state = TCP_REQUEST_STATE_NEW; @@ -1068,12 +1072,10 @@ test_nvmf_tcp_invalid_sgl(void) /* Process a command and ensure that it fails and the request is set up to return an error */ nvmf_tcp_req_process(&ttransport, &tcp_req); - CU_ASSERT(STAILQ_EMPTY(&group->pending_buf_queue)); - CU_ASSERT(tcp_req.state == TCP_REQUEST_STATE_TRANSFERRING_CONTROLLER_TO_HOST); - CU_ASSERT(tqpair.recv_state == NVME_TCP_PDU_RECV_STATE_AWAIT_PDU_READY); - CU_ASSERT(tcp_req.req.rsp->nvme_cpl.cid == cid); - CU_ASSERT(tcp_req.req.rsp->nvme_cpl.status.sct == SPDK_NVME_SCT_GENERIC); - CU_ASSERT(tcp_req.req.rsp->nvme_cpl.status.sc == SPDK_NVME_SC_DATA_SGL_LENGTH_INVALID); + CU_ASSERT(!STAILQ_EMPTY(&group->pending_buf_queue)); + CU_ASSERT(tcp_req.state == TCP_REQUEST_STATE_NEED_BUFFER); + CU_ASSERT(tqpair.recv_state == NVME_TCP_PDU_RECV_STATE_ERROR); + CU_ASSERT(tqpair.mgmt_pdu->hdr.term_req.common.pdu_type == SPDK_NVME_TCP_PDU_TYPE_C2H_TERM_REQ); } static void