[PATCH 0/3] Fix possible hangs and race conditions during login

[PATCH 0/3] Fix possible hangs and race conditions during login

[Maurizio Lombardi]

PATCH 1 fixes a possible hang when during a login operation the initiator
stops sending data to the target and the login_work doesn't get scheduled
by the sk_data_ready callback.

PATCH 3 fixes a bug in iscsi_target_locate_portal(), in the error
code path the login semaphore may end up being released even if
no one called down() against it, this may allow
multiple threads to access the semaphore-protected code.

Maurizio Lombardi (3):
  target: iscsi: fix hang in the iSCSI login code
  target: iscsi: remove unused transport_timer
  target: iscsi: prevent login threads from racing between each other

 drivers/target/iscsi/iscsi_target_login.c |  1 +
 drivers/target/iscsi/iscsi_target_nego.c  | 60 +++++++++++------------
 drivers/target/iscsi/iscsi_target_util.c  | 26 ++++++++++
 drivers/target/iscsi/iscsi_target_util.h  |  3 ++
 include/target/iscsi/iscsi_target_core.h  |  2 +-
 5 files changed, 59 insertions(+), 33 deletions(-)

-- 
2.31.1

[PATCH 1/3] target: iscsi: fix hang in the iSCSI login code

[Maurizio Lombardi]

If the initiator suddenly stops sending data during a login while
keeping the TCP connection open, the sk_data_ready callback won't
schedule the login_work and the latter
will never timeout and release the login semaphore.

All the other login operations will therefore get stuck waiting
for the semaphore to be released.

Add a timer to check if the initiator became unresponsive, this is how it
works:

  * The timer is started in iscsi_target_start_negotiation(), before
    clearing tre LOGIN_FLAGS_INITIAL_PDU flag.
  * If iscsi_target_do_login() returned a non-zero value, this means that
    the login operation either failed or didn't require additional PDUs;
    in this case the timer is immediately stopped.
  * If iscsi_target_do_login() returned zero then the control of
    the login operation is passed to login_work that will take over the
    responsibility of releasing the login semaphore and stopping the timer.
  * If login_work gets stuck, the login timer will expire and
    will force the login to fail (by sending a SIGINT to the login kthread
    and by closing the socket).

Signed-off-by: Maurizio Lombardi <mlombard@redhat.com>
---
 drivers/target/iscsi/iscsi_target_login.c |  1 +
 drivers/target/iscsi/iscsi_target_nego.c  | 56 ++++++++++-------------
 drivers/target/iscsi/iscsi_target_util.c  | 26 +++++++++++
 drivers/target/iscsi/iscsi_target_util.h  |  3 ++
 include/target/iscsi/iscsi_target_core.h  |  1 +
 5 files changed, 56 insertions(+), 31 deletions(-)

diff --git a/drivers/target/iscsi/iscsi_target_login.c b/drivers/target/iscsi/iscsi_target_login.c
index 27e448c2d066..bb7d5a596266 100644
--- a/drivers/target/iscsi/iscsi_target_login.c
+++ b/drivers/target/iscsi/iscsi_target_login.c
@@ -1127,6 +1127,7 @@ static struct iscsit_conn *iscsit_alloc_conn(struct iscsi_np *np)
 	timer_setup(&conn->nopin_response_timer,
 		    iscsit_handle_nopin_response_timeout, 0);
 	timer_setup(&conn->nopin_timer, iscsit_handle_nopin_timeout, 0);
+	timer_setup(&conn->login_timer, iscsit_login_timeout, 0);
 
 	if (iscsit_conn_set_transport(conn, np->np_transport) < 0)
 		goto free_conn;
diff --git a/drivers/target/iscsi/iscsi_target_nego.c b/drivers/target/iscsi/iscsi_target_nego.c
index 24040c118e49..f901a7231c48 100644
--- a/drivers/target/iscsi/iscsi_target_nego.c
+++ b/drivers/target/iscsi/iscsi_target_nego.c
@@ -472,12 +472,18 @@ static int iscsi_target_do_login(struct iscsit_conn *, struct iscsi_login *);
 
 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_check_close: TCP_CLOSE_WAIT|TCP_CLOSE,"
+	switch (sk->sk_state) {
+	case TCP_FIN_WAIT1:
+	case TCP_FIN_WAIT2:
+	case TCP_CLOSE_WAIT:
+	case TCP_LAST_ACK:
+	case TCP_CLOSE:
+		pr_debug("__iscsi_target_sk_check_close: socket closing,"
 			"returning TRUE\n");
 		return true;
+	default:
+		return false;
 	}
-	return false;
 }
 
 static bool iscsi_target_sk_check_close(struct iscsit_conn *conn)
@@ -535,25 +541,6 @@ static void iscsi_target_login_drop(struct iscsit_conn *conn, struct iscsi_login
 	iscsi_target_login_sess_out(conn, zero_tsih, true);
 }
 
