[PATCH] RPC: killing RPC tasks races fixed

Subsystems: kernel nfsd, sunrpc, and lockd servers, networking [general], nfs, sunrpc, and lockd clients, the rest

STALE5645d

5 messages, 2 authors, 2011-03-17 · open the first message on its own page

[PATCH] RPC: killing RPC tasks races fixed

From: Stanislav Kinsbursky <hidden>
Date: 2011-03-17 12:16:57

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 <redacted>

---
 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);
 		}
 	}
 	spin_unlock(&clnt->cl_lock);

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

From: Trond Myklebust <hidden>
Date: 2011-03-17 13:02:51

On Thu, 2011-03-17 at 15:16 +0300, Stanislav Kinsbursky wrote:

quoted hunk

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 <redacted>

---
 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);

-- 
Trond Myklebust
Linux NFS client maintainer

NetApp
Trond.Myklebust-HgOvQuBEEgTQT0dZR+AlfA@public.gmane.org
www.netapp.com

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

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

From: Stanislav Kinsbursky <hidden>
Date: 2011-03-17 15:43:58

17.03.2011 16:01, Trond Myklebust пишет:

On Thu, 2011-03-17 at 15:16 +0300, Stanislav Kinsbursky wrote:

quoted

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<redacted>

---
  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.

quoted

  		}
  	}
  	spin_unlock(&clnt->cl_lock);


-- 
Best regards,
Stanislav Kinsbursky

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

From: Trond Myklebust <hidden>
Date: 2011-03-17 16:44:21

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

17.03.2011 16:01, Trond Myklebust пишет:

quoted

On Thu, 2011-03-17 at 15:16 +0300, Stanislav Kinsbursky wrote:

quoted

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<redacted>

---
  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?

-- 
Trond Myklebust
Linux NFS client maintainer

NetApp
Trond.Myklebust-HgOvQuBEEgTQT0dZR+AlfA@public.gmane.org
www.netapp.com

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

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

From: Stanislav Kinsbursky <hidden>
Date: 2011-03-17 17:04:39

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.

-- 
Best regards,
Stanislav Kinsbursky

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help