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
[email protected] +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)
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