-struct conn_timeout {
-	struct timer_list timer;
-	struct iscsit_conn *conn;
-};
-
-static void iscsi_target_login_timeout(struct timer_list *t)
-{
-	struct conn_timeout *timeout = from_timer(timeout, t, timer);
-	struct iscsit_conn *conn = timeout->conn;
-
-	pr_debug("Entering iscsi_target_login_timeout >>>>>>>>>>>>>>>>>>>\n");
-
-	if (conn->login_kworker) {
-		pr_debug("Sending SIGINT to conn->login_kworker %s/%d\n",
-			 conn->login_kworker->comm, conn->login_kworker->pid);
-		send_sig(SIGINT, conn->login_kworker, 1);
-	}
-}
-
 static void iscsi_target_do_login_rx(struct work_struct *work)
 {
 	struct iscsit_conn *conn = container_of(work,
@@ -562,7 +549,6 @@ static void iscsi_target_do_login_rx(struct work_struct *work)
 	struct iscsi_np *np = login->np;
 	struct iscsi_portal_group *tpg = conn->tpg;
 	struct iscsi_tpg_np *tpg_np = conn->tpg_np;
-	struct conn_timeout timeout;
 	int rc, zero_tsih = login->zero_tsih;
 	bool state;
 
@@ -597,17 +583,13 @@ static void iscsi_target_do_login_rx(struct work_struct *work)
 		goto err;
 	}
 
-	conn->login_kworker = current;
 	allow_signal(SIGINT);
-
-	timeout.conn = conn;
-	timer_setup_on_stack(&timeout.timer, iscsi_target_login_timeout, 0);
-	mod_timer(&timeout.timer, jiffies + TA_LOGIN_TIMEOUT * HZ);
-	pr_debug("Starting login timer for %s/%d\n", current->comm, current->pid);
+	iscsit_start_login_timer(conn);
+	conn->login_kworker = current;
 
 	rc = conn->conn_transport->iscsit_get_login_rx(conn, login);
-	del_timer_sync(&timeout.timer);
-	destroy_timer_on_stack(&timeout.timer);
+
+	iscsit_stop_login_timer(conn);
 	flush_signals(current);
 	conn->login_kworker = NULL;
 
@@ -646,6 +628,13 @@ static void iscsi_target_do_login_rx(struct work_struct *work)
 		if (iscsi_target_sk_check_and_clear(conn,
 						    LOGIN_FLAGS_WRITE_ACTIVE))
 			goto err;
+
+		/*
+		 * Restart the login timer to prevent the
+		 * login process from getting stuck if the initiator
+		 * stops sending data.
+		 */
+		iscsit_start_login_timer(conn);
 	} else if (rc == 1) {
 		cancel_delayed_work(&conn->login_work);
 		iscsi_target_nego_release(conn);
@@ -657,6 +646,7 @@ static void iscsi_target_do_login_rx(struct work_struct *work)
 err:
 	iscsi_target_restore_sock_callbacks(conn);
 	cancel_delayed_work(&conn->login_work);
+	iscsit_stop_login_timer(conn);
 	iscsi_target_login_drop(conn, login);
 	iscsit_deaccess_np(np, tpg, tpg_np);
 }
@@ -1358,6 +1348,9 @@ int iscsi_target_start_negotiation(
 		set_bit(LOGIN_FLAGS_INITIAL_PDU, &conn->login_flags);
 		write_unlock_bh(&sk->sk_callback_lock);
 	}
+
+	iscsit_start_login_timer(conn);
+
 	/*
 	 * If iscsi_target_do_login returns zero to signal more PDU
 	 * exchanges are required to complete the login, go ahead and
@@ -1377,6 +1370,7 @@ int iscsi_target_start_negotiation(
 	}
 	if (ret != 0) {
 		cancel_delayed_work_sync(&conn->login_work);
+		iscsit_stop_login_timer(conn);
 		iscsi_target_nego_release(conn);
 	}
 
diff --git a/drivers/target/iscsi/iscsi_target_util.c b/drivers/target/iscsi/iscsi_target_util.c
index 26dc8ed3045b..414e883c5a0d 100644
--- a/drivers/target/iscsi/iscsi_target_util.c
+++ b/drivers/target/iscsi/iscsi_target_util.c
@@ -1040,6 +1040,32 @@ void iscsit_stop_nopin_timer(struct iscsit_conn *conn)
 	spin_unlock_bh(&conn->nopin_timer_lock);
 }
 
+void iscsit_login_timeout(struct timer_list *t)
+{
+	struct iscsit_conn *conn = from_timer(conn, t, login_timer);
+
+	pr_debug("Entering iscsi_target_login_timeout >>>>>>>>>>>>>>>>>>>\n");
+
+	if (conn->login_kworker) {
+		pr_debug("Sending SIGINT to conn->login_kworker %s/%d\n",
+			 conn->login_kworker->comm, conn->login_kworker->pid);
+		send_sig(SIGINT, conn->login_kworker, 1);
+	}
+	kernel_sock_shutdown(conn->sock, SHUT_RDWR);
+}
+
+void iscsit_start_login_timer(struct iscsit_conn *conn)
+{
+	pr_debug("Login timer started\n");
+	mod_timer(&conn->login_timer, jiffies + TA_LOGIN_TIMEOUT * HZ);
+}
+
+void iscsit_stop_login_timer(struct iscsit_conn *conn)
+{
+	pr_debug("Login timer stopped\n");
+	timer_delete_sync(&conn->login_timer);
+}
+
 int iscsit_send_tx_data(
 	struct iscsit_cmd *cmd,
 	struct iscsit_conn *conn,
diff --git a/drivers/target/iscsi/iscsi_target_util.h b/drivers/target/iscsi/iscsi_target_util.h
index 33ea799a0850..b3ffb7b204a8 100644
--- a/drivers/target/iscsi/iscsi_target_util.h
+++ b/drivers/target/iscsi/iscsi_target_util.h
@@ -56,6 +56,9 @@ extern void iscsit_handle_nopin_timeout(struct timer_list *t);
 extern void __iscsit_start_nopin_timer(struct iscsit_conn *);
 extern void iscsit_start_nopin_timer(struct iscsit_conn *);
 extern void iscsit_stop_nopin_timer(struct iscsit_conn *);
+extern void iscsit_login_timeout(struct timer_list *t);
+extern void iscsit_start_login_timer(struct iscsit_conn *);
+extern void iscsit_stop_login_timer(struct iscsit_conn *);
 extern int iscsit_send_tx_data(struct iscsit_cmd *, struct iscsit_conn *, int);
 extern int iscsit_fe_sendpage_sg(struct iscsit_cmd *, struct iscsit_conn *);
 extern int iscsit_tx_login_rsp(struct iscsit_conn *, u8, u8);
diff --git a/include/target/iscsi/iscsi_target_core.h b/include/target/iscsi/iscsi_target_core.h
index 94d06ddfd80a..c52022a4e93a 100644
--- a/include/target/iscsi/iscsi_target_core.h
+++ b/include/target/iscsi/iscsi_target_core.h
@@ -568,6 +568,7 @@ struct iscsit_conn {
 	struct timer_list	nopin_timer;
 	struct timer_list	nopin_response_timer;
 	struct timer_list	transport_timer;
+	struct timer_list	login_timer;
 	struct task_struct	*login_kworker;
 	/* Spinlock used for add/deleting cmd's from conn_cmd_list */
 	spinlock_t		cmd_lock;
-- 
2.31.1

Re: [PATCH 1/3] target: iscsi: fix hang in the iSCSI login code

[Mike Christie]

On 3/10/23 4:04 AM, Maurizio Lombardi wrote:
> If the initiator suddenly stops sending data during a login while
> keeping the TCP connection open, the sk_data_ready callback won't
> schedule the login_work and the latter
> will never timeout and release the login semaphore.
> 
> All the other login operations will therefore get stuck waiting
> for the semaphore to be released.
> 

You mean np_login_sem right? Do you know why we have to serialize access
to the np during login? Is it just a simple way to handle the internal
target variables for things like MC/s, reinstatement, etc? I saw the tsih
case, but am wondering how easy it is to remove.

If we need the sem why do we use the sk_callback/non-blocking type of
approach when we can only do 1 login at a time per tpg? We end up
creating 2 threads per connection after FFP, so it seems like we would
have enough resources for a temp login thread before FFP. Looking over
the commit history, we seem to hit a good number of bugs in this code.

It seems like we could simplify the login code by do a blocking type of
implementation where:

1. __iscsi_target_login_thread starts a workstruct that runs
iscsi_target_start_negotiation.  It would then run iscsi_target_do_login_rx
which just waits for a response. When we get one, it does iscsi_target_do_login
and if we need more PDUs loops. We have one timer for all this.

2. We can remove the np_login_sem. It would be replaced by a workstruct
in the np. __iscsi_target_login_thread would just flush the work to make sure
we are not running a login on the same np already.

3. We can remove all the sk_callback* related code for iscsit tcp since
recvmsg just return failure when the state changes.

4. It looks like cxgbit will work with some small changes because I think it
just kicks off iscsi_target_do_login_rx after sending a login PDU, then it
just waits in its iscsit_get_login_rx.

5. It looks like isert will also work, because it's isert_get_login_rx can
just wait on the login_req_comp/login_comp already.

Re: [PATCH 1/3] target: iscsi: fix hang in the iSCSI login code

[Maurizio Lombardi]

pá 10. 3. 2023 v 20:53 odesílatel Mike Christie
<michael.christie@oracle.com> napsal:
>
> On 3/10/23 4:04 AM, Maurizio Lombardi wrote:
> > If the initiator suddenly stops sending data during a login while
> > keeping the TCP connection open, the sk_data_ready callback won't
> > schedule the login_work and the latter
> > will never timeout and release the login semaphore.
> >
> > All the other login operations will therefore get stuck waiting
> > for the semaphore to be released.
> >
>
> You mean np_login_sem right? Do you know why we have to serialize access
> to the np during login? Is it just a simple way to handle the internal
> target variables for things like MC/s, reinstatement, etc? I saw the tsih
> case, but am wondering how easy it is to remove.

Yes, I mean the np_login_sem, right now I don't know for sure how easy
is to remove it, but I agree that probably there is room for improvement here
and the login code can be simplified a lot.

>
> If we need the sem why do we use the sk_callback/non-blocking type of
> approach when we can only do 1 login at a time per tpg?

Yeah, I guess that the author just wanted the login thread to go back to
kernel_accept() as fast as possible to be able to serve the next connection.
But yes, if the new connection is against the same tpg, the login
thread will have
to wait for the semaphore anyway.


>
> It seems like we could simplify the login code by do a blocking type of
> implementation where:
>
> 1. __iscsi_target_login_thread starts a workstruct that runs
> iscsi_target_start_negotiation.  It would then run iscsi_target_do_login_rx
> which just waits for a response. When we get one, it does iscsi_target_do_login
> and if we need more PDUs loops. We have one timer for all this.
>
> 2. We can remove the np_login_sem. It would be replaced by a workstruct
> in the np. __iscsi_target_login_thread would just flush the work to make sure
> we are not running a login on the same np already.
>
> 3. We can remove all the sk_callback* related code for iscsit tcp since
> recvmsg just return failure when the state changes.
>
> 4. It looks like cxgbit will work with some small changes because I think it
> just kicks off iscsi_target_do_login_rx after sending a login PDU, then it
> just waits in its iscsit_get_login_rx.
>
> 5. It looks like isert will also work, because it's isert_get_login_rx can
> just wait on the login_req_comp/login_comp already.
>

Ok I think I could work on this new login code.
However, I would suggest to consider taking this patchset as a
stopgap, so the Delphix guys
can keep their customers happy and I can work on this
without feeling the pressure; also, this patchset is easier and safer
to backport to
stable kernels.

Maurizio

Re: [PATCH 1/3] target: iscsi: fix hang in the iSCSI login code

[Mike Christie]

