Re: [PATCH v2] RPC: killing RPC tasks races fixed

Re: [PATCH v2] RPC: killing RPC tasks races fixed

[Peter Lojkin]

On Thu, 2011-03-17 at 15:54:23 2011-03-17 at 18:43 +0300, Stanislav Kinsbursky wrote:

> RPC task RPC_TASK_QUEUED bit is set must be checked before trying to wake up
> task rpc_killall_tasks() because task->tk_waitqueue can not be set (equal to
> NULL).
> Also, as Trond Myklebust mentioned, such approach (instead of checking
> tk_waitqueue to NULL) allows us to "optimise away the call to
> rpc_wake_up_queued_task() altogether for those
> tasks that aren't queued".

you mentioned earlier that you have found this problem in .32 kernel but your patch only works for newer kernels.
does the following patch is correct and needed for pre .36 kernels?

--- a/net/sunrpc/sched.c	2011-03-22 16:55:36.802000287 +0300
+++ b/net/sunrpc/sched.c	2011-03-22 17:01:13.858000242 +0300
@@ -939,7 +939,8 @@
 		if (!(rovr->tk_flags & RPC_TASK_KILLED)) {
 			rovr->tk_flags |= RPC_TASK_KILLED;
 			rpc_exit(rovr, -EIO);
-			rpc_wake_up_task(rovr);
+			if (RPC_IS_QUEUED(rovr))
+				rpc_wake_up_task(rovr);
 		}
 	}
 	spin_unlock(&clnt->cl_lock);

Re: [PATCH v2] RPC: killing RPC tasks races fixed

[Stanislav Kinsbursky]

22.03.2011 17:52, Peter Lojkin пишет:
> On Thu, 2011-03-17 at 15:54:23 2011-03-17 at 18:43 +0300, Stanislav Kinsbursky wrote:
>
>> RPC task RPC_TASK_QUEUED bit is set must be checked before trying to wake up
>> task rpc_killall_tasks() because task->tk_waitqueue can not be set (equal to
>> NULL).
>> Also, as Trond Myklebust mentioned, such approach (instead of checking
>> tk_waitqueue to NULL) allows us to "optimise away the call to
>> rpc_wake_up_queued_task() altogether for those
>> tasks that aren't queued".
>
> you mentioned earlier that you have found this problem in .32 kernel but your patch only works for newer kernels.
> does the following patch is correct and needed for pre .36 kernels?
>


Yes, it is. This bug was found in rhel6 kernel which is based on 2.6.32 vanilla.


> --- a/net/sunrpc/sched.c	2011-03-22 16:55:36.802000287 +0300
> +++ b/net/sunrpc/sched.c	2011-03-22 17:01:13.858000242 +0300
> @@ -939,7 +939,8 @@
>   		if (!(rovr->tk_flags&  RPC_TASK_KILLED)) {
>   			rovr->tk_flags |= RPC_TASK_KILLED;
>   			rpc_exit(rovr, -EIO);
> -			rpc_wake_up_task(rovr);
> +			if (RPC_IS_QUEUED(rovr))
> +				rpc_wake_up_task(rovr);
>   		}
>   	}
>   	spin_unlock(&clnt->cl_lock);
>


-- 
Best regards,
Stanislav Kinsbursky

[PATCH v2] RPC: killing RPC tasks races fixed

[Stanislav Kinsbursky]

RPC task RPC_TASK_QUEUED bit is set must be checked before trying to wake up
task rpc_killall_tasks() because task->tk_waitqueue can not be set (equal to
NULL).
Also, as Trond Myklebust mentioned, such approach (instead of checking
tk_waitqueue to NULL) allows us to "optimise away the call to
rpc_wake_up_queued_task() altogether for those
tasks that aren't queued".

Here is an example of dereferencing of tk_waitqueue equal to NULL:

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..3444757 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 (RPC_IS_QUEUED(rovr))
+				rpc_wake_up_queued_task(rovr->tk_waitqueue, 
+							rovr);
 		}
 	}
 	spin_unlock(&clnt->cl_lock);