* [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