On 3/10/23 4:04 AM, Maurizio Lombardi wrote:
> If the initiator suddenly stops sending data during a login while
> keeping the TCP connection open, the sk_data_ready callback won't
> schedule the login_work and the latter
> will never timeout and release the login semaphore.
> 
> All the other login operations will therefore get stuck waiting
> for the semaphore to be released.
> 
> Add a timer to check if the initiator became unresponsive, this is how it
> works:
> 
>   * The timer is started in iscsi_target_start_negotiation(), before
>     clearing tre LOGIN_FLAGS_INITIAL_PDU flag.
>   * If iscsi_target_do_login() returned a non-zero value, this means that
>     the login operation either failed or didn't require additional PDUs;
>     in this case the timer is immediately stopped.
>   * If iscsi_target_do_login() returned zero then the control of
>     the login operation is passed to login_work that will take over the
>     responsibility of releasing the login semaphore and stopping the timer.
>   * If login_work gets stuck, the login timer will expire and
>     will force the login to fail (by sending a SIGINT to the login kthread
>     and by closing the socket).
> 
> Signed-off-by: Maurizio Lombardi <mlombard@redhat.com>
> ---
>  drivers/target/iscsi/iscsi_target_login.c |  1 +
>  drivers/target/iscsi/iscsi_target_nego.c  | 56 ++++++++++-------------
>  drivers/target/iscsi/iscsi_target_util.c  | 26 +++++++++++
>  drivers/target/iscsi/iscsi_target_util.h  |  3 ++
>  include/target/iscsi/iscsi_target_core.h  |  1 +
>  5 files changed, 56 insertions(+), 31 deletions(-)
> 
> diff --git a/drivers/target/iscsi/iscsi_target_login.c b/drivers/target/iscsi/iscsi_target_login.c
> index 27e448c2d066..bb7d5a596266 100644
> --- a/drivers/target/iscsi/iscsi_target_login.c
> +++ b/drivers/target/iscsi/iscsi_target_login.c
> @@ -1127,6 +1127,7 @@ static struct iscsit_conn *iscsit_alloc_conn(struct iscsi_np *np)
>  	timer_setup(&conn->nopin_response_timer,
>  		    iscsit_handle_nopin_response_timeout, 0);
>  	timer_setup(&conn->nopin_timer, iscsit_handle_nopin_timeout, 0);
> +	timer_setup(&conn->login_timer, iscsit_login_timeout, 0);
>  
>  	if (iscsit_conn_set_transport(conn, np->np_transport) < 0)
>  		goto free_conn;
> diff --git a/drivers/target/iscsi/iscsi_target_nego.c b/drivers/target/iscsi/iscsi_target_nego.c
> index 24040c118e49..f901a7231c48 100644
> --- a/drivers/target/iscsi/iscsi_target_nego.c
> +++ b/drivers/target/iscsi/iscsi_target_nego.c
> @@ -472,12 +472,18 @@ static int iscsi_target_do_login(struct iscsit_conn *, struct iscsi_login *);
>  
>  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_check_close: TCP_CLOSE_WAIT|TCP_CLOSE,"
> +	switch (sk->sk_state) {
> +	case TCP_FIN_WAIT1:
> +	case TCP_FIN_WAIT2:
> +	case TCP_CLOSE_WAIT:
> +	case TCP_LAST_ACK:
> +	case TCP_CLOSE:
> +		pr_debug("__iscsi_target_sk_check_close: socket closing,"
>  			"returning TRUE\n");

Don't need to break up a string. We do it a lot in the lio code, but we've
been trying not to in new code.

>  		return true;
> +	default:
> +		return false;
>  	}
> -	return false;
>  }
>  
>  static bool iscsi_target_sk_check_close(struct iscsit_conn *conn)
> @@ -535,25 +541,6 @@ static void iscsi_target_login_drop(struct iscsit_conn *conn, struct iscsi_login
>  	iscsi_target_login_sess_out(conn, zero_tsih, true);
>  }
>  
> -struct conn_timeout {
> -	struct timer_list timer;
> -	struct iscsit_conn *conn;
> -};
> -
> -static void iscsi_target_login_timeout(struct timer_list *t)
> -{
> -	struct conn_timeout *timeout = from_timer(timeout, t, timer);
> -	struct iscsit_conn *conn = timeout->conn;
> -
> -	pr_debug("Entering iscsi_target_login_timeout >>>>>>>>>>>>>>>>>>>\n");
> -
> -	if (conn->login_kworker) {
> -		pr_debug("Sending SIGINT to conn->login_kworker %s/%d\n",
> -			 conn->login_kworker->comm, conn->login_kworker->pid);
> -		send_sig(SIGINT, conn->login_kworker, 1);
> -	}
> -}
> -
>  static void iscsi_target_do_login_rx(struct work_struct *work)
>  {
>  	struct iscsit_conn *conn = container_of(work,
> @@ -562,7 +549,6 @@ static void iscsi_target_do_login_rx(struct work_struct *work)
>  	struct iscsi_np *np = login->np;
>  	struct iscsi_portal_group *tpg = conn->tpg;
>  	struct iscsi_tpg_np *tpg_np = conn->tpg_np;
> -	struct conn_timeout timeout;
>  	int rc, zero_tsih = login->zero_tsih;
>  	bool state;
>  
> @@ -597,17 +583,13 @@ static void iscsi_target_do_login_rx(struct work_struct *work)
>  		goto err;
>  	}
>  
> -	conn->login_kworker = current;
>  	allow_signal(SIGINT);
> -
> -	timeout.conn = conn;
> -	timer_setup_on_stack(&timeout.timer, iscsi_target_login_timeout, 0);
> -	mod_timer(&timeout.timer, jiffies + TA_LOGIN_TIMEOUT * HZ);
> -	pr_debug("Starting login timer for %s/%d\n", current->comm, current->pid);
> +	iscsit_start_login_timer(conn);
> +	conn->login_kworker = current;>  
>  	rc = conn->conn_transport->iscsit_get_login_rx(conn, login);
> -	del_timer_sync(&timeout.timer);
> -	destroy_timer_on_stack(&timeout.timer);
> +
> +	iscsit_stop_login_timer(conn);
>  	flush_signals(current);
>  	conn->login_kworker = NULL;
>  
> @@ -646,6 +628,13 @@ static void iscsi_target_do_login_rx(struct work_struct *work)
>  		if (iscsi_target_sk_check_and_clear(conn,
>  						    LOGIN_FLAGS_WRITE_ACTIVE))
>  			goto err;
> +
> +		/*
> +		 * Restart the login timer to prevent the
> +		 * login process from getting stuck if the initiator

I would fix up the formatting so the first line is longer.

> +		 * stops sending data.
> +		 */
> +		iscsit_start_login_timer(conn);
>  	} else if (rc == 1) {
>  		cancel_delayed_work(&conn->login_work);
>  		iscsi_target_nego_release(conn);
> @@ -657,6 +646,7 @@ static void iscsi_target_do_login_rx(struct work_struct *work)
>  err:
>  	iscsi_target_restore_sock_callbacks(conn);
>  	cancel_delayed_work(&conn->login_work);
> +	iscsit_stop_login_timer(conn);>  	iscsi_target_login_drop(conn, login);
>  	iscsit_deaccess_np(np, tpg, tpg_np);
>  }
> @@ -1358,6 +1348,9 @@ int iscsi_target_start_negotiation(
>  		set_bit(LOGIN_FLAGS_INITIAL_PDU, &conn->login_flags);
>  		write_unlock_bh(&sk->sk_callback_lock);
>  	}
> +
> +	iscsit_start_login_timer(conn);

At this time, we have the np->np_login_timer running right?

Don't we only need to start this new timer when we know there are
more PDUs and the connection is good (iscsi_target_do_login returns
0 and iscsi_target_sk_check_and_clear returns 0)?

