Possible Race Condition on SIGKILL

Commit Message

Trond Myklebust Jan. 7, 2013, 8:29 p.m. UTC

On Mon, 2013-01-07 at 15:20 -0500, Chris Perl wrote:
> On Mon, Jan 07, 2013 at 07:50:10PM +0000, Myklebust, Trond wrote:
> > Hi Chris,
> > 
> > Excellent sleuthing! Given the thoroughness of your explanation, I'm
> > pretty sure that the attached patch should fix the problem.
> > 
> > Cheers
> >   Trond
> > -- 
> > Trond Myklebust
> > Linux NFS client maintainer
> > 
> > NetApp
> > [e-mail]
> > www.netapp.com
> 
> > From ec8cbb4aff21cd0eac2c6f3fc4273ac72cdd91ef Mon Sep 17 00:00:00 2001
> > From: Trond Myklebust <[e-mail]>
> > Date: Mon, 7 Jan 2013 14:30:46 -0500
> > Subject: [PATCH] SUNRPC: Ensure we release the socket write lock if the
> >  rpc_task exits early
> > 
> > If the rpc_task exits while holding the socket write lock before it has
> > allocated an rpc slot, then the usual mechanism for releasing the write
> > lock in xprt_release() is defeated.
> > 
> > The problem occurs if the call to xprt_lock_write() initially fails, so
> > that the rpc_task is put on the xprt->sending wait queue. If the task
> > exits after being assigned the lock by __xprt_lock_write_func, but
> > before it has retried the call to xprt_lock_and_alloc_slot(), then
> > it calls xprt_release() while holding the write lock, but will
> > immediately exit due to the test for task->tk_rqstp != NULL.
> > 
> > Reported-by: Chris Perl <[e-mail]>
> > Signed-off-by: Trond Myklebust <[e-mail]>
> > Cc: [e-mail] [>= 3.1]
> > ---
> >  net/sunrpc/xprt.c | 6 ++++--
> >  1 file changed, 4 insertions(+), 2 deletions(-)
> > 
> > diff --git a/net/sunrpc/xprt.c b/net/sunrpc/xprt.c
> > index bd462a5..6676457 100644
> > --- a/net/sunrpc/xprt.c
> > +++ b/net/sunrpc/xprt.c
> > @@ -1136,10 +1136,12 @@ static void xprt_request_init(struct rpc_task *task, struct rpc_xprt *xprt)
> >  void xprt_release(struct rpc_task *task)
> >  {
> >  	struct rpc_xprt	*xprt;
> > -	struct rpc_rqst	*req;
> > +	struct rpc_rqst	*req = task->tk_rqstp;
> >  
> > -	if (!(req = task->tk_rqstp))
> > +	if (req == NULL) {
> > +		xprt_release_write(task->tk_xprt, task);
> >  		return;
> > +	}
> >  
> >  	xprt = req->rq_xprt;
> >  	if (task->tk_ops->rpc_count_stats != NULL)
> > -- 
> > 1.7.11.7
> > 
> 
> Ah, I totally missed the call to `rpc_release_task' at the bottom of the
> `__rpc_execute' loop (at least thats how I think we'd get to this function
> you're patching).
> 
> But wouldn't we need to update the call site in
> `rpc_release_resources_task' as well?  It contains an explicit check for
> `task->tk_rqstp' being non null.

Ewww.... You're right: That's a wart that seems to have been copied and
pasted quite a few times.

Here is v2...

Patch

From 40b0ef28fda0b3ecf8e4f35846d9b9a765df4c41 Mon Sep 17 00:00:00 2001
From: Trond Myklebust <[e-mail]>
Date: Mon, 7 Jan 2013 14:30:46 -0500
Subject: [PATCH v2] SUNRPC: Ensure we release the socket write lock if the
 rpc_task exits early

If the rpc_task exits while holding the socket write lock before it has
allocated an rpc slot, then the usual mechanism for releasing the write
lock in xprt_release() is defeated.

The problem occurs if the call to xprt_lock_write() initially fails, so
that the rpc_task is put on the xprt->sending wait queue. If the task
exits after being assigned the lock by __xprt_lock_write_func, but
before it has retried the call to xprt_lock_and_alloc_slot(), then
it calls xprt_release() while holding the write lock, but will
immediately exit due to the test for task->tk_rqstp != NULL.

Reported-by: Chris Perl <[e-mail]>
Signed-off-by: Trond Myklebust <[e-mail]>
Cc: [e-mail] [>= 3.1]
---
 net/sunrpc/sched.c | 3 +--
 net/sunrpc/xprt.c  | 6 ++++--
 2 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/net/sunrpc/sched.c b/net/sunrpc/sched.c
index b4133bd..bfa3171 100644
--- a/net/sunrpc/sched.c
+++ b/net/sunrpc/sched.c
@@ -972,8 +972,7 @@  static void rpc_async_release(struct work_struct *work)
 
 static void rpc_release_resources_task(struct rpc_task *task)
 {
-	if (task->tk_rqstp)
-		xprt_release(task);
+	xprt_release(task);
 	if (task->tk_msg.rpc_cred) {
 		put_rpccred(task->tk_msg.rpc_cred);
 		task->tk_msg.rpc_cred = NULL;
diff --git a/net/sunrpc/xprt.c b/net/sunrpc/xprt.c
index bd462a5..6676457 100644
--- a/net/sunrpc/xprt.c
+++ b/net/sunrpc/xprt.c
@@ -1136,10 +1136,12 @@  static void xprt_request_init(struct rpc_task *task, struct rpc_xprt *xprt)
 void xprt_release(struct rpc_task *task)
 {
 	struct rpc_xprt	*xprt;
-	struct rpc_rqst	*req;
+	struct rpc_rqst	*req = task->tk_rqstp;
 
-	if (!(req = task->tk_rqstp))
+	if (req == NULL) {
+		xprt_release_write(task->tk_xprt, task);
 		return;
+	}
 
 	xprt = req->rq_xprt;
 	if (task->tk_ops->rpc_count_stats != NULL)
-- 
1.7.11.7