| From 25cdda95fda78d22d44157da15aa7ea34be3c804 Mon Sep 17 00:00:00 2001 |
| From: Nicholas Bellinger <nab@linux-iscsi.org> |
| Date: Wed, 24 May 2017 21:47:09 -0700 |
| Subject: iscsi-target: Fix initial login PDU asynchronous socket close OOPs |
| |
| From: Nicholas Bellinger <nab@linux-iscsi.org> |
| |
| commit 25cdda95fda78d22d44157da15aa7ea34be3c804 upstream. |
| |
| This patch fixes a OOPs originally introduced by: |
| |
| commit bb048357dad6d604520c91586334c9c230366a14 |
| Author: Nicholas Bellinger <nab@linux-iscsi.org> |
| Date: Thu Sep 5 14:54:04 2013 -0700 |
| |
| iscsi-target: Add sk->sk_state_change to cleanup after TCP failure |
| |
| which would trigger a NULL pointer dereference when a TCP connection |
| was closed asynchronously via iscsi_target_sk_state_change(), but only |
| when the initial PDU processing in iscsi_target_do_login() from iscsi_np |
| process context was blocked waiting for backend I/O to complete. |
| |
| To address this issue, this patch makes the following changes. |
| |
| First, it introduces some common helper functions used for checking |
| socket closing state, checking login_flags, and atomically checking |
| socket closing state + setting login_flags. |
| |
| Second, it introduces a LOGIN_FLAGS_INITIAL_PDU bit to know when a TCP |
| connection has dropped via iscsi_target_sk_state_change(), but the |
| initial PDU processing within iscsi_target_do_login() in iscsi_np |
| context is still running. For this case, it sets LOGIN_FLAGS_CLOSED, |
| but doesn't invoke schedule_delayed_work(). |
| |
| The original NULL pointer dereference case reported by MNC is now handled |
| by iscsi_target_do_login() doing a iscsi_target_sk_check_close() before |
| transitioning to FFP to determine when the socket has already closed, |
| or iscsi_target_start_negotiation() if the login needs to exchange |
| more PDUs (eg: iscsi_target_do_login returned 0) but the socket has |
| closed. For both of these cases, the cleanup up of remaining connection |
| resources will occur in iscsi_target_start_negotiation() from iscsi_np |
| process context once the failure is detected. |
| |
| Finally, to handle to case where iscsi_target_sk_state_change() is |
| called after the initial PDU procesing is complete, it now invokes |
| conn->login_work -> iscsi_target_do_login_rx() to perform cleanup once |
| existing iscsi_target_sk_check_close() checks detect connection failure. |
| For this case, the cleanup of remaining connection resources will occur |
| in iscsi_target_do_login_rx() from delayed workqueue process context |
| once the failure is detected. |
| |
| Reported-by: Mike Christie <mchristi@redhat.com> |
| Reviewed-by: Mike Christie <mchristi@redhat.com> |
| Tested-by: Mike Christie <mchristi@redhat.com> |
| Cc: Mike Christie <mchristi@redhat.com> |
| Reported-by: Hannes Reinecke <hare@suse.com> |
| Cc: Hannes Reinecke <hare@suse.com> |
| Cc: Sagi Grimberg <sagi@grimberg.me> |
| Cc: Varun Prakash <varun@chelsio.com> |
| Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org> |
| Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> |
| |
| |
| --- |
| drivers/target/iscsi/iscsi_target_nego.c | 204 ++++++++++++++++++++----------- |
| include/target/iscsi/iscsi_target_core.h | 1 |
| 2 files changed, 138 insertions(+), 67 deletions(-) |
| |
| --- a/drivers/target/iscsi/iscsi_target_nego.c |
| +++ b/drivers/target/iscsi/iscsi_target_nego.c |
| @@ -489,14 +489,60 @@ static void iscsi_target_restore_sock_ca |
| |
| static int iscsi_target_do_login(struct iscsi_conn *, struct iscsi_login *); |
| |
| -static bool iscsi_target_sk_state_check(struct sock *sk) |
| +static bool __iscsi_target_sk_check_close(struct sock *sk) |
| { |
| if (sk->sk_state == TCP_CLOSE_WAIT || sk->sk_state == TCP_CLOSE) { |
| - pr_debug("iscsi_target_sk_state_check: TCP_CLOSE_WAIT|TCP_CLOSE," |
| + pr_debug("__iscsi_target_sk_check_close: TCP_CLOSE_WAIT|TCP_CLOSE," |
| "returning FALSE\n"); |
| - return false; |
| + return true; |
| } |
| - return true; |
| + return false; |
| +} |
| + |
| +static bool iscsi_target_sk_check_close(struct iscsi_conn *conn) |
| +{ |
| + bool state = false; |
| + |
| + if (conn->sock) { |
| + struct sock *sk = conn->sock->sk; |
| + |
| + read_lock_bh(&sk->sk_callback_lock); |
| + state = (__iscsi_target_sk_check_close(sk) || |
| + test_bit(LOGIN_FLAGS_CLOSED, &conn->login_flags)); |
| + read_unlock_bh(&sk->sk_callback_lock); |
| + } |
| + return state; |
| +} |
| + |
| +static bool iscsi_target_sk_check_flag(struct iscsi_conn *conn, unsigned int flag) |
| +{ |
| + bool state = false; |
| + |
| + if (conn->sock) { |
| + struct sock *sk = conn->sock->sk; |
| + |
| + read_lock_bh(&sk->sk_callback_lock); |
| + state = test_bit(flag, &conn->login_flags); |
| + read_unlock_bh(&sk->sk_callback_lock); |
| + } |
| + return state; |
| +} |
| + |
| +static bool iscsi_target_sk_check_and_clear(struct iscsi_conn *conn, unsigned int flag) |
| +{ |
| + bool state = false; |
| + |
| + if (conn->sock) { |
| + struct sock *sk = conn->sock->sk; |
| + |
| + write_lock_bh(&sk->sk_callback_lock); |
| + state = (__iscsi_target_sk_check_close(sk) || |
| + test_bit(LOGIN_FLAGS_CLOSED, &conn->login_flags)); |
| + if (!state) |
| + clear_bit(flag, &conn->login_flags); |
| + write_unlock_bh(&sk->sk_callback_lock); |
| + } |
| + return state; |
| } |
| |
| static void iscsi_target_login_drop(struct iscsi_conn *conn, struct iscsi_login *login) |
| @@ -536,6 +582,20 @@ static void iscsi_target_do_login_rx(str |
| |
| pr_debug("entering iscsi_target_do_login_rx, conn: %p, %s:%d\n", |
| conn, current->comm, current->pid); |
| + /* |
| + * If iscsi_target_do_login_rx() has been invoked by ->sk_data_ready() |
| + * before initial PDU processing in iscsi_target_start_negotiation() |
| + * has completed, go ahead and retry until it's cleared. |
| + * |
| + * Otherwise if the TCP connection drops while this is occuring, |
| + * iscsi_target_start_negotiation() will detect the failure, call |
| + * cancel_delayed_work_sync(&conn->login_work), and cleanup the |
| + * remaining iscsi connection resources from iscsi_np process context. |
| + */ |
| + if (iscsi_target_sk_check_flag(conn, LOGIN_FLAGS_INITIAL_PDU)) { |
| + schedule_delayed_work(&conn->login_work, msecs_to_jiffies(10)); |
| + return; |
| + } |
| |
| spin_lock(&tpg->tpg_state_lock); |
| state = (tpg->tpg_state == TPG_STATE_ACTIVE); |
| @@ -543,26 +603,12 @@ static void iscsi_target_do_login_rx(str |
| |
| if (!state) { |
| pr_debug("iscsi_target_do_login_rx: tpg_state != TPG_STATE_ACTIVE\n"); |
| - iscsi_target_restore_sock_callbacks(conn); |
| - iscsi_target_login_drop(conn, login); |
| - iscsit_deaccess_np(np, tpg, tpg_np); |
| - return; |
| + goto err; |
| } |
| |
| - if (conn->sock) { |
| - struct sock *sk = conn->sock->sk; |
| - |
| - read_lock_bh(&sk->sk_callback_lock); |
| - state = iscsi_target_sk_state_check(sk); |
| - read_unlock_bh(&sk->sk_callback_lock); |
| - |
| - if (!state) { |
| - pr_debug("iscsi_target_do_login_rx, TCP state CLOSE\n"); |
| - iscsi_target_restore_sock_callbacks(conn); |
| - iscsi_target_login_drop(conn, login); |
| - iscsit_deaccess_np(np, tpg, tpg_np); |
| - return; |
| - } |
| + if (iscsi_target_sk_check_close(conn)) { |
| + pr_debug("iscsi_target_do_login_rx, TCP state CLOSE\n"); |
| + goto err; |
| } |
| |
| conn->login_kworker = current; |
| @@ -580,34 +626,29 @@ static void iscsi_target_do_login_rx(str |
| flush_signals(current); |
| conn->login_kworker = NULL; |
| |
| - if (rc < 0) { |
| - iscsi_target_restore_sock_callbacks(conn); |
| - iscsi_target_login_drop(conn, login); |
| - iscsit_deaccess_np(np, tpg, tpg_np); |
| - return; |
| - } |
| + if (rc < 0) |
| + goto err; |
| |
| pr_debug("iscsi_target_do_login_rx after rx_login_io, %p, %s:%d\n", |
| conn, current->comm, current->pid); |
| |
| rc = iscsi_target_do_login(conn, login); |
| if (rc < 0) { |
| - iscsi_target_restore_sock_callbacks(conn); |
| - iscsi_target_login_drop(conn, login); |
| - iscsit_deaccess_np(np, tpg, tpg_np); |
| + goto err; |
| } else if (!rc) { |
| - if (conn->sock) { |
| - struct sock *sk = conn->sock->sk; |
| - |
| - write_lock_bh(&sk->sk_callback_lock); |
| - clear_bit(LOGIN_FLAGS_READ_ACTIVE, &conn->login_flags); |
| - write_unlock_bh(&sk->sk_callback_lock); |
| - } |
| + if (iscsi_target_sk_check_and_clear(conn, LOGIN_FLAGS_READ_ACTIVE)) |
| + goto err; |
| } else if (rc == 1) { |
| iscsi_target_nego_release(conn); |
| iscsi_post_login_handler(np, conn, zero_tsih); |
| iscsit_deaccess_np(np, tpg, tpg_np); |
| } |
| + return; |
| + |
| +err: |
| + iscsi_target_restore_sock_callbacks(conn); |
| + iscsi_target_login_drop(conn, login); |
| + iscsit_deaccess_np(np, tpg, tpg_np); |
| } |
| |
| static void iscsi_target_do_cleanup(struct work_struct *work) |
| @@ -655,31 +696,54 @@ static void iscsi_target_sk_state_change |
| orig_state_change(sk); |
| return; |
| } |
| + state = __iscsi_target_sk_check_close(sk); |
| + pr_debug("__iscsi_target_sk_close_change: state: %d\n", state); |
| + |
| if (test_bit(LOGIN_FLAGS_READ_ACTIVE, &conn->login_flags)) { |
| pr_debug("Got LOGIN_FLAGS_READ_ACTIVE=1 sk_state_change" |
| " conn: %p\n", conn); |
| + if (state) |
| + set_bit(LOGIN_FLAGS_CLOSED, &conn->login_flags); |
| write_unlock_bh(&sk->sk_callback_lock); |
| orig_state_change(sk); |
| return; |
| } |
| - if (test_and_set_bit(LOGIN_FLAGS_CLOSED, &conn->login_flags)) { |
| + if (test_bit(LOGIN_FLAGS_CLOSED, &conn->login_flags)) { |
| pr_debug("Got LOGIN_FLAGS_CLOSED=1 sk_state_change conn: %p\n", |
| conn); |
| write_unlock_bh(&sk->sk_callback_lock); |
| orig_state_change(sk); |
| return; |
| } |
| + /* |
| + * If the TCP connection has dropped, go ahead and set LOGIN_FLAGS_CLOSED, |
| + * but only queue conn->login_work -> iscsi_target_do_login_rx() |
| + * processing if LOGIN_FLAGS_INITIAL_PDU has already been cleared. |
| + * |
| + * When iscsi_target_do_login_rx() runs, iscsi_target_sk_check_close() |
| + * will detect the dropped TCP connection from delayed workqueue context. |
| + * |
| + * If LOGIN_FLAGS_INITIAL_PDU is still set, which means the initial |
| + * iscsi_target_start_negotiation() is running, iscsi_target_do_login() |
| + * via iscsi_target_sk_check_close() or iscsi_target_start_negotiation() |
| + * via iscsi_target_sk_check_and_clear() is responsible for detecting the |
| + * dropped TCP connection in iscsi_np process context, and cleaning up |
| + * the remaining iscsi connection resources. |
| + */ |
| + if (state) { |
| + pr_debug("iscsi_target_sk_state_change got failed state\n"); |
| + set_bit(LOGIN_FLAGS_CLOSED, &conn->login_flags); |
| + state = test_bit(LOGIN_FLAGS_INITIAL_PDU, &conn->login_flags); |
| + write_unlock_bh(&sk->sk_callback_lock); |
| |
| - state = iscsi_target_sk_state_check(sk); |
| - write_unlock_bh(&sk->sk_callback_lock); |
| - |
| - pr_debug("iscsi_target_sk_state_change: state: %d\n", state); |
| + orig_state_change(sk); |
| |
| - if (!state) { |
| - pr_debug("iscsi_target_sk_state_change got failed state\n"); |
| - schedule_delayed_work(&conn->login_cleanup_work, 0); |
| + if (!state) |
| + schedule_delayed_work(&conn->login_work, 0); |
| return; |
| } |
| + write_unlock_bh(&sk->sk_callback_lock); |
| + |
| orig_state_change(sk); |
| } |
| |
| @@ -944,6 +1008,15 @@ static int iscsi_target_do_login(struct |
| 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 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); |
| @@ -970,21 +1043,6 @@ static int iscsi_target_do_login(struct |
| break; |
| } |
| |
| - if (conn->sock) { |
| - struct sock *sk = conn->sock->sk; |
| - bool state; |
| - |
| - read_lock_bh(&sk->sk_callback_lock); |
| - state = iscsi_target_sk_state_check(sk); |
| - read_unlock_bh(&sk->sk_callback_lock); |
| - |
| - if (!state) { |
| - pr_debug("iscsi_target_do_login() failed state for" |
| - " conn: %p\n", conn); |
| - return -1; |
| - } |
| - } |
| - |
| return 0; |
| } |
| |
| @@ -1251,13 +1309,25 @@ int iscsi_target_start_negotiation( |
| if (conn->sock) { |
| struct sock *sk = conn->sock->sk; |
| |
| - write_lock_bh(&sk->sk_callback_lock); |
| - set_bit(LOGIN_FLAGS_READY, &conn->login_flags); |
| - write_unlock_bh(&sk->sk_callback_lock); |
| - } |
| + write_lock_bh(&sk->sk_callback_lock); |
| + set_bit(LOGIN_FLAGS_READY, &conn->login_flags); |
| + set_bit(LOGIN_FLAGS_INITIAL_PDU, &conn->login_flags); |
| + write_unlock_bh(&sk->sk_callback_lock); |
| + } |
| + /* |
| + * If iscsi_target_do_login returns zero to signal more PDU |
| + * exchanges are required to complete the login, go ahead and |
| + * clear LOGIN_FLAGS_INITIAL_PDU but only if the TCP connection |
| + * is still active. |
| + * |
| + * Otherwise if TCP connection dropped asynchronously, go ahead |
| + * and perform connection cleanup now. |
| + */ |
| + ret = iscsi_target_do_login(conn, login); |
| + if (!ret && iscsi_target_sk_check_and_clear(conn, LOGIN_FLAGS_INITIAL_PDU)) |
| + ret = -1; |
| |
| - ret = iscsi_target_do_login(conn, login); |
| - if (ret < 0) { |
| + if (ret < 0) { |
| cancel_delayed_work_sync(&conn->login_work); |
| cancel_delayed_work_sync(&conn->login_cleanup_work); |
| iscsi_target_restore_sock_callbacks(conn); |
| --- a/include/target/iscsi/iscsi_target_core.h |
| +++ b/include/target/iscsi/iscsi_target_core.h |
| @@ -562,6 +562,7 @@ struct iscsi_conn { |
| #define LOGIN_FLAGS_READ_ACTIVE 1 |
| #define LOGIN_FLAGS_CLOSED 2 |
| #define LOGIN_FLAGS_READY 4 |
| +#define LOGIN_FLAGS_INITIAL_PDU 8 |
| unsigned long login_flags; |
| struct delayed_work login_work; |
| struct delayed_work login_cleanup_work; |