I think you can just kill np timer and only use the login_timer for
both cases. So I mean set the thread to kill as the login one and start
this login_timer in __iscsi_target_login_thread where we used to call
iscsi_start_login_thread_timer. You would then mod the timer when we
transition from iscsi_target_start_negotiation to waiting for the next
PDU.

> +
>  	/*
>  	 * If iscsi_target_do_login returns zero to signal more PDU
>  	 * exchanges are required to complete the login, go ahead and
> @@ -1377,6 +1370,7 @@ int iscsi_target_start_negotiation(
>  	}
>  	if (ret != 0) {
>  		cancel_delayed_work_sync(&conn->login_work);
> +		iscsit_stop_login_timer(conn);
>  		iscsi_target_nego_release(conn);
>  	}
>  
> diff --git a/drivers/target/iscsi/iscsi_target_util.c b/drivers/target/iscsi/iscsi_target_util.c
> index 26dc8ed3045b..414e883c5a0d 100644
> --- a/drivers/target/iscsi/iscsi_target_util.c
> +++ b/drivers/target/iscsi/iscsi_target_util.c
> @@ -1040,6 +1040,32 @@ void iscsit_stop_nopin_timer(struct iscsit_conn *conn)
>  	spin_unlock_bh(&conn->nopin_timer_lock);
>  }
>  
> +void iscsit_login_timeout(struct timer_list *t)
> +{
> +	struct iscsit_conn *conn = from_timer(conn, t, login_timer);
> +
> +	pr_debug("Entering iscsi_target_login_timeout >>>>>>>>>>>>>>>>>>>\n");
> +
> +	if (conn->login_kworker) {
> +		pr_debug("Sending SIGINT to conn->login_kworker %s/%d\n",
> +			 conn->login_kworker->comm, conn->login_kworker->pid);
> +		send_sig(SIGINT, conn->login_kworker, 1);
> +	}
> +	kernel_sock_shutdown(conn->sock, SHUT_RDWR);

For isert and cxgbit we won't have conn->sock set so I think you need some
sort of callout for those drivers, or maybe set LOGIN_FLAGS_CLOSED and queue
the login_work. Maybe the latter will work for all drivers as well. You probably
need some extra locking and LOGIN_FLAGS checks to handle an issue similar to
below.

If we do need to do the kernel_sock_shutdown we might need to add some code
to not call it twice (I'm not sure it's fully supported). It looks like we
can race and do:

1. login_work has just been queued.
2. But the login timer fires. We run kernel_sock_shutdown and
iscsi_target_sk_state_change starts to run but has not yet taken
the sk_callback_lock because..
3. iscsit_get_login_rx already started and passed the
iscsi_target_sk_check_close check. It re-arms the timer and calls
iscsit_get_login_rx.
4. iscsi_target_sk_state_change takes the lock and sets LOGIN_FLAGS_CLOSED.
5. We timeout again from #3 since we called kernel_sock_shutdown before.

Re: [PATCH 1/3] target: iscsi: fix hang in the iSCSI login code

[Maurizio Lombardi]

út 14. 3. 2023 v 0:52 odesílatel Mike Christie
<michael.christie@oracle.com> napsal:
>
> > +     case TCP_CLOSE:
> > +             pr_debug("__iscsi_target_sk_check_close: socket closing,"
> >                       "returning TRUE\n");
>
> Don't need to break up a string. We do it a lot in the lio code, but we've
> been trying not to in new code.
>
> > +             /*
> > +              * Restart the login timer to prevent the
> > +              * login process from getting stuck if the initiator
>
> I would fix up the formatting so the first line is longer.

Ok

> > @@ -1358,6 +1348,9 @@ int iscsi_target_start_negotiation(
> >               set_bit(LOGIN_FLAGS_INITIAL_PDU, &conn->login_flags);
> >               write_unlock_bh(&sk->sk_callback_lock);
> >       }
> > +
> > +     iscsit_start_login_timer(conn);
>
> At this time, we have the np->np_login_timer running right?

Yes.

>
> Don't we only need to start this new timer when we know there are
> more PDUs and the connection is good (iscsi_target_do_login returns
> 0 and iscsi_target_sk_check_and_clear returns 0)?

The moment iscsi_target_sk_check_and_clear() clears the
LOGIN_FLAGS_INITIAL_PDU flag
and returns 0, the login worker may be already running.
If we start the timer after the call to
iscsi_target_sk_check_and_clear(), we could have a race condition:

1) login_work runs and reschedules itself non-stop because
LOGIN_FLAGS_INITIAL_PDU is set
2) login kthread calls  iscsi_target_sk_check_and_clear() and clears
LOGIN_FLAGS_INITIAL_PDU
3) login work runs and completes the login
4) login kthread starts the timer
5) No one stops the timer, it fires and kills the connection despite
the fact the login was successful.

I could however replace this code:

ret = iscsi_target_do_login(conn, login);
 if (!ret && iscsi_target_sk_check_and_clear(conn, LOGIN_FLAGS_INITIAL_PDU))
           ret = -1;

with the following, if you like it more:

ret = iscsi_target_do_login(conn, login);
if (!ret) {
      iscsit_start_login_timer(conn);
      if (iscsi_target_sk_check_and_clear(conn, LOGIN_FLAGS_INITIAL_PDU)) {
           iscsit_stop_login_timer(conn);
           ret = -1;
      }
}

>
> I think you can just kill np timer and only use the login_timer for
> both cases. So I mean set the thread to kill as the login one and start
> this login_timer in __iscsi_target_login_thread where we used to call
> iscsi_start_login_thread_timer. You would then mod the timer when we
> transition from iscsi_target_start_negotiation to waiting for the next
> PDU.
>

Yes, maybe, but I would need to find a way to detect if conn->login_kworker
is pointing to the login thread or to the login_work's thread, because
the np_login_timer is supposed to clear the ISCSI_TF_RUNNING flag.

maybe something like this:

if (conn->login_kworker == conn->tpg_np->tpg_np->np_thread) {
     spin_lock_bh(&np->np_thread_lock);
     if (!(np->np_login_timer_flags & ISCSI_TF_STOP))
           np->np_login_timer_flags &= ~ISCSI_TF_RUNNING;
     spin_unlock_bh(&np->np_thread_lock);
}

> For isert and cxgbit we won't have conn->sock set so I think you need some
> sort of callout for those drivers, or maybe set LOGIN_FLAGS_CLOSED and queue
> the login_work. Maybe the latter will work for all drivers as well. You probably
> need some extra locking and LOGIN_FLAGS checks to handle an issue similar to
> below.

Hmm, that would need to be tested, because LOGIN_FLAGS_CLOSED is supposed
to be set when the socket is already in the process of getting closed
(it's state is TCP_CLOSE_WAIT or TCP_FIN_WAIT* or whatever)
So If I set LOGIN_FLAGS_CLOSED in the timer and the socket is
TCP_ESTABLISHED it means that I am trying to
do the opposite, will the socket be properly closed
by isert/cxgbit in this case?

>
> If we do need to do the kernel_sock_shutdown we might need to add some code
> to not call it twice (I'm not sure it's fully supported). It looks like we
> can race and do:

Correct, thanks. In that case a check like this should be sufficient.

if (!test_bit( LOGIN_FLAGS_CLOSED))
     kernel_sock_shutdown(sock, SHUT_RDWR);


Maurizio

Re: [PATCH 1/3] target: iscsi: fix hang in the iSCSI login code

[Mike Christie]

On 3/14/23 6:09 AM, Maurizio Lombardi wrote:
> út 14. 3. 2023 v 0:52 odesílatel Mike Christie
> <michael.christie@oracle.com> napsal:
>>
>>> +     case TCP_CLOSE:
>>> +             pr_debug("__iscsi_target_sk_check_close: socket closing,"
>>>                       "returning TRUE\n");
>>
>> Don't need to break up a string. We do it a lot in the lio code, but we've
>> been trying not to in new code.
>>
>>> +             /*
>>> +              * Restart the login timer to prevent the
>>> +              * login process from getting stuck if the initiator
>>
>> I would fix up the formatting so the first line is longer.
> 
> Ok
> 
>>> @@ -1358,6 +1348,9 @@ int iscsi_target_start_negotiation(
>>>               set_bit(LOGIN_FLAGS_INITIAL_PDU, &conn->login_flags);
>>>               write_unlock_bh(&sk->sk_callback_lock);
>>>       }
>>> +
>>> +     iscsit_start_login_timer(conn);
>>
>> At this time, we have the np->np_login_timer running right?
> 
> Yes.
> 
>>
>> Don't we only need to start this new timer when we know there are
>> more PDUs and the connection is good (iscsi_target_do_login returns
>> 0 and iscsi_target_sk_check_and_clear returns 0)?
> 
> The moment iscsi_target_sk_check_and_clear() clears the
> LOGIN_FLAGS_INITIAL_PDU flag
> and returns 0, the login worker may be already running.
> If we start the timer after the call to
> iscsi_target_sk_check_and_clear(), we could have a race condition:
> 
> 1) login_work runs and reschedules itself non-stop because
> LOGIN_FLAGS_INITIAL_PDU is set
> 2) login kthread calls  iscsi_target_sk_check_and_clear() and clears
> LOGIN_FLAGS_INITIAL_PDU
> 3) login work runs and completes the login
> 4) login kthread starts the timer
> 5) No one stops the timer, it fires and kills the connection despite
> the fact the login was successful.
> 
> I could however replace this code:
>
> ret = iscsi_target_do_login(conn, login);
>  if (!ret && iscsi_target_sk_check_and_clear(conn, LOGIN_FLAGS_INITIAL_PDU))
>            ret = -1;
> 
> with the following, if you like it more:
> 
> ret = iscsi_target_do_login(conn, login);
> if (!ret) {
>       iscsit_start_login_timer(conn);
>       if (iscsi_target_sk_check_and_clear(conn, LOGIN_FLAGS_INITIAL_PDU)) {
>            iscsit_stop_login_timer(conn);
>            ret = -1;
>       }
> }

Ah yeah, I wasn't thinking specifically about this race when I wrote the
above comment. With the combined timer below, I was thinking this is handled
when you set/check the login_kworker thread.

> 
>>
>> I think you can just kill np timer and only use the login_timer for
>> both cases. So I mean set the thread to kill as the login one and start
>> this login_timer in __iscsi_target_login_thread where we used to call
>> iscsi_start_login_thread_timer. You would then mod the timer when we
>> transition from iscsi_target_start_negotiation to waiting for the next
>> PDU.
>>
> 
> Yes, maybe, but I would need to find a way to detect if conn->login_kworker
> is pointing to the login thread or to the login_work's thread, because
> the np_login_timer is supposed to clear the ISCSI_TF_RUNNING flag.
> 
> maybe something like this:
> 
> if (conn->login_kworker == conn->tpg_np->tpg_np->np_thread) {
>      spin_lock_bh(&np->np_thread_lock);
>      if (!(np->np_login_timer_flags & ISCSI_TF_STOP))
>            np->np_login_timer_flags &= ~ISCSI_TF_RUNNING;
>      spin_unlock_bh(&np->np_thread_lock);
> }

We don't need any of the np_login_timer_flags code if we are using your per
conn login_timer do we?

For the new timer:
- We are adding one per conn timer.
- We use that for both the initial pdu and later ones.
- The timeout function, sends a signal if there is a thread set or does whatever
we figure out below for the case where there is no thread (we don't do any
np_login_timer_flags stuff).
- We probably don't need to do both the signal and whatever we decide below.
Or, we need to check some of the LOGIN_FLAGS since for example we don't
need to queue the login_work and set LOGIN_FLAGS_CLOSED if LOGIN_FLAGS_INITIAL_PDU
is set.
- The iscsi_start_login_timer function handles setting the login_kworker thread.

So we do:
1. Replace iscsi_start_login_thread_timer/iscsi_stop_login_thread_timer with
iscsit_start_login_timer/iscsit_stop_login_timer

__iscsi_target_login_thread only calls iscsit_stop_login_timer on failure.
On success it will either timeout waiting for the next PDU or
iscsi_target_do_login_rx will called and reset the timer.

2. You also have a iscsit_mod_login_timer depending on how you want to handle
the race you described above.
3. iscsi_target_start_negotiation only mods the timer if iscsi_target_do_login/
iscsi_target_sk_check_and_clear is successful and if iscsi_target_do_login_rx
has not already run.

For the latter, in iscsi_start/mod_login_timer you could add a check like your
np_thread above where if the login_work has already reset login_kworker then
we just return. Or we can add a new LOGIN_FLAGS flag, or add a bool arg to
iscsi_start/mod_login_timer where if set to true and if the login_kworker thread
does not equal current, then you know iscsi_target_do_login_rx already took over
the timer and do nothing.

4. You call iscsit_start_login_timer/iscsit_stop_login_timer like you are in
iscsi_target_do_login_rx.

> 
>> For isert and cxgbit we won't have conn->sock set so I think you need some
>> sort of callout for those drivers, or maybe set LOGIN_FLAGS_CLOSED and queue
>> the login_work. Maybe the latter will work for all drivers as well. You probably
>> need some extra locking and LOGIN_FLAGS checks to handle an issue similar to
>> below.
> 
> Hmm, that would need to be tested, because LOGIN_FLAGS_CLOSED is supposed> to be set when the socket is already in the process of getting closed
> (it's state is TCP_CLOSE_WAIT or TCP_FIN_WAIT* or whatever)
> So If I set LOGIN_FLAGS_CLOSED in the timer and the socket is
> TCP_ESTABLISHED it means that I am trying to
> do the opposite, will the socket be properly closed
> by isert/cxgbit in this case?

I'm open to suggestions. I don't really care how we fix it, but it should
work for all the drivers.

If we go the login_work route, ignore LOGIN_FLAGS_CLOSED for a second. I'm
just talking about the code path we use for error handling in this type of
case. We can easily just add a new bit.

- For iscsit tcp, if we are reading in data in iscsit_get_login_rx, and the
timer fires and we get a signal, iscsit_get_login_rx will return a failure
and we do the goto err path. So in this case we, the connection is not closed
and we go through iscsi_target_login_drop, so we know it works.

isert/cxgbit doesn't use sockets. We don't hit any of the LOGIN_FLAGS or
sk_state checks for them. The tcp connection might or might not be closed
for both drivers when the timer fires.

- For cxgbit, we can be waiting in iscsit_get_login_rx. It seems to kick this off
after we send a login PDU. The connection might be open or closed, then we timeout
and get a signal and break from our wait in iscsit_get_login_rx. We then do the
goto err path like iscsit tcp.

- For isert, after the first PDU we queue login_work when we get a login PDU.
So, I don't think we do anything right now if after the first PDU nothing is
sent like in your bug. iscsi_target_login_sess_out works for the initial PDU
timing out though, so we know we can call it when the conn is open.

Note: The isert_disconnected_handler is sort of like tcp's sk_state_change callout,
but isert just calls to iscsit_cause_connection_reinstatement which doesn't do
anything (it doesn't set LOGIN_FLAGS and iscsi_target_nego.c doesn't have any
checks for what isert is doing).

My suggestion to set LOGIN_FLAGS_CLOSED and then to queue the login work would
work like the iscsit tcp case above. When the work runs, it sees the bit and we
go down the goto err path and run iscsi_target_login_drop.

- If your concern is that we would abuse LOGIN_FLAGS_CLOSED, then we can add a
new bit.
- I didn't write out everything. Like the ordering of cancel_delayed_work and
iscsit_stop_login_timer would need to be reversed. We probably can add some
more LOGIN_FLAGS checks like in iscsi_target_sk_state_change so we don't always
queue the login_work like is done when the ACTIVE bits are set (or we could
kill that and just always queue the login_work).

If your concern was that we have no idea about all the code paths then yeah
it needs testing.

Re: [PATCH 1/3] target: iscsi: fix hang in the iSCSI login code

[Maurizio Lombardi]

út 14. 3. 2023 v 22:23 odesílatel Mike Christie
<michael.christie@oracle.com> napsal:
>
> On 3/14/23 6:09 AM, Maurizio Lombardi wrote:
> > út 14. 3. 2023 v 0:52 odesílatel Mike Christie
> > <michael.christie@oracle.com> napsal:
> >>
> >>> +     case TCP_CLOSE:
> >>> +             pr_debug("__iscsi_target_sk_check_close: socket closing,"
> >>>                       "returning TRUE\n");
> >>
> >> Don't need to break up a string. We do it a lot in the lio code, but we've
> >> been trying not to in new code.
> >>
> >>> +             /*
> >>> +              * Restart the login timer to prevent the
> >>> +              * login process from getting stuck if the initiator
> >>
> >> I would fix up the formatting so the first line is longer.
> >
> > Ok
> >
> >>> @@ -1358,6 +1348,9 @@ int iscsi_target_start_negotiation(
> >>>               set_bit(LOGIN_FLAGS_INITIAL_PDU, &conn->login_flags);
> >>>               write_unlock_bh(&sk->sk_callback_lock);
> >>>       }
> >>> +
> >>> +     iscsit_start_login_timer(conn);
> >>
> >> At this time, we have the np->np_login_timer running right?
> >
> > Yes.
> >
> >>
> >> Don't we only need to start this new timer when we know there are
> >> more PDUs and the connection is good (iscsi_target_do_login returns
> >> 0 and iscsi_target_sk_check_and_clear returns 0)?
> >
> > The moment iscsi_target_sk_check_and_clear() clears the
> > LOGIN_FLAGS_INITIAL_PDU flag
> > and returns 0, the login worker may be already running.
> > If we start the timer after the call to
> > iscsi_target_sk_check_and_clear(), we could have a race condition:
> >
> > 1) login_work runs and reschedules itself non-stop because
> > LOGIN_FLAGS_INITIAL_PDU is set
> > 2) login kthread calls  iscsi_target_sk_check_and_clear() and clears
> > LOGIN_FLAGS_INITIAL_PDU
> > 3) login work runs and completes the login
> > 4) login kthread starts the timer
> > 5) No one stops the timer, it fires and kills the connection despite
> > the fact the login was successful.
> >
> > I could however replace this code:
> >
> > ret = iscsi_target_do_login(conn, login);
> >  if (!ret && iscsi_target_sk_check_and_clear(conn, LOGIN_FLAGS_INITIAL_PDU))
> >            ret = -1;
> >
> > with the following, if you like it more:
> >
> > ret = iscsi_target_do_login(conn, login);
> > if (!ret) {
> >       iscsit_start_login_timer(conn);
> >       if (iscsi_target_sk_check_and_clear(conn, LOGIN_FLAGS_INITIAL_PDU)) {
> >            iscsit_stop_login_timer(conn);
> >            ret = -1;
> >       }
> > }
>
> Ah yeah, I wasn't thinking specifically about this race when I wrote the
> above comment. With the combined timer below, I was thinking this is handled
> when you set/check the login_kworker thread.
>
> >
> >>
> >> I think you can just kill np timer and only use the login_timer for
> >> both cases. So I mean set the thread to kill as the login one and start
> >> this login_timer in __iscsi_target_login_thread where we used to call
> >> iscsi_start_login_thread_timer. You would then mod the timer when we
> >> transition from iscsi_target_start_negotiation to waiting for the next
> >> PDU.
> >>
> >
> > Yes, maybe, but I would need to find a way to detect if conn->login_kworker
> > is pointing to the login thread or to the login_work's thread, because
> > the np_login_timer is supposed to clear the ISCSI_TF_RUNNING flag.
> >
> > maybe something like this:
> >
> > if (conn->login_kworker == conn->tpg_np->tpg_np->np_thread) {
> >      spin_lock_bh(&np->np_thread_lock);
> >      if (!(np->np_login_timer_flags & ISCSI_TF_STOP))
> >            np->np_login_timer_flags &= ~ISCSI_TF_RUNNING;
> >      spin_unlock_bh(&np->np_thread_lock);
> > }
>
> We don't need any of the np_login_timer_flags code if we are using your per
> conn login_timer do we?

Ah yes you're right, I was just confused by all those
"ISCSI_TF_RUNNING/STOP" flags used all
over the place, then I realized that every timer has its own flags.

>
> For the new timer:
> - We are adding one per conn timer.
> - We use that for both the initial pdu and later ones.
> - The timeout function, sends a signal if there is a thread set or does whatever
> we figure out below for the case where there is no thread (we don't do any
> np_login_timer_flags stuff).
> - We probably don't need to do both the signal and whatever we decide below.
> Or, we need to check some of the LOGIN_FLAGS since for example we don't
> need to queue the login_work and set LOGIN_FLAGS_CLOSED if LOGIN_FLAGS_INITIAL_PDU
> is set.
> - The iscsi_start_login_timer function handles setting the login_kworker thread.

Ok I have a V2 almost ready, I'm testing it right now.

Maurizio

[PATCH 2/3] target: iscsi: remove unused transport_timer

[Maurizio Lombardi]

Signed-off-by: Maurizio Lombardi <mlombard@redhat.com>
---
 include/target/iscsi/iscsi_target_core.h | 1 -
 1 file changed, 1 deletion(-)

diff --git a/include/target/iscsi/iscsi_target_core.h b/include/target/iscsi/iscsi_target_core.h
index c52022a4e93a..491a6abf0efa 100644
--- a/include/target/iscsi/iscsi_target_core.h
+++ b/include/target/iscsi/iscsi_target_core.h
@@ -567,7 +567,6 @@ struct iscsit_conn {
 	struct iscsi_login	*login;
 	struct timer_list	nopin_timer;
 	struct timer_list	nopin_response_timer;
-	struct timer_list	transport_timer;
 	struct timer_list	login_timer;
 	struct task_struct	*login_kworker;
 	/* Spinlock used for add/deleting cmd's from conn_cmd_list */
