| Message ID | 20220222124217.21715-1-ppavlu@suse.cz (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | target/iscsi: Fix detection of excess number of login exchanges | expand |
On 2/22/22 6:42 AM, Petr Pavlu wrote: > From: Petr Pavlu <petr.pavlu@suse.com> > > Function iscsi_target_do_login() attempts to cancel a connection when > a number of login exchanges reaches MAX_LOGIN_PDUS (7). This is done by > having a local counter and incrementing+checking it as the function > processes requests in a loop. A problem is that since the login rework in > back in 2013, the function always processes only a single request and the > loop is terminated at the end of the first iteration. This means the > counter reaches only value 1 and any excess number of login requests is > never rejected. > > Fix the problem by introducing iscsi_login.negotiation_exchanges counter > and update the logic to count exchanges per each login phase as described > in RFC 7143: >> 6.2. Text Mode Negotiation: >> [...] >> In the Login Phase (see Section 6.3), every stage is a separate >> negotiation. [...] >> [...] >> An iSCSI initiator or target MAY terminate a negotiation that does >> not terminate within an implementation-specific reasonable time or >> number of exchanges but SHOULD allow at least six (6) exchanges. > It wasn't clear to me what this fixes. Today, are initiators sending more than 6 exchanges and if so what happens to the target? Is it crashing or annoying to user or cause some sort of endless login so we run out of resources? Or is this more of code cleanup? When does this happen and with what initiators?
On 3/1/22 2:23 AM, Mike Christie wrote: > On 2/22/22 6:42 AM, Petr Pavlu wrote: >> From: Petr Pavlu <petr.pavlu@suse.com> >> >> Function iscsi_target_do_login() attempts to cancel a connection when >> a number of login exchanges reaches MAX_LOGIN_PDUS (7). This is done by >> having a local counter and incrementing+checking it as the function >> processes requests in a loop. A problem is that since the login rework in >> back in 2013, the function always processes only a single request and the >> loop is terminated at the end of the first iteration. This means the >> counter reaches only value 1 and any excess number of login requests is >> never rejected. >> >> Fix the problem by introducing iscsi_login.negotiation_exchanges counter >> and update the logic to count exchanges per each login phase as described >> in RFC 7143: >>> 6.2. Text Mode Negotiation: >>> [...] >>> In the Login Phase (see Section 6.3), every stage is a separate >>> negotiation. [...] >>> [...] >>> An iSCSI initiator or target MAY terminate a negotiation that does >>> not terminate within an implementation-specific reasonable time or >>> number of exchanges but SHOULD allow at least six (6) exchanges. >> > > It wasn't clear to me what this fixes. Today, are initiators sending more > than 6 exchanges and if so what happens to the target? Is it crashing or > annoying to user or cause some sort of endless login so we run out of > resources? Or is this more of code cleanup? > > When does this happen and with what initiators? This issue is only something that I noticed while reading through the target code because of some different problem. In that sense, the patch is more a code cleanup. My tests to verify the patch were also artificial (attached). I have now additionally tried some simple examples with sending extensive number of Login requests in a loop to the target and did not observe any immediate problem with running out of resources. A possible alternative might be therefore to remove this logic, not sure. Thanks, Petr
diff --git a/drivers/target/iscsi/iscsi_target_nego.c b/drivers/target/iscsi/iscsi_target_nego.c index c0ed6f8e5c5b..a5077ea09f6c 100644 --- a/drivers/target/iscsi/iscsi_target_nego.c +++ b/drivers/target/iscsi/iscsi_target_nego.c @@ -970,65 +970,65 @@ static int iscsi_target_handle_csg_one(struct iscsi_conn *conn, struct iscsi_log static int iscsi_target_do_login(struct iscsi_conn *conn, struct iscsi_login *login) { - int pdu_count = 0; struct iscsi_login_req *login_req; struct iscsi_login_rsp *login_rsp; login_req = (struct iscsi_login_req *) login->req; login_rsp = (struct iscsi_login_rsp *) login->rsp; - while (1) { - if (++pdu_count > MAX_LOGIN_PDUS) { - pr_err("MAX_LOGIN_PDUS count reached.\n"); - iscsit_tx_login_rsp(conn, ISCSI_STATUS_CLS_TARGET_ERR, - ISCSI_LOGIN_STATUS_TARGET_ERROR); + switch (ISCSI_LOGIN_CURRENT_STAGE(login_req->flags)) { + case 0: + login_rsp->flags &= ~ISCSI_FLAG_LOGIN_CURRENT_STAGE_MASK; + if (iscsi_target_handle_csg_zero(conn, login) < 0) return -1; - } - - switch (ISCSI_LOGIN_CURRENT_STAGE(login_req->flags)) { - case 0: - login_rsp->flags &= ~ISCSI_FLAG_LOGIN_CURRENT_STAGE_MASK; - if (iscsi_target_handle_csg_zero(conn, login) < 0) - return -1; - break; - case 1: - login_rsp->flags |= ISCSI_FLAG_LOGIN_CURRENT_STAGE1; - if (iscsi_target_handle_csg_one(conn, login) < 0) + break; + case 1: + login_rsp->flags |= ISCSI_FLAG_LOGIN_CURRENT_STAGE1; + if (iscsi_target_handle_csg_one(conn, login) < 0) + return -1; + if (login_rsp->flags & ISCSI_FLAG_LOGIN_TRANSIT) { + /* + * Check to make sure the TCP connection has not dropped + * asynchronously while session reinstatement was + * occurring in this kthread context, before + * transitioning to full feature phase operation. + */ + if (iscsi_target_sk_check_close(conn)) return -1; - if (login_rsp->flags & ISCSI_FLAG_LOGIN_TRANSIT) { - /* - * Check to make sure the TCP connection has not - * dropped asynchronously while session reinstatement - * was occuring in this kthread context, before - * transitioning to full feature phase operation. - */ - if (iscsi_target_sk_check_close(conn)) - return -1; - login->tsih = conn->sess->tsih; - login->login_complete = 1; - iscsi_target_restore_sock_callbacks(conn); - if (iscsi_target_do_tx_login_io(conn, - login) < 0) - return -1; - return 1; - } - break; - default: - pr_err("Illegal CSG: %d received from" - " Initiator, protocol error.\n", - ISCSI_LOGIN_CURRENT_STAGE(login_req->flags)); - break; + login->tsih = conn->sess->tsih; + login->login_complete = 1; + iscsi_target_restore_sock_callbacks(conn); + if (iscsi_target_do_tx_login_io(conn, login) < 0) + return -1; + return 1; } + break; + default: + pr_err("Illegal CSG: %d received from Initiator," + " protocol error.\n", + ISCSI_LOGIN_CURRENT_STAGE(login_req->flags)); + break; + } - if (iscsi_target_do_tx_login_io(conn, login) < 0) + if (login_rsp->flags & ISCSI_FLAG_LOGIN_TRANSIT) + login->negotiation_exchanges = 0; + else { + login->negotiation_exchanges++; + if (login->negotiation_exchanges >= MAX_LOGIN_PDUS) { + pr_err("MAX_LOGIN_PDUS count reached.\n"); + iscsit_tx_login_rsp(conn, ISCSI_STATUS_CLS_TARGET_ERR, + ISCSI_LOGIN_STATUS_TARGET_ERROR); return -1; - - if (login_rsp->flags & ISCSI_FLAG_LOGIN_TRANSIT) { - login_rsp->flags &= ~ISCSI_FLAG_LOGIN_TRANSIT; - login_rsp->flags &= ~ISCSI_FLAG_LOGIN_NEXT_STAGE_MASK; } - break; + } + + if (iscsi_target_do_tx_login_io(conn, login) < 0) + return -1; + + if (login_rsp->flags & ISCSI_FLAG_LOGIN_TRANSIT) { + login_rsp->flags &= ~ISCSI_FLAG_LOGIN_TRANSIT; + login_rsp->flags &= ~ISCSI_FLAG_LOGIN_NEXT_STAGE_MASK; } return 0; diff --git a/include/target/iscsi/iscsi_target_core.h b/include/target/iscsi/iscsi_target_core.h index 1eccb2ac7d02..b6a5e1cf3f77 100644 --- a/include/target/iscsi/iscsi_target_core.h +++ b/include/target/iscsi/iscsi_target_core.h @@ -705,6 +705,7 @@ struct iscsi_login { u32 rsp_length; u16 cid; u16 tsih; + u8 negotiation_exchanges; char req[ISCSI_HDR_LEN]; char rsp[ISCSI_HDR_LEN]; char *req_buf;