diff mbox

RPC: killing RPC tasks races fixed

Message ID 20110317121638.15035.39410.stgit@localhost6.localdomain6
State Not Applicable, archived
Delegated to: David Miller
Headers show

Commit Message

Stanislav Kinsbursky March 17, 2011, 12:16 p.m. UTC 
task->tk_waitqueue must be checked for NULL before trying to wake up task in
rpc_killall_tasks() because it can be NULL.

Here is an example:

CPU 0               	CPU 1				CPU 2
--------------------	---------------------	--------------------------
nfs4_run_open_task
rpc_run_task
rpc_execute
rpc_set_active
rpc_make_runnable
(waiting)
			rpc_async_schedule
			nfs4_open_prepare
			nfs_wait_on_sequence
						nfs_umount_begin
						rpc_killall_tasks
						rpc_wake_up_task
						rpc_wake_up_queued_task
						spin_lock(tk_waitqueue == NULL)
						BUG()
			rpc_sleep_on
			spin_lock(&q->lock)
			__rpc_sleep_on
			task->tk_waitqueue = q

Signed-off-by: Stanislav Kinsbursky <skinsbursky@openvz.org>

---
 net/sunrpc/clnt.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Trond Myklebust March 17, 2011, 1:01 p.m. UTC | #1 
On Thu, 2011-03-17 at 15:16 +0300, Stanislav Kinsbursky wrote:
> task->tk_waitqueue must be checked for NULL before trying to wake up task in
> rpc_killall_tasks() because it can be NULL.
> 
> Here is an example:
> 
> CPU 0               	CPU 1				CPU 2
> --------------------	---------------------	--------------------------
> nfs4_run_open_task
> rpc_run_task
> rpc_execute
> rpc_set_active
> rpc_make_runnable
> (waiting)
> 			rpc_async_schedule
> 			nfs4_open_prepare
> 			nfs_wait_on_sequence
> 						nfs_umount_begin
> 						rpc_killall_tasks
> 						rpc_wake_up_task
> 						rpc_wake_up_queued_task
> 						spin_lock(tk_waitqueue == NULL)
> 						BUG()
> 			rpc_sleep_on
> 			spin_lock(&q->lock)
> 			__rpc_sleep_on
> 			task->tk_waitqueue = q
> 
> Signed-off-by: Stanislav Kinsbursky <skinsbursky@openvz.org>
> 
> ---
>  net/sunrpc/clnt.c |    4 +++-
>  1 files changed, 3 insertions(+), 1 deletions(-)
> 
> diff --git a/net/sunrpc/clnt.c b/net/sunrpc/clnt.c
> index 57d344c..24039fe 100644
> --- a/net/sunrpc/clnt.c
> +++ b/net/sunrpc/clnt.c
> @@ -436,7 +436,9 @@ void rpc_killall_tasks(struct rpc_clnt *clnt)
>  		if (!(rovr->tk_flags & RPC_TASK_KILLED)) {
>  			rovr->tk_flags |= RPC_TASK_KILLED;
>  			rpc_exit(rovr, -EIO);
> -			rpc_wake_up_queued_task(rovr->tk_waitqueue, rovr);
> +			if (rovr->tk_waitqueue)
> +				rpc_wake_up_queued_task(rovr->tk_waitqueue, 
> +							rovr);

Testing for RPC_IS_QUEUED(rovr) would be better, since that would
optimise away the call to rpc_wake_up_queued_task() altogether for those
tasks that aren't queued.

>  		}
>  	}
>  	spin_unlock(&clnt->cl_lock);
>
Stanislav Kinsbursky March 17, 2011, 3:43 p.m. UTC | #2 
17.03.2011 16:01, Trond Myklebust пишет:
> On Thu, 2011-03-17 at 15:16 +0300, Stanislav Kinsbursky wrote:
>> task->tk_waitqueue must be checked for NULL before trying to wake up task in
>> rpc_killall_tasks() because it can be NULL.
>>
>> Here is an example:
>>
>> CPU 0               	CPU 1				CPU 2
>> --------------------	---------------------	--------------------------
>> nfs4_run_open_task
>> rpc_run_task
>> rpc_execute
>> rpc_set_active
>> rpc_make_runnable
>> (waiting)
>> 			rpc_async_schedule
>> 			nfs4_open_prepare
>> 			nfs_wait_on_sequence
>> 						nfs_umount_begin
>> 						rpc_killall_tasks
>> 						rpc_wake_up_task
>> 						rpc_wake_up_queued_task
>> 						spin_lock(tk_waitqueue == NULL)
>> 						BUG()
>> 			rpc_sleep_on
>> 			spin_lock(&q->lock)
>> 			__rpc_sleep_on
>> 			task->tk_waitqueue = q
>>
>> Signed-off-by: Stanislav Kinsbursky<skinsbursky@openvz.org>
>>
>> ---
>>   net/sunrpc/clnt.c |    4 +++-
>>   1 files changed, 3 insertions(+), 1 deletions(-)
>>
>> diff --git a/net/sunrpc/clnt.c b/net/sunrpc/clnt.c
>> index 57d344c..24039fe 100644
>> --- a/net/sunrpc/clnt.c
>> +++ b/net/sunrpc/clnt.c
>> @@ -436,7 +436,9 @@ void rpc_killall_tasks(struct rpc_clnt *clnt)
>>   		if (!(rovr->tk_flags&  RPC_TASK_KILLED)) {
>>   			rovr->tk_flags |= RPC_TASK_KILLED;
>>   			rpc_exit(rovr, -EIO);
>> -			rpc_wake_up_queued_task(rovr->tk_waitqueue, rovr);
>> +			if (rovr->tk_waitqueue)
>> +				rpc_wake_up_queued_task(rovr->tk_waitqueue,
>> +							rovr);
>
> Testing for RPC_IS_QUEUED(rovr) would be better, since that would
> optimise away the call to rpc_wake_up_queued_task() altogether for those
> tasks that aren't queued.
>

Yes, I agree with testing RPC_IS_QUEUED(rovr) since such approach looks
clearer and in 2.6.38 tk_waitqueue is initialized prior to set
RPC_TASK_QUEUED bit.
But I found this problem in 2.6.32 rhel kernel where this set sequence is inversed.
Will send fixed version soon.

>>   		}
>>   	}
>>   	spin_unlock(&clnt->cl_lock);
>>
>
Trond Myklebust March 17, 2011, 4:44 p.m. UTC | #3 
On Thu, 2011-03-17 at 18:43 +0300, Stanislav Kinsbursky wrote:
> 17.03.2011 16:01, Trond Myklebust пишет:
> > On Thu, 2011-03-17 at 15:16 +0300, Stanislav Kinsbursky wrote:
> >> task->tk_waitqueue must be checked for NULL before trying to wake up task in
> >> rpc_killall_tasks() because it can be NULL.
> >>
> >> Here is an example:
> >>
> >> CPU 0               	CPU 1				CPU 2
> >> --------------------	---------------------	--------------------------
> >> nfs4_run_open_task
> >> rpc_run_task
> >> rpc_execute
> >> rpc_set_active
> >> rpc_make_runnable
> >> (waiting)
> >> 			rpc_async_schedule
> >> 			nfs4_open_prepare
> >> 			nfs_wait_on_sequence
> >> 						nfs_umount_begin
> >> 						rpc_killall_tasks
> >> 						rpc_wake_up_task
> >> 						rpc_wake_up_queued_task
> >> 						spin_lock(tk_waitqueue == NULL)
> >> 						BUG()
> >> 			rpc_sleep_on
> >> 			spin_lock(&q->lock)
> >> 			__rpc_sleep_on
> >> 			task->tk_waitqueue = q
> >>
> >> Signed-off-by: Stanislav Kinsbursky<skinsbursky@openvz.org>
> >>
> >> ---
> >>   net/sunrpc/clnt.c |    4 +++-
> >>   1 files changed, 3 insertions(+), 1 deletions(-)
> >>
> >> diff --git a/net/sunrpc/clnt.c b/net/sunrpc/clnt.c
> >> index 57d344c..24039fe 100644
> >> --- a/net/sunrpc/clnt.c
> >> +++ b/net/sunrpc/clnt.c
> >> @@ -436,7 +436,9 @@ void rpc_killall_tasks(struct rpc_clnt *clnt)
> >>   		if (!(rovr->tk_flags&  RPC_TASK_KILLED)) {
> >>   			rovr->tk_flags |= RPC_TASK_KILLED;
> >>   			rpc_exit(rovr, -EIO);
> >> -			rpc_wake_up_queued_task(rovr->tk_waitqueue, rovr);
> >> +			if (rovr->tk_waitqueue)
> >> +				rpc_wake_up_queued_task(rovr->tk_waitqueue,
> >> +							rovr);
> >
> > Testing for RPC_IS_QUEUED(rovr) would be better, since that would
> > optimise away the call to rpc_wake_up_queued_task() altogether for those
> > tasks that aren't queued.
> >
> 
> Yes, I agree with testing RPC_IS_QUEUED(rovr) since such approach looks
> clearer and in 2.6.38 tk_waitqueue is initialized prior to set
> RPC_TASK_QUEUED bit.
> But I found this problem in 2.6.32 rhel kernel where this set sequence is inversed.
> Will send fixed version soon.

Are you sure? Why would the 2.6.32 rhel kernel differ from the mainline
2.6.32 kernel in this respect?
Stanislav Kinsbursky March 17, 2011, 5:04 p.m. UTC | #4 
17.03.2011 19:44, Trond Myklebust пишет:
>
> Are you sure? Why would the 2.6.32 rhel kernel differ from the mainline
> 2.6.32 kernel in this respect?
>

Checked again and realized, that was wrong.
This set sequence is the same in 2.6.32 rhel, 2.6.32 and 2.6.38 kernels.
diff mbox

Patch

diff --git a/net/sunrpc/clnt.c b/net/sunrpc/clnt.c
index 57d344c..24039fe 100644
--- a/net/sunrpc/clnt.c
+++ b/net/sunrpc/clnt.c
@@ -436,7 +436,9 @@  void rpc_killall_tasks(struct rpc_clnt *clnt)
 		if (!(rovr->tk_flags & RPC_TASK_KILLED)) {
 			rovr->tk_flags |= RPC_TASK_KILLED;
 			rpc_exit(rovr, -EIO);
-			rpc_wake_up_queued_task(rovr->tk_waitqueue, rovr);
+			if (rovr->tk_waitqueue)
+				rpc_wake_up_queued_task(rovr->tk_waitqueue, 
+							rovr);
 		}
 	}
 	spin_unlock(&clnt->cl_lock);