-- 
2.31.1

[PATCH 3/3] target: iscsi: prevent login threads from racing between each other

[Maurizio Lombardi]

The tpg->np_login_sem is a semaphore that is used to serialize the login
process when multiple login threads run concurrently against the same
target portal group.

The iscsi_target_locate_portal() function finds the tpg,
calls iscsit_access_np() against the np_login_sem semaphore
and saves the tpg pointer in conn->tpg;

If iscsi_target_locate_portal() fails, the caller will check for the
conn->tpg pointer and, if it's not NULL, then it will assume
that iscsi_target_locate_portal() called iscsit_access_np() on the
semaphore.

Make sure that conn->tpg gets initialized only if iscsit_access_np()
was successful, otherwise iscsit_deaccess_np() may end up
being called against a semaphore we never took, allowing more than one
thread to access the same tpg.

Signed-off-by: Maurizio Lombardi <mlombard@redhat.com>
---
 drivers/target/iscsi/iscsi_target_nego.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/target/iscsi/iscsi_target_nego.c b/drivers/target/iscsi/iscsi_target_nego.c
index f901a7231c48..8461cb27b3b3 100644
--- a/drivers/target/iscsi/iscsi_target_nego.c
+++ b/drivers/target/iscsi/iscsi_target_nego.c
@@ -1120,6 +1120,7 @@ int iscsi_target_locate_portal(
 	iscsi_target_set_sock_callbacks(conn);
 
 	login->np = np;
+	conn->tpg = NULL;
 
 	login_req = (struct iscsi_login_req *) login->req;
 	payload_length = ntoh24(login_req->dlength);
@@ -1187,7 +1188,6 @@ int iscsi_target_locate_portal(
 	 */
 	sessiontype = strncmp(s_buf, DISCOVERY, 9);
 	if (!sessiontype) {
-		conn->tpg = iscsit_global->discovery_tpg;
 		if (!login->leading_connection)
 			goto get_target;
 
@@ -1204,9 +1204,11 @@ int iscsi_target_locate_portal(
 		 * Serialize access across the discovery struct iscsi_portal_group to
 		 * process login attempt.
 		 */
+		conn->tpg = iscsit_global->discovery_tpg;
 		if (iscsit_access_np(np, conn->tpg) < 0) {
 			iscsit_tx_login_rsp(conn, ISCSI_STATUS_CLS_TARGET_ERR,
 				ISCSI_LOGIN_STATUS_SVC_UNAVAILABLE);
+			conn->tpg = NULL;
 			ret = -1;
 			goto out;
 		}
-- 
2.31.1