Re: [PATCH] iscsi_target: race condition on shutdown

[Hannes Reinecke <hare@suse.de>]

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)