* [PATCH] iscsi_target: race condition on shutdown @ 2013-12-05 13:54 Hannes Reinecke 2013-12-11 23:44 ` Nicholas A. Bellinger 0 siblings, 1 reply; 7+ messages in thread From: Hannes Reinecke @ 2013-12-05 13:54 UTC (permalink / raw) To: Nic Bellinger; +Cc: target-devel, linux-scsi, Hannes Reinecke When shutting down a target there is a race condition between iscsit_del_np() and __iscsi_target_login_thread(). The latter sets the thread pointer to NULL, and the former tries to issue kthread_stop() on that pointer without any synchronization. This patchs adds proper synchronization pointer between those calls to ensure that a) the thread is correctly terminate and b) kthread_stop() isn't called with a NULL pointer. In the long run iscsi_target_login_thread() should be converted into a workqueue. Signed-off-by: Hannes Reinecke <hare@suse.de> --- drivers/target/iscsi/iscsi_target.c | 12 +++++++++--- drivers/target/iscsi/iscsi_target_login.c | 9 ++++++--- 2 files changed, 15 insertions(+), 6 deletions(-) diff --git a/drivers/target/iscsi/iscsi_target.c b/drivers/target/iscsi/iscsi_target.c index bf76fc4..c7bf3c9 100644 --- a/drivers/target/iscsi/iscsi_target.c +++ b/drivers/target/iscsi/iscsi_target.c @@ -457,15 +457,21 @@ int iscsit_del_np(struct iscsi_np *np) } np->np_thread_state = ISCSI_NP_THREAD_SHUTDOWN; spin_unlock_bh(&np->np_thread_lock); - - if (np->np_thread) { + /* Give __iscsi_target_login_thread() a chance to run */ + schedule(); + spin_lock_bh(&np->np_thread_lock); + if ((np->np_thread_state == ISCSI_NP_THREAD_SHUTDOWN) + && np->np_thread) { + np->np_thread_state = ISCSI_NP_THREAD_EXIT; + spin_unlock_bh(&np->np_thread_lock); /* * We need to send the signal to wakeup Linux/Net * which may be sleeping in sock_accept().. */ send_sig(SIGINT, np->np_thread, 1); kthread_stop(np->np_thread); - } + } else + spin_unlock_bh(&np->np_thread_lock); np->np_transport->iscsit_free_np(np); diff --git a/drivers/target/iscsi/iscsi_target_login.c b/drivers/target/iscsi/iscsi_target_login.c index 4eb93b2..b375d26 100644 --- a/drivers/target/iscsi/iscsi_target_login.c +++ b/drivers/target/iscsi/iscsi_target_login.c @@ -1405,7 +1405,8 @@ out: stop = kthread_should_stop(); if (!stop && signal_pending(current)) { spin_lock_bh(&np->np_thread_lock); - stop = (np->np_thread_state == ISCSI_NP_THREAD_SHUTDOWN); + stop = (np->np_thread_state == ISCSI_NP_THREAD_SHUTDOWN || + np->np_thread_state == ISCSI_NP_THREAD_EXIT); spin_unlock_bh(&np->np_thread_lock); } /* Wait for another socket.. */ @@ -1414,8 +1415,10 @@ out: exit: iscsi_stop_login_thread_timer(np); spin_lock_bh(&np->np_thread_lock); - np->np_thread_state = ISCSI_NP_THREAD_EXIT; - np->np_thread = NULL; + if (np->np_thread_state != ISCSI_NP_THREAD_EXIT) { + np->np_thread_state = ISCSI_NP_THREAD_EXIT; + np->np_thread = NULL; + } spin_unlock_bh(&np->np_thread_lock); return 0; -- 1.7.12.4 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] iscsi_target: race condition on shutdown 2013-12-05 13:54 [PATCH] iscsi_target: race condition on shutdown Hannes Reinecke @ 2013-12-11 23:44 ` Nicholas A. Bellinger 2013-12-12 7:18 ` Hannes Reinecke 0 siblings, 1 reply; 7+ messages in thread From: Nicholas A. Bellinger @ 2013-12-11 23:44 UTC (permalink / raw) To: Hannes Reinecke; +Cc: Nic Bellinger, target-devel, linux-scsi Hi Hannes, Btw, apologies for the delayed response on this.. Comments are below. On Thu, 2013-12-05 at 14:54 +0100, Hannes Reinecke wrote: > When shutting down a target there is a race condition between > iscsit_del_np() and __iscsi_target_login_thread(). > The latter sets the thread pointer to NULL, and the former > tries to issue kthread_stop() on that pointer without any > synchronization. > > This patchs adds proper synchronization pointer between those > calls to ensure that a) the thread is correctly terminate and > b) kthread_stop() isn't called with a NULL pointer. > > In the long run iscsi_target_login_thread() should be converted > into a workqueue. > > Signed-off-by: Hannes Reinecke <hare@suse.de> > --- > drivers/target/iscsi/iscsi_target.c | 12 +++++++++--- > drivers/target/iscsi/iscsi_target_login.c | 9 ++++++--- > 2 files changed, 15 insertions(+), 6 deletions(-) > > diff --git a/drivers/target/iscsi/iscsi_target.c b/drivers/target/iscsi/iscsi_target.c > index bf76fc4..c7bf3c9 100644 > --- a/drivers/target/iscsi/iscsi_target.c > +++ b/drivers/target/iscsi/iscsi_target.c > @@ -457,15 +457,21 @@ int iscsit_del_np(struct iscsi_np *np) > } > np->np_thread_state = ISCSI_NP_THREAD_SHUTDOWN; > spin_unlock_bh(&np->np_thread_lock); > - > - if (np->np_thread) { > + /* Give __iscsi_target_login_thread() a chance to run */ > + schedule(); > + spin_lock_bh(&np->np_thread_lock); > + if ((np->np_thread_state == ISCSI_NP_THREAD_SHUTDOWN) > + && np->np_thread) { > + np->np_thread_state = ISCSI_NP_THREAD_EXIT; > + spin_unlock_bh(&np->np_thread_lock); > /* > * We need to send the signal to wakeup Linux/Net > * which may be sleeping in sock_accept().. > */ > send_sig(SIGINT, np->np_thread, 1); > kthread_stop(np->np_thread); > - } > + } else > + spin_unlock_bh(&np->np_thread_lock); > > np->np_transport->iscsit_free_np(np); > > diff --git a/drivers/target/iscsi/iscsi_target_login.c b/drivers/target/iscsi/iscsi_target_login.c > index 4eb93b2..b375d26 100644 > --- a/drivers/target/iscsi/iscsi_target_login.c > +++ b/drivers/target/iscsi/iscsi_target_login.c > @@ -1405,7 +1405,8 @@ out: > stop = kthread_should_stop(); > if (!stop && signal_pending(current)) { > spin_lock_bh(&np->np_thread_lock); > - stop = (np->np_thread_state == ISCSI_NP_THREAD_SHUTDOWN); > + stop = (np->np_thread_state == ISCSI_NP_THREAD_SHUTDOWN || > + np->np_thread_state == ISCSI_NP_THREAD_EXIT); > spin_unlock_bh(&np->np_thread_lock); > } > /* Wait for another socket.. */ > @@ -1414,8 +1415,10 @@ out: > exit: > iscsi_stop_login_thread_timer(np); > spin_lock_bh(&np->np_thread_lock); > - np->np_thread_state = ISCSI_NP_THREAD_EXIT; > - np->np_thread = NULL; > + if (np->np_thread_state != ISCSI_NP_THREAD_EXIT) { > + np->np_thread_state = ISCSI_NP_THREAD_EXIT; > + np->np_thread = NULL; > + } > spin_unlock_bh(&np->np_thread_lock); > > return 0; I'm not sure this extra logic is necessary. How about just clearing np->np_thread in iscsit_del_np instead..? Care to verify on your side with the following patch..? --nab diff --git a/drivers/target/iscsi/iscsi_target.c b/drivers/target/iscsi/iscsi_target.c index 02182ab..0086719 100644 --- a/drivers/target/iscsi/iscsi_target.c +++ b/drivers/target/iscsi/iscsi_target.c @@ -465,6 +465,7 @@ int iscsit_del_np(struct iscsi_np *np) */ send_sig(SIGINT, np->np_thread, 1); kthread_stop(np->np_thread); + np->np_thread = NULL; } np->np_transport->iscsit_free_np(np); diff --git a/drivers/target/iscsi/iscsi_target_login.c b/drivers/target/iscsi/iscsi_target_login.c index 4eb93b2..6ab43b6 100644 --- a/drivers/target/iscsi/iscsi_target_login.c +++ b/drivers/target/iscsi/iscsi_target_login.c @@ -1415,7 +1415,6 @@ exit: iscsi_stop_login_thread_timer(np); spin_lock_bh(&np->np_thread_lock); np->np_thread_state = ISCSI_NP_THREAD_EXIT; - np->np_thread = NULL; spin_unlock_bh(&np->np_thread_lock); return 0; ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] iscsi_target: race condition on shutdown 2013-12-11 23:44 ` Nicholas A. Bellinger @ 2013-12-12 7:18 ` Hannes Reinecke 2013-12-12 7:54 ` Nicholas A. Bellinger 0 siblings, 1 reply; 7+ messages in thread From: Hannes Reinecke @ 2013-12-12 7:18 UTC (permalink / raw) To: Nicholas A. Bellinger; +Cc: Nic Bellinger, target-devel, linux-scsi On 12/12/2013 12:44 AM, Nicholas A. Bellinger wrote: > Hi Hannes, > > Btw, apologies for the delayed response on this.. Comments are below. > > On Thu, 2013-12-05 at 14:54 +0100, Hannes Reinecke wrote: >> When shutting down a target there is a race condition between >> iscsit_del_np() and __iscsi_target_login_thread(). >> The latter sets the thread pointer to NULL, and the former >> tries to issue kthread_stop() on that pointer without any >> synchronization. >> >> This patchs adds proper synchronization pointer between those >> calls to ensure that a) the thread is correctly terminate and >> b) kthread_stop() isn't called with a NULL pointer. >> >> In the long run iscsi_target_login_thread() should be converted >> into a workqueue. >> >> Signed-off-by: Hannes Reinecke <hare@suse.de> >> --- >> drivers/target/iscsi/iscsi_target.c | 12 +++++++++--- >> drivers/target/iscsi/iscsi_target_login.c | 9 ++++++--- >> 2 files changed, 15 insertions(+), 6 deletions(-) >> >> diff --git a/drivers/target/iscsi/iscsi_target.c b/drivers/target/iscsi/iscsi_target.c >> index bf76fc4..c7bf3c9 100644 >> --- a/drivers/target/iscsi/iscsi_target.c >> +++ b/drivers/target/iscsi/iscsi_target.c >> @@ -457,15 +457,21 @@ int iscsit_del_np(struct iscsi_np *np) >> } >> np->np_thread_state = ISCSI_NP_THREAD_SHUTDOWN; >> spin_unlock_bh(&np->np_thread_lock); >> - >> - if (np->np_thread) { >> + /* Give __iscsi_target_login_thread() a chance to run */ >> + schedule(); >> + spin_lock_bh(&np->np_thread_lock); >> + if ((np->np_thread_state == ISCSI_NP_THREAD_SHUTDOWN) >> + && np->np_thread) { >> + np->np_thread_state = ISCSI_NP_THREAD_EXIT; >> + spin_unlock_bh(&np->np_thread_lock); >> /* >> * We need to send the signal to wakeup Linux/Net >> * which may be sleeping in sock_accept().. >> */ >> send_sig(SIGINT, np->np_thread, 1); >> kthread_stop(np->np_thread); >> - } >> + } else >> + spin_unlock_bh(&np->np_thread_lock); >> >> np->np_transport->iscsit_free_np(np); >> >> diff --git a/drivers/target/iscsi/iscsi_target_login.c b/drivers/target/iscsi/iscsi_target_login.c >> index 4eb93b2..b375d26 100644 >> --- a/drivers/target/iscsi/iscsi_target_login.c >> +++ b/drivers/target/iscsi/iscsi_target_login.c >> @@ -1405,7 +1405,8 @@ out: >> stop = kthread_should_stop(); >> if (!stop && signal_pending(current)) { >> spin_lock_bh(&np->np_thread_lock); >> - stop = (np->np_thread_state == ISCSI_NP_THREAD_SHUTDOWN); >> + stop = (np->np_thread_state == ISCSI_NP_THREAD_SHUTDOWN || >> + np->np_thread_state == ISCSI_NP_THREAD_EXIT); >> spin_unlock_bh(&np->np_thread_lock); >> } >> /* Wait for another socket.. */ >> @@ -1414,8 +1415,10 @@ out: >> exit: >> iscsi_stop_login_thread_timer(np); >> spin_lock_bh(&np->np_thread_lock); >> - np->np_thread_state = ISCSI_NP_THREAD_EXIT; >> - np->np_thread = NULL; >> + if (np->np_thread_state != ISCSI_NP_THREAD_EXIT) { >> + np->np_thread_state = ISCSI_NP_THREAD_EXIT; >> + np->np_thread = NULL; >> + } >> spin_unlock_bh(&np->np_thread_lock); >> >> return 0; > > I'm not sure this extra logic is necessary. How about just clearing > np->np_thread in iscsit_del_np instead..? > > Care to verify on your side with the following patch..? > > --nab > > diff --git a/drivers/target/iscsi/iscsi_target.c b/drivers/target/iscsi/iscsi_target.c > index 02182ab..0086719 100644 > --- a/drivers/target/iscsi/iscsi_target.c > +++ b/drivers/target/iscsi/iscsi_target.c > @@ -465,6 +465,7 @@ int iscsit_del_np(struct iscsi_np *np) > */ > send_sig(SIGINT, np->np_thread, 1); > kthread_stop(np->np_thread); > + np->np_thread = NULL; > } > > np->np_transport->iscsit_free_np(np); > diff --git a/drivers/target/iscsi/iscsi_target_login.c b/drivers/target/iscsi/iscsi_target_login.c > index 4eb93b2..6ab43b6 100644 > --- a/drivers/target/iscsi/iscsi_target_login.c > +++ b/drivers/target/iscsi/iscsi_target_login.c > @@ -1415,7 +1415,6 @@ exit: > iscsi_stop_login_thread_timer(np); > spin_lock_bh(&np->np_thread_lock); > np->np_thread_state = ISCSI_NP_THREAD_EXIT; > - np->np_thread = NULL; > spin_unlock_bh(&np->np_thread_lock); > > return 0; > > The problem here is that 'kthread_stop()' is supposed to be called with a _valid_ task structure. There is this race window: np->np_thread_state = ISCSI_NP_THREAD_SHUTDOWN; spin_unlock_bh(&np->np_thread_lock); here -> if (np->np_thread) { /* If the login thread exits before we evaluate 'np->np_thread' the pointer is stale and kthread_stop will be called with an invalid task structure. So at the very least we need to check the thread_state before evaluating 'np->np_thread' (which will evaluate to 'true' anyway if we were to follow up with your patch). But in doing so we would need to protect is by the thread_lock to synchronize the state. And we'll end up with quite the same patch as I've send originally. In fact, it was an invalid call to kthread_stop() which triggered the whole patch in the first place :-) I would love to be proven wrong, as I'm not keen on the 'schedule()' in there. But I fail to see another way out here, short of converting the entire kthread into a workqueue item ... Cheers, Hannes -- Dr. Hannes Reinecke zSeries & Storage hare@suse.de +49 911 74053 688 SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg) ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] iscsi_target: race condition on shutdown 2013-12-12 7:18 ` Hannes Reinecke @ 2013-12-12 7:54 ` Nicholas A. Bellinger 2013-12-12 8:05 ` Hannes Reinecke 0 siblings, 1 reply; 7+ messages in thread From: Nicholas A. Bellinger @ 2013-12-12 7:54 UTC (permalink / raw) To: Hannes Reinecke; +Cc: Nic Bellinger, target-devel, linux-scsi On Thu, 2013-12-12 at 08:18 +0100, Hannes Reinecke wrote: > On 12/12/2013 12:44 AM, Nicholas A. Bellinger wrote: <SNIP> > > I'm not sure this extra logic is necessary. How about just clearing > > np->np_thread in iscsit_del_np instead..? > > > > Care to verify on your side with the following patch..? > > > > --nab > > > > diff --git a/drivers/target/iscsi/iscsi_target.c b/drivers/target/iscsi/iscsi_target.c > > index 02182ab..0086719 100644 > > --- a/drivers/target/iscsi/iscsi_target.c > > +++ b/drivers/target/iscsi/iscsi_target.c > > @@ -465,6 +465,7 @@ int iscsit_del_np(struct iscsi_np *np) > > */ > > send_sig(SIGINT, np->np_thread, 1); > > kthread_stop(np->np_thread); > > + np->np_thread = NULL; > > } > > > > np->np_transport->iscsit_free_np(np); > > diff --git a/drivers/target/iscsi/iscsi_target_login.c b/drivers/target/iscsi/iscsi_target_login.c > > index 4eb93b2..6ab43b6 100644 > > --- a/drivers/target/iscsi/iscsi_target_login.c > > +++ b/drivers/target/iscsi/iscsi_target_login.c > > @@ -1415,7 +1415,6 @@ exit: > > iscsi_stop_login_thread_timer(np); > > spin_lock_bh(&np->np_thread_lock); > > np->np_thread_state = ISCSI_NP_THREAD_EXIT; > > - np->np_thread = NULL; > > spin_unlock_bh(&np->np_thread_lock); > > > > return 0; > > > > > The problem here is that 'kthread_stop()' is supposed to be called > with a _valid_ task structure. > > There is this race window: > > np->np_thread_state = ISCSI_NP_THREAD_SHUTDOWN; > spin_unlock_bh(&np->np_thread_lock); > here -> > if (np->np_thread) { > /* > > If the login thread exits before we evaluate 'np->np_thread' > the pointer is stale and kthread_stop will be called with > an invalid task structure. > > So at the very least we need to check the thread_state before > evaluating 'np->np_thread' (which will evaluate to 'true' anyway if > we were to follow up with your patch). > But in doing so we would need to protect is by the thread_lock > to synchronize the state. > And we'll end up with quite the same patch as I've send originally. > > In fact, it was an invalid call to kthread_stop() which triggered > the whole patch in the first place :-) > Mmmm, point taken.. > I would love to be proven wrong, as I'm not keen on the 'schedule()' > in there. But I fail to see another way out here, short of > converting the entire kthread into a workqueue item ... Thinking about this a bit more, I think the pre-kthread API np_thead_state check to exit at the out: label in __iscsi_target_login_thread() is the culprit.. How about following to only exit when iscsit_del_np() -> kthread_stop() has been called, and kthread_should_stop() is true..? --nab diff --git a/drivers/target/iscsi/iscsi_target.c b/drivers/target/iscsi/iscsi_target.c index 02182ab..0086719 100644 --- a/drivers/target/iscsi/iscsi_target.c +++ b/drivers/target/iscsi/iscsi_target.c @@ -465,6 +465,7 @@ int iscsit_del_np(struct iscsi_np *np) */ send_sig(SIGINT, np->np_thread, 1); kthread_stop(np->np_thread); + np->np_thread = NULL; } np->np_transport->iscsit_free_np(np); diff --git a/drivers/target/iscsi/iscsi_target_login.c b/drivers/target/iscsi/iscsi_target_login.c index 4eb93b2..e29279e 100644 --- a/drivers/target/iscsi/iscsi_target_login.c +++ b/drivers/target/iscsi/iscsi_target_login.c @@ -1403,11 +1403,6 @@ old_sess_out: out: stop = kthread_should_stop(); - if (!stop && signal_pending(current)) { - spin_lock_bh(&np->np_thread_lock); - stop = (np->np_thread_state == ISCSI_NP_THREAD_SHUTDOWN); - spin_unlock_bh(&np->np_thread_lock); - } /* Wait for another socket.. */ if (!stop) return 1; @@ -1415,7 +1410,6 @@ exit: iscsi_stop_login_thread_timer(np); spin_lock_bh(&np->np_thread_lock); np->np_thread_state = ISCSI_NP_THREAD_EXIT; - np->np_thread = NULL; spin_unlock_bh(&np->np_thread_lock); return 0; ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] iscsi_target: race condition on shutdown 2013-12-12 7:54 ` Nicholas A. Bellinger @ 2013-12-12 8:05 ` Hannes Reinecke 2013-12-16 19:49 ` Nicholas A. Bellinger 0 siblings, 1 reply; 7+ messages in thread From: Hannes Reinecke @ 2013-12-12 8:05 UTC (permalink / raw) To: Nicholas A. Bellinger; +Cc: Nic Bellinger, target-devel, linux-scsi On 12/12/2013 08:54 AM, Nicholas A. Bellinger wrote: > On Thu, 2013-12-12 at 08:18 +0100, Hannes Reinecke wrote: [ .. ] >> The problem here is that 'kthread_stop()' is supposed to be called >> with a _valid_ task structure. >> >> There is this race window: >> >> np->np_thread_state = ISCSI_NP_THREAD_SHUTDOWN; >> spin_unlock_bh(&np->np_thread_lock); >> here -> >> if (np->np_thread) { >> /* >> >> If the login thread exits before we evaluate 'np->np_thread' >> the pointer is stale and kthread_stop will be called with >> an invalid task structure. >> >> So at the very least we need to check the thread_state before >> evaluating 'np->np_thread' (which will evaluate to 'true' anyway if >> we were to follow up with your patch). >> But in doing so we would need to protect is by the thread_lock >> to synchronize the state. >> And we'll end up with quite the same patch as I've send originally. >> >> In fact, it was an invalid call to kthread_stop() which triggered >> the whole patch in the first place :-) >> > > Mmmm, point taken.. > >> I would love to be proven wrong, as I'm not keen on the 'schedule()' >> in there. But I fail to see another way out here, short of >> converting the entire kthread into a workqueue item ... > > Thinking about this a bit more, I think the pre-kthread API > np_thead_state check to exit at the out: label in > __iscsi_target_login_thread() is the culprit.. > > How about following to only exit when iscsit_del_np() -> kthread_stop() > has been called, and kthread_should_stop() is true..? > > --nab > > diff --git a/drivers/target/iscsi/iscsi_target.c b/drivers/target/iscsi/iscsi_target.c > index 02182ab..0086719 100644 > --- a/drivers/target/iscsi/iscsi_target.c > +++ b/drivers/target/iscsi/iscsi_target.c > @@ -465,6 +465,7 @@ int iscsit_del_np(struct iscsi_np *np) > */ > send_sig(SIGINT, np->np_thread, 1); > kthread_stop(np->np_thread); > + np->np_thread = NULL; > } > > np->np_transport->iscsit_free_np(np); > diff --git a/drivers/target/iscsi/iscsi_target_login.c b/drivers/target/iscsi/iscsi_target_login.c > index 4eb93b2..e29279e 100644 > --- a/drivers/target/iscsi/iscsi_target_login.c > +++ b/drivers/target/iscsi/iscsi_target_login.c > @@ -1403,11 +1403,6 @@ old_sess_out: > > out: > stop = kthread_should_stop(); > - if (!stop && signal_pending(current)) { > - spin_lock_bh(&np->np_thread_lock); > - stop = (np->np_thread_state == ISCSI_NP_THREAD_SHUTDOWN); > - spin_unlock_bh(&np->np_thread_lock); > - } > /* Wait for another socket.. */ > if (!stop) > return 1; > @@ -1415,7 +1410,6 @@ exit: > iscsi_stop_login_thread_timer(np); > spin_lock_bh(&np->np_thread_lock); > np->np_thread_state = ISCSI_NP_THREAD_EXIT; > - np->np_thread = NULL; > spin_unlock_bh(&np->np_thread_lock); > > return 0; > > Yes. Far better. I'll give it a spin. Cheers, Hannes -- Dr. Hannes Reinecke zSeries & Storage hare@suse.de +49 911 74053 688 SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg) ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] iscsi_target: race condition on shutdown 2013-12-12 8:05 ` Hannes Reinecke @ 2013-12-16 19:49 ` Nicholas A. Bellinger 2013-12-18 2:27 ` Nicholas A. Bellinger 0 siblings, 1 reply; 7+ messages in thread From: Nicholas A. Bellinger @ 2013-12-16 19:49 UTC (permalink / raw) To: Hannes Reinecke; +Cc: Nic Bellinger, target-devel, linux-scsi Hey Hannes, On Thu, 2013-12-12 at 09:05 +0100, Hannes Reinecke wrote: > On 12/12/2013 08:54 AM, Nicholas A. Bellinger wrote: > > On Thu, 2013-12-12 at 08:18 +0100, Hannes Reinecke wrote: > [ .. ] <SNIP> > > Mmmm, point taken.. > > > >> I would love to be proven wrong, as I'm not keen on the 'schedule()' > >> in there. But I fail to see another way out here, short of > >> converting the entire kthread into a workqueue item ... > > > > Thinking about this a bit more, I think the pre-kthread API > > np_thead_state check to exit at the out: label in > > __iscsi_target_login_thread() is the culprit.. > > > > How about following to only exit when iscsit_del_np() -> kthread_stop() > > has been called, and kthread_should_stop() is true..? > > > > --nab > > > > diff --git a/drivers/target/iscsi/iscsi_target.c b/drivers/target/iscsi/iscsi_target.c > > index 02182ab..0086719 100644 > > --- a/drivers/target/iscsi/iscsi_target.c > > +++ b/drivers/target/iscsi/iscsi_target.c > > @@ -465,6 +465,7 @@ int iscsit_del_np(struct iscsi_np *np) > > */ > > send_sig(SIGINT, np->np_thread, 1); > > kthread_stop(np->np_thread); > > + np->np_thread = NULL; > > } > > > > np->np_transport->iscsit_free_np(np); > > diff --git a/drivers/target/iscsi/iscsi_target_login.c b/drivers/target/iscsi/iscsi_target_login.c > > index 4eb93b2..e29279e 100644 > > --- a/drivers/target/iscsi/iscsi_target_login.c > > +++ b/drivers/target/iscsi/iscsi_target_login.c > > @@ -1403,11 +1403,6 @@ old_sess_out: > > > > out: > > stop = kthread_should_stop(); > > - if (!stop && signal_pending(current)) { > > - spin_lock_bh(&np->np_thread_lock); > > - stop = (np->np_thread_state == ISCSI_NP_THREAD_SHUTDOWN); > > - spin_unlock_bh(&np->np_thread_lock); > > - } > > /* Wait for another socket.. */ > > if (!stop) > > return 1; > > @@ -1415,7 +1410,6 @@ exit: > > iscsi_stop_login_thread_timer(np); > > spin_lock_bh(&np->np_thread_lock); > > np->np_thread_state = ISCSI_NP_THREAD_EXIT; > > - np->np_thread = NULL; > > spin_unlock_bh(&np->np_thread_lock); > > > > return 0; > > > > > Yes. Far better. > > I'll give it a spin. > Any chance to confirm this patch..? I'd like to include this in the -rc5 PULL request over the next days. --nab ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] iscsi_target: race condition on shutdown 2013-12-16 19:49 ` Nicholas A. Bellinger @ 2013-12-18 2:27 ` Nicholas A. Bellinger 0 siblings, 0 replies; 7+ messages in thread From: Nicholas A. Bellinger @ 2013-12-18 2:27 UTC (permalink / raw) To: Hannes Reinecke; +Cc: Nic Bellinger, target-devel, linux-scsi Hey Hannes, On Mon, 2013-12-16 at 11:49 -0800, Nicholas A. Bellinger wrote: > Hey Hannes, > > On Thu, 2013-12-12 at 09:05 +0100, Hannes Reinecke wrote: > > On 12/12/2013 08:54 AM, Nicholas A. Bellinger wrote: > > > On Thu, 2013-12-12 at 08:18 +0100, Hannes Reinecke wrote: > > [ .. ] > > <SNIP> > > > > Mmmm, point taken.. > > > > > >> I would love to be proven wrong, as I'm not keen on the 'schedule()' > > >> in there. But I fail to see another way out here, short of > > >> converting the entire kthread into a workqueue item ... > > > > > > Thinking about this a bit more, I think the pre-kthread API > > > np_thead_state check to exit at the out: label in > > > __iscsi_target_login_thread() is the culprit.. > > > > > > How about following to only exit when iscsit_del_np() -> kthread_stop() > > > has been called, and kthread_should_stop() is true..? > > > > > > --nab > > > > > > diff --git a/drivers/target/iscsi/iscsi_target.c b/drivers/target/iscsi/iscsi_target.c > > > index 02182ab..0086719 100644 > > > --- a/drivers/target/iscsi/iscsi_target.c > > > +++ b/drivers/target/iscsi/iscsi_target.c > > > @@ -465,6 +465,7 @@ int iscsit_del_np(struct iscsi_np *np) > > > */ > > > send_sig(SIGINT, np->np_thread, 1); > > > kthread_stop(np->np_thread); > > > + np->np_thread = NULL; > > > } > > > > > > np->np_transport->iscsit_free_np(np); > > > diff --git a/drivers/target/iscsi/iscsi_target_login.c b/drivers/target/iscsi/iscsi_target_login.c > > > index 4eb93b2..e29279e 100644 > > > --- a/drivers/target/iscsi/iscsi_target_login.c > > > +++ b/drivers/target/iscsi/iscsi_target_login.c > > > @@ -1403,11 +1403,6 @@ old_sess_out: > > > > > > out: > > > stop = kthread_should_stop(); > > > - if (!stop && signal_pending(current)) { > > > - spin_lock_bh(&np->np_thread_lock); > > > - stop = (np->np_thread_state == ISCSI_NP_THREAD_SHUTDOWN); > > > - spin_unlock_bh(&np->np_thread_lock); > > > - } > > > /* Wait for another socket.. */ > > > if (!stop) > > > return 1; > > > @@ -1415,7 +1410,6 @@ exit: > > > iscsi_stop_login_thread_timer(np); > > > spin_lock_bh(&np->np_thread_lock); > > > np->np_thread_state = ISCSI_NP_THREAD_EXIT; > > > - np->np_thread = NULL; > > > spin_unlock_bh(&np->np_thread_lock); > > > > > > return 0; > > > > > > > > Yes. Far better. > > > > I'll give it a spin. > > > > Any chance to confirm this patch..? > > I'd like to include this in the -rc5 PULL request over the next days. Any testing feedback on this..? Waiting for your Reviewed-by + Tested-by before pushing this one. --nab ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2013-12-18 2:27 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2013-12-05 13:54 [PATCH] iscsi_target: race condition on shutdown Hannes Reinecke 2013-12-11 23:44 ` Nicholas A. Bellinger 2013-12-12 7:18 ` Hannes Reinecke 2013-12-12 7:54 ` Nicholas A. Bellinger 2013-12-12 8:05 ` Hannes Reinecke 2013-12-16 19:49 ` Nicholas A. Bellinger 2013-12-18 2:27 ` Nicholas A. Bellinger
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox