Hello Jiang Yi,
Apologies for the delayed response on your bug-report + patch.
Comments inline below.
On Tue, 2017-05-16 at 17:57 +0800, Jiang Yi wrote:
> Hi Nic,
>
> There are three timing problems in the kthread usages of
> iscsi_target_mod:
>
> np_thread of struct iscsi_np
> rx_thread and tx_thread of struct iscsi_conn
>
> In iscsit_close_connection(), it called
>
> send_sig(SIGINT, conn->tx_thread, 1);
> kthread_stop(conn->tx_thread);
>
> In conn->tx_thread, which is iscsi_target_tx_thread(), when it receive
> SIGINT the kthread will exit without checking the return value of
> kthread_should_stop().
>
> So if iscsi_target_tx_thread() exit right between send_sig(SIGINT...)
> and kthread_stop(...), the kthread_stop() will try to stop an already
> stopped kthread.
>
> This is invalid according to the documentation of kthread_stop().
>
> I think the kthread should wait until kthread_should_stop() returns
> true. I propose a patch:
>
Nice catch on this long standing issue.
Although I've never seen this bug triggered in the wild, your comments
are correct that any of the iscsi-target kthreads exiting due to
send_sig(SIGINT, ...) being received causing a kthread to exit before
kthread_stop() is called, is incorrect.
> Signed-off-by: Jiang Yi <jiangyilism@xxxxxxxxx>
> ---
> drivers/target/iscsi/iscsi_target.c | 19 +++++++++++++++++--
> drivers/target/iscsi/iscsi_target_erl0.c | 6 +++++-
> drivers/target/iscsi/iscsi_target_erl0.h | 2 +-
> drivers/target/iscsi/iscsi_target_login.c | 4 ++++
> 4 files changed, 27 insertions(+), 4 deletions(-)
>
So your patch looks good, but after some testing and review there where
two additional cases that needed to be addressed.
The first is in iscsi_target_tx_thread during explicit iscsi logout,
where iscsi_handle_response_queue() returns -ECONNRESET to signal
iscsit_logout_post_handler() has completed.
The two cases in iscsit_logout_post_handler_closesession() and
iscsit_logout_post_handler_samecid() are responsible for clearing
conn->tx_thread_active to false, and need to be treated the same as the
other conn_freed = true cases.
Here's the incremental patch:
@ -3815,12 +3817,14 @@ int iscsi_target_tx_thread(void *arg)
goto transport_err;
ret = iscsit_handle_response_queue(conn);
- if (ret == 1)
+ if (ret == 1) {
goto get_immediate;
- else if (ret == -ECONNRESET)
+ } else if (ret == -ECONNRESET) {
+ conn_freed = true;
goto out;
- else if (ret < 0)
+ } else if (ret < 0) {
goto transport_err;
+ }
}
The second is in iscsi_target_rx_thread(), when an early failure occurs
triggered during login by iscsi_target_do_tx_login_io() socket send, or
a iscsit_start_kthreads() kthread_run() failure.
For both cases, send_sig(SIGINT, ...) and kthread_stop() are called to
shutdown the kthread, so it needs to wait for kthread_should_stop()
using conn_freed = false just as in the other cases.
Here's the incremental patch:
@@ -4016,7 +4026,7 @@ int iscsi_target_rx_thread(void *arg)
*/
rc = wait_for_completion_interruptible(&conn->rx_login_comp);
if (rc < 0 || iscsi_target_check_conn_state(conn))
- return 0;
+ goto out;
if (!conn->conn_transport->iscsit_get_rx_pdu)
return 0;
Both of these changes have been git squashed into your original patch
and applied to target-pending/master, with a slightly improved patch
title here:
iscsi-target: Always wait for kthread_should_stop() before kthread exit
https://git.kernel.org/pub/scm/linux/kernel/git/nab/target-pending.git/commit/?id=bf7e762866dc692eb9e696a506e99fbef55f4fa5
Thank you for your contribution.
--
To unsubscribe from this list: send the line "unsubscribe target-devel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
