Re: [PATCH] target: fix deadlock on unload
On Fri, 2014-06-27 at 08:35 -0400, Mikulas Patocka wrote: > > On Thu, 26 Jun 2014, Nicholas A. Bellinger wrote: > > > Hi Mikulas, > > > > On Mon, 2014-06-23 at 13:42 -0400, Mikulas Patocka wrote: > > > target: fix deadlock on unload > > > > > > On uniprocessor preemptible kernel, target core deadlocks on unload. The > > > following events happen: > > > * iscsit_del_np is called > > > * it calls send_sig(SIGINT, np->np_thread, 1); > > > * the scheduler switches to the np_thread > > > * the np_thread is woken up, it sees that kthread_should_stop() returns > > > false, so it doesn't terminate > > > * the np_thread clears signals with flush_signals(current); and goes back > > > to sleep in iscsit_accept_np > > > * the scheduler switches back to iscsit_del_np > > > * iscsit_del_np calls kthread_stop(np->np_thread); > > > * the np_thread is waiting in iscsit_accept_np and it doesn't respond to > > > kthread_stop > > > > > > The deadlock could be resolved if the administrator sends SIGINT signal to > > > the np_thread with killall -INT iscsi_np > > > > > > The reproducible deadlock was introduced in commit > > > db6077fd0b7dd41dc6ff18329cec979379071f87, but the thread-stopping code was > > > racy even before. > > > > > > This patch fixes the problem. Using kthread_should_stop to stop the > > > np_thread is unreliable, so we test np_thread_state instead. If > > > np_thread_state equals ISCSI_NP_THREAD_SHUTDOWN, the thread exits. > > > > > > Signed-off-by: Mikulas Patocka > > > Cc: sta...@vger.kernel.org > > > > > > > Apologies for the delayed response.. > > > > Applied to target-pending/master and including in the next -rc3 PULL > > request. > > > > Also FYI, I've added '3.12+' to the stable tag to match how far back > > commit db6077fd0 has been included in stable. > > > > Thanks, > > > > --nab > > Hi > > I think db6077fd0 should be backported to stable kernels beginning with > 3.10 (because they set np->np_thread = NULL in > __iscsi_target_login_thread). The current 3.10-stable branch misses this > patch. > > > This patch for unload deadlock should be backported to all stable kernels > (because unload is racy there), but because of different code, we should > make a different patch for old stable branches. > > For example in 3.4.95, __iscsi_target_login_thread contains this code: > spin_lock_bh(>np_thread_lock); > if (np->np_thread_state == ISCSI_NP_THREAD_RESET) { > np->np_thread_state = ISCSI_NP_THREAD_ACTIVE; > complete(>np_restart_comp); > } else { > np->np_thread_state = ISCSI_NP_THREAD_ACTIVE; > } > spin_unlock_bh(>np_thread_lock); > If the state is ISCSI_NP_THREAD_SHUTDOWN, the above piece of code will > change it to ISCSI_NP_THREAD_ACTIVE and open the same kthread_should_stop > race. > Ok, dropping the 'v3.12+' stable tag from this patch, and will include it in a PATCH-v3.10.y series together with db6077fd0 once Greg-KH attempts to queue it up. As for v3.4.x, care to send along a separate patch with your Tested-by + Signed-off-by..? Thanks for the extra stable info! --nab -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] target: fix deadlock on unload
On Fri, 2014-06-27 at 08:35 -0400, Mikulas Patocka wrote: On Thu, 26 Jun 2014, Nicholas A. Bellinger wrote: Hi Mikulas, On Mon, 2014-06-23 at 13:42 -0400, Mikulas Patocka wrote: target: fix deadlock on unload On uniprocessor preemptible kernel, target core deadlocks on unload. The following events happen: * iscsit_del_np is called * it calls send_sig(SIGINT, np-np_thread, 1); * the scheduler switches to the np_thread * the np_thread is woken up, it sees that kthread_should_stop() returns false, so it doesn't terminate * the np_thread clears signals with flush_signals(current); and goes back to sleep in iscsit_accept_np * the scheduler switches back to iscsit_del_np * iscsit_del_np calls kthread_stop(np-np_thread); * the np_thread is waiting in iscsit_accept_np and it doesn't respond to kthread_stop The deadlock could be resolved if the administrator sends SIGINT signal to the np_thread with killall -INT iscsi_np The reproducible deadlock was introduced in commit db6077fd0b7dd41dc6ff18329cec979379071f87, but the thread-stopping code was racy even before. This patch fixes the problem. Using kthread_should_stop to stop the np_thread is unreliable, so we test np_thread_state instead. If np_thread_state equals ISCSI_NP_THREAD_SHUTDOWN, the thread exits. Signed-off-by: Mikulas Patocka mpato...@redhat.com Cc: sta...@vger.kernel.org Apologies for the delayed response.. Applied to target-pending/master and including in the next -rc3 PULL request. Also FYI, I've added '3.12+' to the stable tag to match how far back commit db6077fd0 has been included in stable. Thanks, --nab Hi I think db6077fd0 should be backported to stable kernels beginning with 3.10 (because they set np-np_thread = NULL in __iscsi_target_login_thread). The current 3.10-stable branch misses this patch. This patch for unload deadlock should be backported to all stable kernels (because unload is racy there), but because of different code, we should make a different patch for old stable branches. For example in 3.4.95, __iscsi_target_login_thread contains this code: spin_lock_bh(np-np_thread_lock); if (np-np_thread_state == ISCSI_NP_THREAD_RESET) { np-np_thread_state = ISCSI_NP_THREAD_ACTIVE; complete(np-np_restart_comp); } else { np-np_thread_state = ISCSI_NP_THREAD_ACTIVE; } spin_unlock_bh(np-np_thread_lock); If the state is ISCSI_NP_THREAD_SHUTDOWN, the above piece of code will change it to ISCSI_NP_THREAD_ACTIVE and open the same kthread_should_stop race. Ok, dropping the 'v3.12+' stable tag from this patch, and will include it in a PATCH-v3.10.y series together with db6077fd0 once Greg-KH attempts to queue it up. As for v3.4.x, care to send along a separate patch with your Tested-by + Signed-off-by..? Thanks for the extra stable info! --nab -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] target: fix deadlock on unload
On Thu, 26 Jun 2014, Nicholas A. Bellinger wrote: > Hi Mikulas, > > On Mon, 2014-06-23 at 13:42 -0400, Mikulas Patocka wrote: > > target: fix deadlock on unload > > > > On uniprocessor preemptible kernel, target core deadlocks on unload. The > > following events happen: > > * iscsit_del_np is called > > * it calls send_sig(SIGINT, np->np_thread, 1); > > * the scheduler switches to the np_thread > > * the np_thread is woken up, it sees that kthread_should_stop() returns > > false, so it doesn't terminate > > * the np_thread clears signals with flush_signals(current); and goes back > > to sleep in iscsit_accept_np > > * the scheduler switches back to iscsit_del_np > > * iscsit_del_np calls kthread_stop(np->np_thread); > > * the np_thread is waiting in iscsit_accept_np and it doesn't respond to > > kthread_stop > > > > The deadlock could be resolved if the administrator sends SIGINT signal to > > the np_thread with killall -INT iscsi_np > > > > The reproducible deadlock was introduced in commit > > db6077fd0b7dd41dc6ff18329cec979379071f87, but the thread-stopping code was > > racy even before. > > > > This patch fixes the problem. Using kthread_should_stop to stop the > > np_thread is unreliable, so we test np_thread_state instead. If > > np_thread_state equals ISCSI_NP_THREAD_SHUTDOWN, the thread exits. > > > > Signed-off-by: Mikulas Patocka > > Cc: sta...@vger.kernel.org > > > > Apologies for the delayed response.. > > Applied to target-pending/master and including in the next -rc3 PULL > request. > > Also FYI, I've added '3.12+' to the stable tag to match how far back > commit db6077fd0 has been included in stable. > > Thanks, > > --nab Hi I think db6077fd0 should be backported to stable kernels beginning with 3.10 (because they set np->np_thread = NULL in __iscsi_target_login_thread). The current 3.10-stable branch misses this patch. This patch for unload deadlock should be backported to all stable kernels (because unload is racy there), but because of different code, we should make a different patch for old stable branches. For example in 3.4.95, __iscsi_target_login_thread contains this code: spin_lock_bh(>np_thread_lock); if (np->np_thread_state == ISCSI_NP_THREAD_RESET) { np->np_thread_state = ISCSI_NP_THREAD_ACTIVE; complete(>np_restart_comp); } else { np->np_thread_state = ISCSI_NP_THREAD_ACTIVE; } spin_unlock_bh(>np_thread_lock); If the state is ISCSI_NP_THREAD_SHUTDOWN, the above piece of code will change it to ISCSI_NP_THREAD_ACTIVE and open the same kthread_should_stop race. Mikulas -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] target: fix deadlock on unload
On Thu, 26 Jun 2014, Nicholas A. Bellinger wrote: Hi Mikulas, On Mon, 2014-06-23 at 13:42 -0400, Mikulas Patocka wrote: target: fix deadlock on unload On uniprocessor preemptible kernel, target core deadlocks on unload. The following events happen: * iscsit_del_np is called * it calls send_sig(SIGINT, np-np_thread, 1); * the scheduler switches to the np_thread * the np_thread is woken up, it sees that kthread_should_stop() returns false, so it doesn't terminate * the np_thread clears signals with flush_signals(current); and goes back to sleep in iscsit_accept_np * the scheduler switches back to iscsit_del_np * iscsit_del_np calls kthread_stop(np-np_thread); * the np_thread is waiting in iscsit_accept_np and it doesn't respond to kthread_stop The deadlock could be resolved if the administrator sends SIGINT signal to the np_thread with killall -INT iscsi_np The reproducible deadlock was introduced in commit db6077fd0b7dd41dc6ff18329cec979379071f87, but the thread-stopping code was racy even before. This patch fixes the problem. Using kthread_should_stop to stop the np_thread is unreliable, so we test np_thread_state instead. If np_thread_state equals ISCSI_NP_THREAD_SHUTDOWN, the thread exits. Signed-off-by: Mikulas Patocka mpato...@redhat.com Cc: sta...@vger.kernel.org Apologies for the delayed response.. Applied to target-pending/master and including in the next -rc3 PULL request. Also FYI, I've added '3.12+' to the stable tag to match how far back commit db6077fd0 has been included in stable. Thanks, --nab Hi I think db6077fd0 should be backported to stable kernels beginning with 3.10 (because they set np-np_thread = NULL in __iscsi_target_login_thread). The current 3.10-stable branch misses this patch. This patch for unload deadlock should be backported to all stable kernels (because unload is racy there), but because of different code, we should make a different patch for old stable branches. For example in 3.4.95, __iscsi_target_login_thread contains this code: spin_lock_bh(np-np_thread_lock); if (np-np_thread_state == ISCSI_NP_THREAD_RESET) { np-np_thread_state = ISCSI_NP_THREAD_ACTIVE; complete(np-np_restart_comp); } else { np-np_thread_state = ISCSI_NP_THREAD_ACTIVE; } spin_unlock_bh(np-np_thread_lock); If the state is ISCSI_NP_THREAD_SHUTDOWN, the above piece of code will change it to ISCSI_NP_THREAD_ACTIVE and open the same kthread_should_stop race. Mikulas -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] target: fix deadlock on unload
Hi Mikulas, On Mon, 2014-06-23 at 13:42 -0400, Mikulas Patocka wrote: > target: fix deadlock on unload > > On uniprocessor preemptible kernel, target core deadlocks on unload. The > following events happen: > * iscsit_del_np is called > * it calls send_sig(SIGINT, np->np_thread, 1); > * the scheduler switches to the np_thread > * the np_thread is woken up, it sees that kthread_should_stop() returns > false, so it doesn't terminate > * the np_thread clears signals with flush_signals(current); and goes back > to sleep in iscsit_accept_np > * the scheduler switches back to iscsit_del_np > * iscsit_del_np calls kthread_stop(np->np_thread); > * the np_thread is waiting in iscsit_accept_np and it doesn't respond to > kthread_stop > > The deadlock could be resolved if the administrator sends SIGINT signal to > the np_thread with killall -INT iscsi_np > > The reproducible deadlock was introduced in commit > db6077fd0b7dd41dc6ff18329cec979379071f87, but the thread-stopping code was > racy even before. > > This patch fixes the problem. Using kthread_should_stop to stop the > np_thread is unreliable, so we test np_thread_state instead. If > np_thread_state equals ISCSI_NP_THREAD_SHUTDOWN, the thread exits. > > Signed-off-by: Mikulas Patocka > Cc: sta...@vger.kernel.org > Apologies for the delayed response.. Applied to target-pending/master and including in the next -rc3 PULL request. Also FYI, I've added '3.12+' to the stable tag to match how far back commit db6077fd0 has been included in stable. Thanks, --nab -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] target: fix deadlock on unload
Hi Mikulas, On Mon, 2014-06-23 at 13:42 -0400, Mikulas Patocka wrote: target: fix deadlock on unload On uniprocessor preemptible kernel, target core deadlocks on unload. The following events happen: * iscsit_del_np is called * it calls send_sig(SIGINT, np-np_thread, 1); * the scheduler switches to the np_thread * the np_thread is woken up, it sees that kthread_should_stop() returns false, so it doesn't terminate * the np_thread clears signals with flush_signals(current); and goes back to sleep in iscsit_accept_np * the scheduler switches back to iscsit_del_np * iscsit_del_np calls kthread_stop(np-np_thread); * the np_thread is waiting in iscsit_accept_np and it doesn't respond to kthread_stop The deadlock could be resolved if the administrator sends SIGINT signal to the np_thread with killall -INT iscsi_np The reproducible deadlock was introduced in commit db6077fd0b7dd41dc6ff18329cec979379071f87, but the thread-stopping code was racy even before. This patch fixes the problem. Using kthread_should_stop to stop the np_thread is unreliable, so we test np_thread_state instead. If np_thread_state equals ISCSI_NP_THREAD_SHUTDOWN, the thread exits. Signed-off-by: Mikulas Patocka mpato...@redhat.com Cc: sta...@vger.kernel.org Apologies for the delayed response.. Applied to target-pending/master and including in the next -rc3 PULL request. Also FYI, I've added '3.12+' to the stable tag to match how far back commit db6077fd0 has been included in stable. Thanks, --nab -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH] target: fix deadlock on unload
target: fix deadlock on unload On uniprocessor preemptible kernel, target core deadlocks on unload. The following events happen: * iscsit_del_np is called * it calls send_sig(SIGINT, np->np_thread, 1); * the scheduler switches to the np_thread * the np_thread is woken up, it sees that kthread_should_stop() returns false, so it doesn't terminate * the np_thread clears signals with flush_signals(current); and goes back to sleep in iscsit_accept_np * the scheduler switches back to iscsit_del_np * iscsit_del_np calls kthread_stop(np->np_thread); * the np_thread is waiting in iscsit_accept_np and it doesn't respond to kthread_stop The deadlock could be resolved if the administrator sends SIGINT signal to the np_thread with killall -INT iscsi_np The reproducible deadlock was introduced in commit db6077fd0b7dd41dc6ff18329cec979379071f87, but the thread-stopping code was racy even before. This patch fixes the problem. Using kthread_should_stop to stop the np_thread is unreliable, so we test np_thread_state instead. If np_thread_state equals ISCSI_NP_THREAD_SHUTDOWN, the thread exits. Signed-off-by: Mikulas Patocka Cc: sta...@vger.kernel.org --- drivers/target/iscsi/iscsi_target_login.c | 13 +++-- 1 file changed, 7 insertions(+), 6 deletions(-) Index: linux-3.14.8/drivers/target/iscsi/iscsi_target_login.c === --- linux-3.14.8.orig/drivers/target/iscsi/iscsi_target_login.c 2014-06-19 00:10:44.634113370 +0200 +++ linux-3.14.8/drivers/target/iscsi/iscsi_target_login.c 2014-06-23 19:05:44.887437402 +0200 @@ -1196,7 +1196,7 @@ old_sess_out: static int __iscsi_target_login_thread(struct iscsi_np *np) { u8 *buffer, zero_tsih = 0; - int ret = 0, rc, stop; + int ret = 0, rc; struct iscsi_conn *conn = NULL; struct iscsi_login *login; struct iscsi_portal_group *tpg = NULL; @@ -1210,6 +1210,9 @@ static int __iscsi_target_login_thread(s if (np->np_thread_state == ISCSI_NP_THREAD_RESET) { np->np_thread_state = ISCSI_NP_THREAD_ACTIVE; complete(>np_restart_comp); + } else if (np->np_thread_state == ISCSI_NP_THREAD_SHUTDOWN) { + spin_unlock_bh(>np_thread_lock); + goto exit; } else { np->np_thread_state = ISCSI_NP_THREAD_ACTIVE; } @@ -1402,10 +1405,8 @@ old_sess_out: } out: - stop = kthread_should_stop(); - /* Wait for another socket.. */ - if (!stop) - return 1; + return 1; + exit: iscsi_stop_login_thread_timer(np); spin_lock_bh(>np_thread_lock); @@ -1422,7 +1423,7 @@ int iscsi_target_login_thread(void *arg) allow_signal(SIGINT); - while (!kthread_should_stop()) { + while (1) { ret = __iscsi_target_login_thread(np); /* * We break and exit here unless another sock_accept() call -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH] target: fix deadlock on unload
target: fix deadlock on unload On uniprocessor preemptible kernel, target core deadlocks on unload. The following events happen: * iscsit_del_np is called * it calls send_sig(SIGINT, np-np_thread, 1); * the scheduler switches to the np_thread * the np_thread is woken up, it sees that kthread_should_stop() returns false, so it doesn't terminate * the np_thread clears signals with flush_signals(current); and goes back to sleep in iscsit_accept_np * the scheduler switches back to iscsit_del_np * iscsit_del_np calls kthread_stop(np-np_thread); * the np_thread is waiting in iscsit_accept_np and it doesn't respond to kthread_stop The deadlock could be resolved if the administrator sends SIGINT signal to the np_thread with killall -INT iscsi_np The reproducible deadlock was introduced in commit db6077fd0b7dd41dc6ff18329cec979379071f87, but the thread-stopping code was racy even before. This patch fixes the problem. Using kthread_should_stop to stop the np_thread is unreliable, so we test np_thread_state instead. If np_thread_state equals ISCSI_NP_THREAD_SHUTDOWN, the thread exits. Signed-off-by: Mikulas Patocka mpato...@redhat.com Cc: sta...@vger.kernel.org --- drivers/target/iscsi/iscsi_target_login.c | 13 +++-- 1 file changed, 7 insertions(+), 6 deletions(-) Index: linux-3.14.8/drivers/target/iscsi/iscsi_target_login.c === --- linux-3.14.8.orig/drivers/target/iscsi/iscsi_target_login.c 2014-06-19 00:10:44.634113370 +0200 +++ linux-3.14.8/drivers/target/iscsi/iscsi_target_login.c 2014-06-23 19:05:44.887437402 +0200 @@ -1196,7 +1196,7 @@ old_sess_out: static int __iscsi_target_login_thread(struct iscsi_np *np) { u8 *buffer, zero_tsih = 0; - int ret = 0, rc, stop; + int ret = 0, rc; struct iscsi_conn *conn = NULL; struct iscsi_login *login; struct iscsi_portal_group *tpg = NULL; @@ -1210,6 +1210,9 @@ static int __iscsi_target_login_thread(s if (np-np_thread_state == ISCSI_NP_THREAD_RESET) { np-np_thread_state = ISCSI_NP_THREAD_ACTIVE; complete(np-np_restart_comp); + } else if (np-np_thread_state == ISCSI_NP_THREAD_SHUTDOWN) { + spin_unlock_bh(np-np_thread_lock); + goto exit; } else { np-np_thread_state = ISCSI_NP_THREAD_ACTIVE; } @@ -1402,10 +1405,8 @@ old_sess_out: } out: - stop = kthread_should_stop(); - /* Wait for another socket.. */ - if (!stop) - return 1; + return 1; + exit: iscsi_stop_login_thread_timer(np); spin_lock_bh(np-np_thread_lock); @@ -1422,7 +1423,7 @@ int iscsi_target_login_thread(void *arg) allow_signal(SIGINT); - while (!kthread_should_stop()) { + while (1) { ret = __iscsi_target_login_thread(np); /* * We break and exit here unless another sock_accept() call -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/