From b39b5d85023ac07a5ab063111fe74f86f89fdcba Mon Sep 17 00:00:00 2001 From: Vladislav Bolkhovitin Date: Sat, 17 May 2014 01:04:08 +0000 Subject: [PATCH] scst_main: Fix race between scst_resume_activity() and scst_init_thread() After SCST_FLAG_SUSPENDED has been cleared it is essential that scst_do_job_init() reexamines scst_init_cmd_list to avoid that commands get stuck in the command init list. This patch fixes the following race condition that can occur if SCST_FLAG_SUSPENDED has been set and if scst_init_cmd_list is not empty: * scst_do_job_init() returns to scst_init_thread() and leaves the commands that were on the init list on that list. * scst_init_thread() invokes test_init_cmd_list(). * test_init_cmd_list() returns false because SCST_FLAG_SUSPENDED has been set. * scst_resume_activity() clears SCST_FLAG_SUSPENDED and invokes wake_up_all(&scst_init_cmd_list_waitQ). However, since scst_init_thread() has not yet added the init thread back to scst_init_cmd_list_waitQ this wake_up_all() call doesn't do anything. * scst_init_thread() adds the init thread to scst_init_cmd_list_waitQ and unlocks scst_init_lock. Additionally, remove an unneeded smp_mb__after_clear_bit() call. wake_up_all() guarantees that if it wakes up a thread that that thread sees all store operations that were performed by the thread that invoked wake_up_all() and that preceeded the wake_up_all() invocation. Signed-off-by: Bart Van Assche git-svn-id: http://svn.code.sf.net/p/scst/svn/trunk@5524 d57e44dd-8a1f-0410-8b47-8ef2f437770f --- scst/src/scst_main.c | 13 ++++++++----- scst/src/scst_targ.c | 4 ---- 2 files changed, 8 insertions(+), 9 deletions(-) diff --git a/scst/src/scst_main.c b/scst/src/scst_main.c index 9fb82157b..867e7f6b5 100644 --- a/scst/src/scst_main.c +++ b/scst/src/scst_main.c @@ -1060,17 +1060,20 @@ static void __scst_resume_activity(void) goto out; clear_bit(SCST_FLAG_SUSPENDED, &scst_flags); - /* - * The barrier is needed to make sure all woken up threads see the - * cleared flag. Not sure if it's really needed, but let's be safe. - */ - smp_mb__after_clear_bit(); mutex_lock(&scst_cmd_threads_mutex); list_for_each_entry(l, &scst_cmd_threads_list, lists_list_entry) { wake_up_all(&l->cmd_list_waitQ); } mutex_unlock(&scst_cmd_threads_mutex); + + /* + * Wait until scst_init_thread() either is waiting or has reexamined + * scst_flags. + */ + spin_lock_irq(&scst_init_lock); + spin_unlock_irq(&scst_init_lock); + wake_up_all(&scst_init_cmd_list_waitQ); spin_lock_irq(&scst_mcmd_lock); diff --git a/scst/src/scst_targ.c b/scst/src/scst_targ.c index 98b11ddc9..e00fbde4d 100644 --- a/scst/src/scst_targ.c +++ b/scst/src/scst_targ.c @@ -4547,10 +4547,6 @@ restart: goto restart; } - /* It isn't really needed, but let's keep it */ - if (susp != test_bit(SCST_FLAG_SUSPENDED, &scst_flags)) - goto restart; - TRACE_EXIT(); return; }