SUNRPC: Invalid BUG_ON() popping in rpc_release_task()

PATCH In review Steve Dickson 2 participants 3 messages Jan 21, 2007

Subsystems (3)

Networking NFS client NFS server

lore

Share & tools

lore

Steve Dickson

author

2007-01-21 11:53 UTC #0 lore raw

PATCH 1/1 SUNRPC: Invalid BUG_ON() popping in rpc_release_task()

Over the weekend, while doing some Secure NFS testing, I found that the BUG_ON() in rpc_release_task() would pop when the Kerberos server was not set up correctly (which I know is a very common problem when people start using this feature).

The code path looks like:
rpc_call_sync() calls rpc_call_setup() which calls rpcauth_bindcred() which fails (due to a krb5 problem) and sets tk_status to -EACCES.

Back in rpc_call_sync(), since (tk_status != 0) rpc_release_task() is called. Unfortunately, since this task was never active, (meaning rpc_execute() and rpc_set_active() were never called) the tk_magic id was never set and the global task list was never set up. So when rpc_release_task() is call, the world come tumbling down...

So the attached patch ensures the RPC_TASK_ACTIVE run state bit is set before calling the BUG_ON() and cleaning up the task list.

steved.

Only check the magic number and clean the tk_task queue iff the task was active.

Signed-off-by Steve Dickson <[email protected]>

-------------------------------------------------

net/sunrpc/sched.c renamed from net/sunrpc/sched.c.orig +9 -7

@@ -898,16 +898,18 @@ EXPORT_SYMBOL(rpc_put_task);
 
 void rpc_release_task(struct rpc_task *task)
 {
-#ifdef RPC_DEBUG
-	BUG_ON(task->tk_magic != RPC_TASK_MAGIC_ID);
-#endif
 	dprintk("RPC: %4d release task\n", task->tk_pid);
 
-	/* Remove from global task list */
-	spin_lock(&rpc_sched_lock);
-	list_del(&task->tk_task);
-	spin_unlock(&rpc_sched_lock);
+	if (test_bit(RPC_TASK_ACTIVE, &task->tk_runstate)) {
+#ifdef RPC_DEBUG
+		BUG_ON(task->tk_magic != RPC_TASK_MAGIC_ID);
+#endif
 
+		/* Remove from global task list */
+		spin_lock(&rpc_sched_lock);
+		list_del(&task->tk_task);
+		spin_unlock(&rpc_sched_lock);
+	}
 	BUG_ON (RPC_IS_QUEUED(task));
 
 	/* Synchronously delete any running timer */

-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your opinions on IT & business topics through brief surveys - and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV

_______________________________________________
NFS maillist - [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs

Trond Myklebust

2007-01-21 17:25 UTC #1 lore raw

Quoted 16 lines · Steve Dickson

Over the weekend, while doing some Secure NFS testing, I found that the BUG_ON() in rpc_release_task() would pop when the Kerberos server was not set up correctly (which I know is a very common problem when people start using this feature).
The code path looks like:
rpc_call_sync() calls rpc_call_setup() which calls rpcauth_bindcred() which fails (due to a krb5 problem) and sets tk_status to -EACCES.
Back in rpc_call_sync(), since (tk_status != 0) rpc_release_task() is called. Unfortunately, since this task was never active, (meaning rpc_execute() and rpc_set_active() were never called) the tk_magic id was never set and the global task list was never set up. So when rpc_release_task() is call, the world come tumbling down...

Quoted 6 lines

So the attached patch ensures the RPC_TASK_ACTIVE run state bit is set before calling the BUG_ON() and cleaning up the task list.
steved.

I've already posted the attach patch to fix this issue.

Cheers,
Trond

We shouldn't be calling rpc_release_task() for tasks that are not active.

Signed-off-by Trond Myklebust <[email protected]>

---

 include/linux/sunrpc/sched.h |    1 -
 net/sunrpc/clnt.c            |    8 +++-----
 net/sunrpc/sched.c           |    3 ++-
 3 files changed, 5 insertions(+), 7 deletions(-)

include/linux/sunrpc/sched.h +0 -1

index 97c7616..8b6ce60 100644
@@ -250,7 +250,6 @@ void		rpc_init_task(struct rpc_task *tas
 				int flags, const struct rpc_call_ops *ops,
 				void *data);
 void		rpc_put_task(struct rpc_task *);
-void		rpc_release_task(struct rpc_task *);
 void		rpc_exit_task(struct rpc_task *);
 void		rpc_release_calldata(const struct rpc_call_ops *, void *);
 void		rpc_killall_tasks(struct rpc_clnt *);

net/sunrpc/clnt.c +3 -5

index aba528b..16c9fbc 100644
@@ -490,16 +490,14 @@ int rpc_call_sync(struct rpc_clnt *clnt,
 
 	/* Set up the call info struct and execute the task */
 	status = task->tk_status;
-	if (status != 0) {
-		rpc_release_task(task);
+	if (status != 0)
 		goto out;
-	}
 	atomic_inc(&task->tk_count);
 	status = rpc_execute(task);
 	if (status == 0)
 		status = task->tk_status;
-	rpc_put_task(task);
 out:
+	rpc_put_task(task);
 	rpc_restore_sigmask(&oldset);
 	return status;
 }

@@ -537,7 +535,7 @@ rpc_call_async(struct rpc_clnt *clnt, st
 	if (status == 0)
 		rpc_execute(task);
 	else
-		rpc_release_task(task);
+		rpc_put_task(task);
 
 	rpc_restore_sigmask(&oldset);		
 	return status;

net/sunrpc/sched.c +2 -1

index 79bc4cd..fc083f0 100644
@@ -42,6 +42,7 @@ static mempool_t	*rpc_buffer_mempool __r
 static void			__rpc_default_timer(struct rpc_task *task);
 static void			rpciod_killall(void);
 static void			rpc_async_schedule(struct work_struct *);
+static void			 rpc_release_task(struct rpc_task *task);
 
 /*
  * RPC tasks sit here while waiting for conditions to improve.

@@ -896,7 +897,7 @@ void rpc_put_task(struct rpc_task *task)
 }
 EXPORT_SYMBOL(rpc_put_task);
 
-void rpc_release_task(struct rpc_task *task)
+static void rpc_release_task(struct rpc_task *task)
 {
 #ifdef RPC_DEBUG
 	BUG_ON(task->tk_magic != RPC_TASK_MAGIC_ID);

_______________________________________________
NFS maillist - [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs

Steve Dickson

author

2007-01-21 19:59 UTC #2 lore raw

Quoted 1 line · Trond Myklebust

I've already posted the attach patch to fix this issue.

great! thanks!

steved.