From 2124e3b07fe7bc1648596b971d9e58180b6fcbad Mon Sep 17 00:00:00 2001 From: Alexander Motin Date: Wed, 17 Dec 2014 15:13:21 +0000 Subject: [PATCH] Make sequence numbers checks more strict. While we don't support MCS, hole in received sequence numbers may mean only PDU loss. While we don't support lost PDU recovery, terminate the connection to avoid stuck commands. While there, improve handling of sequence numbers wrap after 2^32 PDUs. MFC after: 2 weeks --- sys/cam/ctl/ctl_frontend_iscsi.c | 48 ++++++++++++++++++++++++------- sys/cam/ctl/ctl_frontend_iscsi.h | 1 + sys/dev/iscsi/iscsi.c | 49 ++++++++++++++++++-------------- sys/dev/iscsi/iscsi_proto.h | 3 ++ usr.sbin/ctld/discovery.c | 12 ++++---- usr.sbin/ctld/login.c | 6 ++-- usr.sbin/iscsid/discovery.c | 4 +-- usr.sbin/iscsid/login.c | 2 +- 8 files changed, 81 insertions(+), 44 deletions(-) diff --git a/sys/cam/ctl/ctl_frontend_iscsi.c b/sys/cam/ctl/ctl_frontend_iscsi.c index 8ec582586b77..0114b57b8247 100644 --- a/sys/cam/ctl/ctl_frontend_iscsi.c +++ b/sys/cam/ctl/ctl_frontend_iscsi.c @@ -233,19 +233,34 @@ cfiscsi_pdu_update_cmdsn(const struct icl_pdu *request) } #endif - /* - * The target MUST silently ignore any non-immediate command outside - * of this range. - */ - if (cmdsn < cs->cs_cmdsn || cmdsn > cs->cs_cmdsn + maxcmdsn_delta) { - CFISCSI_SESSION_UNLOCK(cs); - CFISCSI_SESSION_WARN(cs, "received PDU with CmdSN %d, " - "while expected CmdSN was %d", cmdsn, cs->cs_cmdsn); - return (true); - } + if ((request->ip_bhs->bhs_opcode & ISCSI_BHS_OPCODE_IMMEDIATE) == 0) { + /* + * The target MUST silently ignore any non-immediate command + * outside of this range. + */ + if (ISCSI_SNLT(cmdsn, cs->cs_cmdsn) || + ISCSI_SNGT(cmdsn, cs->cs_cmdsn + maxcmdsn_delta)) { + CFISCSI_SESSION_UNLOCK(cs); + CFISCSI_SESSION_WARN(cs, "received PDU with CmdSN %u, " + "while expected %u", cmdsn, cs->cs_cmdsn); + return (true); + } - if ((request->ip_bhs->bhs_opcode & ISCSI_BHS_OPCODE_IMMEDIATE) == 0) + /* + * We don't support multiple connections now, so any + * discontinuity in CmdSN means lost PDUs. Since we don't + * support PDU retransmission -- terminate the connection. + */ + if (cmdsn != cs->cs_cmdsn) { + CFISCSI_SESSION_UNLOCK(cs); + CFISCSI_SESSION_WARN(cs, "received PDU with CmdSN %u, " + "while expected %u; dropping connection", + cmdsn, cs->cs_cmdsn); + cfiscsi_session_terminate(cs); + return (true); + } cs->cs_cmdsn++; + } CFISCSI_SESSION_UNLOCK(cs); @@ -892,6 +907,16 @@ cfiscsi_pdu_handle_data_out(struct icl_pdu *request) return; } + if (cdw->cdw_datasn != ntohl(bhsdo->bhsdo_datasn)) { + CFISCSI_SESSION_WARN(cs, "received Data-Out PDU with " + "DataSN %u, while expected %u; dropping connection", + ntohl(bhsdo->bhsdo_datasn), cdw->cdw_datasn); + icl_pdu_free(request); + cfiscsi_session_terminate(cs); + return; + } + cdw->cdw_datasn++; + io = cdw->cdw_ctl_io; KASSERT((io->io_hdr.flags & CTL_FLAG_DATA_MASK) != CTL_FLAG_DATA_IN, ("CTL_FLAG_DATA_IN")); @@ -2650,6 +2675,7 @@ cfiscsi_datamove_out(union ctl_io *io) cdw->cdw_target_transfer_tag = target_transfer_tag; cdw->cdw_initiator_task_tag = bhssc->bhssc_initiator_task_tag; cdw->cdw_r2t_end = io->scsiio.kern_data_len; + cdw->cdw_datasn = 0; /* Set initial data pointer for the CDW respecting ext_data_filled. */ if (io->scsiio.kern_sg_entries > 0) { diff --git a/sys/cam/ctl/ctl_frontend_iscsi.h b/sys/cam/ctl/ctl_frontend_iscsi.h index 336b69da3234..5000f4c9c067 100644 --- a/sys/cam/ctl/ctl_frontend_iscsi.h +++ b/sys/cam/ctl/ctl_frontend_iscsi.h @@ -58,6 +58,7 @@ struct cfiscsi_data_wait { char *cdw_sg_addr; size_t cdw_sg_len; uint32_t cdw_r2t_end; + uint32_t cdw_datasn; }; #define CFISCSI_SESSION_STATE_INVALID 0 diff --git a/sys/dev/iscsi/iscsi.c b/sys/dev/iscsi/iscsi.c index af7184684d35..f9edc8246c31 100644 --- a/sys/dev/iscsi/iscsi.c +++ b/sys/dev/iscsi/iscsi.c @@ -192,7 +192,7 @@ iscsi_pdu_prepare(struct icl_pdu *request) * Data-Out PDU does not contain CmdSN. */ if (bhssc->bhssc_opcode != ISCSI_BHS_OPCODE_SCSI_DATA_OUT) { - if (is->is_cmdsn > is->is_maxcmdsn && + if (ISCSI_SNGT(is->is_cmdsn, is->is_maxcmdsn) && (bhssc->bhssc_opcode & ISCSI_BHS_OPCODE_IMMEDIATE) == 0) { /* * Current MaxCmdSN prevents us from sending any more @@ -201,8 +201,10 @@ iscsi_pdu_prepare(struct icl_pdu *request) * or by maintenance thread. */ #if 0 - ISCSI_SESSION_DEBUG(is, "postponing send, CmdSN %d, ExpCmdSN %d, MaxCmdSN %d, opcode 0x%x", - is->is_cmdsn, is->is_expcmdsn, is->is_maxcmdsn, bhssc->bhssc_opcode); + ISCSI_SESSION_DEBUG(is, "postponing send, CmdSN %u, " + "ExpCmdSN %u, MaxCmdSN %u, opcode 0x%x", + is->is_cmdsn, is->is_expcmdsn, is->is_maxcmdsn, + bhssc->bhssc_opcode); #endif return (true); } @@ -611,7 +613,7 @@ iscsi_pdu_update_statsn(const struct icl_pdu *response) { const struct iscsi_bhs_data_in *bhsdi; struct iscsi_session *is; - uint32_t expcmdsn, maxcmdsn; + uint32_t expcmdsn, maxcmdsn, statsn; is = PDU_SESSION(response); @@ -630,26 +632,27 @@ iscsi_pdu_update_statsn(const struct icl_pdu *response) */ if (bhsdi->bhsdi_opcode != ISCSI_BHS_OPCODE_SCSI_DATA_IN || (bhsdi->bhsdi_flags & BHSDI_FLAGS_S) != 0) { - if (ntohl(bhsdi->bhsdi_statsn) < is->is_statsn) { - ISCSI_SESSION_WARN(is, - "PDU StatSN %d >= session StatSN %d, opcode 0x%x", - is->is_statsn, ntohl(bhsdi->bhsdi_statsn), - bhsdi->bhsdi_opcode); + statsn = ntohl(bhsdi->bhsdi_statsn); + if (statsn != is->is_statsn && statsn != (is->is_statsn + 1)) { + /* XXX: This is normal situation for MCS */ + ISCSI_SESSION_WARN(is, "PDU 0x%x StatSN %u != " + "session ExpStatSN %u (or + 1); reconnecting", + bhsdi->bhsdi_opcode, statsn, is->is_statsn); + iscsi_session_reconnect(is); } - is->is_statsn = ntohl(bhsdi->bhsdi_statsn); + if (ISCSI_SNGT(statsn, is->is_statsn)) + is->is_statsn = statsn; } expcmdsn = ntohl(bhsdi->bhsdi_expcmdsn); maxcmdsn = ntohl(bhsdi->bhsdi_maxcmdsn); - /* - * XXX: Compare using Serial Arithmetic Sense. - */ - if (maxcmdsn + 1 < expcmdsn) { - ISCSI_SESSION_DEBUG(is, "PDU MaxCmdSN %d + 1 < PDU ExpCmdSN %d; ignoring", + if (ISCSI_SNLT(maxcmdsn + 1, expcmdsn)) { + ISCSI_SESSION_DEBUG(is, + "PDU MaxCmdSN %u + 1 < PDU ExpCmdSN %u; ignoring", maxcmdsn, expcmdsn); } else { - if (maxcmdsn > is->is_maxcmdsn) { + if (ISCSI_SNGT(maxcmdsn, is->is_maxcmdsn)) { is->is_maxcmdsn = maxcmdsn; /* @@ -658,15 +661,19 @@ iscsi_pdu_update_statsn(const struct icl_pdu *response) */ if (!STAILQ_EMPTY(&is->is_postponed)) cv_signal(&is->is_maintenance_cv); - } else if (maxcmdsn < is->is_maxcmdsn) { - ISCSI_SESSION_DEBUG(is, "PDU MaxCmdSN %d < session MaxCmdSN %d; ignoring", + } else if (ISCSI_SNLT(maxcmdsn, is->is_maxcmdsn)) { + /* XXX: This is normal situation for MCS */ + ISCSI_SESSION_DEBUG(is, + "PDU MaxCmdSN %u < session MaxCmdSN %u; ignoring", maxcmdsn, is->is_maxcmdsn); } - if (expcmdsn > is->is_expcmdsn) { + if (ISCSI_SNGT(expcmdsn, is->is_expcmdsn)) { is->is_expcmdsn = expcmdsn; - } else if (expcmdsn < is->is_expcmdsn) { - ISCSI_SESSION_DEBUG(is, "PDU ExpCmdSN %d < session ExpCmdSN %d; ignoring", + } else if (ISCSI_SNLT(expcmdsn, is->is_expcmdsn)) { + /* XXX: This is normal situation for MCS */ + ISCSI_SESSION_DEBUG(is, + "PDU ExpCmdSN %u < session ExpCmdSN %u; ignoring", expcmdsn, is->is_expcmdsn); } } diff --git a/sys/dev/iscsi/iscsi_proto.h b/sys/dev/iscsi/iscsi_proto.h index 97d73a7a074e..46572ce62d46 100644 --- a/sys/dev/iscsi/iscsi_proto.h +++ b/sys/dev/iscsi/iscsi_proto.h @@ -38,6 +38,9 @@ #define __CTASSERT(x, y) typedef char __assert_ ## y [(x) ? 1 : -1] #endif +#define ISCSI_SNGT(x, y) ((int32_t)(x) - (int32_t)(y) > 0) +#define ISCSI_SNLT(x, y) ((int32_t)(x) - (int32_t)(y) < 0) + #define ISCSI_BHS_SIZE 48 #define ISCSI_HEADER_DIGEST_SIZE 4 #define ISCSI_DATA_DIGEST_SIZE 4 diff --git a/usr.sbin/ctld/discovery.c b/usr.sbin/ctld/discovery.c index 01c9913051ba..b370e0be5867 100644 --- a/usr.sbin/ctld/discovery.c +++ b/usr.sbin/ctld/discovery.c @@ -65,13 +65,13 @@ text_receive(struct connection *conn) */ if ((bhstr->bhstr_flags & BHSTR_FLAGS_CONTINUE) != 0) log_errx(1, "received Text PDU with unsupported \"C\" flag"); - if (ntohl(bhstr->bhstr_cmdsn) < conn->conn_cmdsn) { + if (ISCSI_SNLT(ntohl(bhstr->bhstr_cmdsn), conn->conn_cmdsn)) { log_errx(1, "received Text PDU with decreasing CmdSN: " - "was %d, is %d", conn->conn_cmdsn, ntohl(bhstr->bhstr_cmdsn)); + "was %u, is %u", conn->conn_cmdsn, ntohl(bhstr->bhstr_cmdsn)); } if (ntohl(bhstr->bhstr_expstatsn) != conn->conn_statsn) { log_errx(1, "received Text PDU with wrong StatSN: " - "is %d, should be %d", ntohl(bhstr->bhstr_expstatsn), + "is %u, should be %u", ntohl(bhstr->bhstr_expstatsn), conn->conn_statsn); } conn->conn_cmdsn = ntohl(bhstr->bhstr_cmdsn); @@ -120,14 +120,14 @@ logout_receive(struct connection *conn) if ((bhslr->bhslr_reason & 0x7f) != BHSLR_REASON_CLOSE_SESSION) log_debugx("received Logout PDU with invalid reason 0x%x; " "continuing anyway", bhslr->bhslr_reason & 0x7f); - if (ntohl(bhslr->bhslr_cmdsn) < conn->conn_cmdsn) { + if (ISCSI_SNLT(ntohl(bhslr->bhslr_cmdsn), conn->conn_cmdsn)) { log_errx(1, "received Logout PDU with decreasing CmdSN: " - "was %d, is %d", conn->conn_cmdsn, + "was %u, is %u", conn->conn_cmdsn, ntohl(bhslr->bhslr_cmdsn)); } if (ntohl(bhslr->bhslr_expstatsn) != conn->conn_statsn) { log_errx(1, "received Logout PDU with wrong StatSN: " - "is %d, should be %d", ntohl(bhslr->bhslr_expstatsn), + "is %u, should be %u", ntohl(bhslr->bhslr_expstatsn), conn->conn_statsn); } conn->conn_cmdsn = ntohl(bhslr->bhslr_cmdsn); diff --git a/usr.sbin/ctld/login.c b/usr.sbin/ctld/login.c index fc41f5178d34..7add09b5d9bc 100644 --- a/usr.sbin/ctld/login.c +++ b/usr.sbin/ctld/login.c @@ -127,17 +127,17 @@ login_receive(struct connection *conn, bool initial) log_errx(1, "received Login PDU with unsupported " "Version-min 0x%x", bhslr->bhslr_version_min); } - if (ntohl(bhslr->bhslr_cmdsn) < conn->conn_cmdsn) { + if (ISCSI_SNLT(ntohl(bhslr->bhslr_cmdsn), conn->conn_cmdsn)) { login_send_error(request, 0x02, 0x05); log_errx(1, "received Login PDU with decreasing CmdSN: " - "was %d, is %d", conn->conn_cmdsn, + "was %u, is %u", conn->conn_cmdsn, ntohl(bhslr->bhslr_cmdsn)); } if (initial == false && ntohl(bhslr->bhslr_expstatsn) != conn->conn_statsn) { login_send_error(request, 0x02, 0x05); log_errx(1, "received Login PDU with wrong ExpStatSN: " - "is %d, should be %d", ntohl(bhslr->bhslr_expstatsn), + "is %u, should be %u", ntohl(bhslr->bhslr_expstatsn), conn->conn_statsn); } conn->conn_cmdsn = ntohl(bhslr->bhslr_cmdsn); diff --git a/usr.sbin/iscsid/discovery.c b/usr.sbin/iscsid/discovery.c index c87d9fff7356..a8975d73077a 100644 --- a/usr.sbin/iscsid/discovery.c +++ b/usr.sbin/iscsid/discovery.c @@ -66,7 +66,7 @@ text_receive(struct connection *conn) log_errx(1, "received Text PDU with unsupported \"C\" flag"); if (ntohl(bhstr->bhstr_statsn) != conn->conn_statsn + 1) { log_errx(1, "received Text PDU with wrong StatSN: " - "is %d, should be %d", ntohl(bhstr->bhstr_statsn), + "is %u, should be %u", ntohl(bhstr->bhstr_statsn), conn->conn_statsn + 1); } conn->conn_statsn = ntohl(bhstr->bhstr_statsn); @@ -112,7 +112,7 @@ logout_receive(struct connection *conn) ntohs(bhslr->bhslr_response)); if (ntohl(bhslr->bhslr_statsn) != conn->conn_statsn + 1) { log_errx(1, "received Logout PDU with wrong StatSN: " - "is %d, should be %d", ntohl(bhslr->bhslr_statsn), + "is %u, should be %u", ntohl(bhslr->bhslr_statsn), conn->conn_statsn + 1); } conn->conn_statsn = ntohl(bhslr->bhslr_statsn); diff --git a/usr.sbin/iscsid/login.c b/usr.sbin/iscsid/login.c index afe7ebbc6bfd..360b0ef186ab 100644 --- a/usr.sbin/iscsid/login.c +++ b/usr.sbin/iscsid/login.c @@ -257,7 +257,7 @@ login_receive(struct connection *conn) * to be bug in NetBSD iSCSI target. */ log_warnx("received Login PDU with wrong StatSN: " - "is %d, should be %d", ntohl(bhslr->bhslr_statsn), + "is %u, should be %u", ntohl(bhslr->bhslr_statsn), conn->conn_statsn + 1); } conn->conn_tsih = ntohs(bhslr->bhslr_tsih);