On Mon, Aug 07, 2017 at 03:16:22AM +0000, Nicholas A. Bellinger wrote:
> From: Nicholas Bellinger <nab@xxxxxxxxxxxxxxx>
>
> commit 25cdda95fda78d22d44157da15aa7ea34be3c804 upstream.
>
> This patch fixes a OOPs originally introduced by:
>
> commit bb048357dad6d604520c91586334c9c230366a14
> Author: Nicholas Bellinger <nab@xxxxxxxxxxxxxxx>
> 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@xxxxxxxxxx>
> Reviewed-by: Mike Christie <mchristi@xxxxxxxxxx>
> Tested-by: Mike Christie <mchristi@xxxxxxxxxx>
> Cc: Mike Christie <mchristi@xxxxxxxxxx>
> Reported-by: Hannes Reinecke <hare@xxxxxxxx>
> Cc: Hannes Reinecke <hare@xxxxxxxx>
> Cc: Sagi Grimberg <sagi@xxxxxxxxxxx>
> Cc: Varun Prakash <varun@xxxxxxxxxxx>
> Signed-off-by: Nicholas Bellinger <nab@xxxxxxxxxxxxxxx>
> ---
> drivers/target/iscsi/iscsi_target_nego.c | 204 +++++++++++++++++++++----------
> include/target/iscsi/iscsi_target_core.h | 1 +
> 2 files changed, 138 insertions(+), 67 deletions(-)
>
> diff --git a/drivers/target/iscsi/iscsi_target_nego.c b/drivers/target/iscsi/iscsi_target_nego.c
> index 6693d7c..e8efb42 100644
> --- a/drivers/target/iscsi/iscsi_target_nego.c
> +++ b/drivers/target/iscsi/iscsi_target_nego.c
> @@ -490,14 +490,60 @@ static void iscsi_target_restore_sock_callbacks(struct iscsi_conn *conn)
>
> 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)
> @@ -537,6 +583,20 @@ static void iscsi_target_do_login_rx(struct work_struct *work)
>
> 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);
> @@ -544,26 +604,12 @@ static void iscsi_target_do_login_rx(struct work_struct *work)
>
> 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;
> @@ -581,34 +627,29 @@ static void iscsi_target_do_login_rx(struct work_struct *work)
> 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)
> @@ -656,31 +697,54 @@ static void iscsi_target_sk_state_change(struct sock *sk)
> 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);
> }
>
> @@ -945,6 +1009,15 @@ static int iscsi_target_do_login(struct iscsi_conn *conn, struct iscsi_login *lo
> 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);
> @@ -971,21 +1044,6 @@ static int iscsi_target_do_login(struct iscsi_conn *conn, struct iscsi_login *lo
> 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;
> }
>
> @@ -1252,13 +1310,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);
> diff --git a/include/target/iscsi/iscsi_target_core.h b/include/target/iscsi/iscsi_target_core.h
> index 33b2e75..c8132b4 100644
> --- a/include/target/iscsi/iscsi_target_core.h
> +++ b/include/target/iscsi/iscsi_target_core.h
> @@ -563,6 +563,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;
Now applied, thanks.
greg k-h