Re: [PATCH] iscsi_target: race condition on shutdown

[Hannes Reinecke <hare@suse.de>]

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)