Re: [PATCH] Fix race corrupting rpc upcall list

[Trond Myklebust <Trond.Myklebust@netapp.com>]

On Tue, 2010-09-07 at 01:12 -0400, J. Bruce Fields wrote:
> From: J. Bruce Fields <bfields@redhat.com>
> 
> If rpc_queue_upcall() adds a new upcall to the rpci->pipe list just
> after rpc_pipe_release calls rpc_purge_list(), but before it calls
> gss_pipe_release (as rpci->ops->release_pipe(inode)), then the latter
> will free a message without deleting it from the rpci->pipe list.
> 
> We will be left with a freed object on the rpc->pipe list.  Most
> frequent symptoms are kernel crashes in rpc.gssd system calls on the
> pipe in question.
> 
> We could just add a list_del(&gss_msg->msg.list) here.  But I can see no
> reason for doing all this cleanup here; the preceding rpc_purge_list()
> should have done the job, except possibly for any newly queued upcalls
> as above, which can safely be left to wait for another opener.

Hi Bruce,

Looking again at this issue, I think I see why the ->release_pipe() is
needed. While the call to rpc_purge_list() does indeed clear the list of
all those messages that are waiting for their upcall to complete, it
does nothing for the messages that have successfully been read by the
daemon, but are now waiting for a reply.

How about something like the patch below instead?

Cheers
  Trond

-------------------------------------------------------------------------------------------
SUNRPC: Fix race corrupting rpc upcall

From: Trond Myklebust <Trond.Myklebust@netapp.com>

If rpc_queue_upcall() adds a new upcall to the rpci->pipe list just
after rpc_pipe_release calls rpc_purge_list(), but before it calls
gss_pipe_release (as rpci->ops->release_pipe(inode)), then the latter
will free a message without deleting it from the rpci->pipe list.

We will be left with a freed object on the rpc->pipe list.  Most
frequent symptoms are kernel crashes in rpc.gssd system calls on the
pipe in question.

Reported-by: J. Bruce Fields <bfields@redhat.com>
Signed-off-by: Trond Myklebust <Trond.Myklebust@netapp.com>
---

 net/sunrpc/auth_gss/auth_gss.c |    9 +++++----
 net/sunrpc/rpc_pipe.c          |    6 +++---
 2 files changed, 8 insertions(+), 7 deletions(-)


diff --git a/net/sunrpc/auth_gss/auth_gss.c b/net/sunrpc/auth_gss/auth_gss.c
index dcfc66b..12c4859 100644
--- a/net/sunrpc/auth_gss/auth_gss.c
+++ b/net/sunrpc/auth_gss/auth_gss.c
@@ -745,17 +745,18 @@ gss_pipe_release(struct inode *inode)
 	struct rpc_inode *rpci = RPC_I(inode);
 	struct gss_upcall_msg *gss_msg;
 
+restart:
 	spin_lock(&inode->i_lock);
-	while (!list_empty(&rpci->in_downcall)) {
+	list_for_each_entry(gss_msg, &rpci->in_downcall, list) {
 
-		gss_msg = list_entry(rpci->in_downcall.next,
-				struct gss_upcall_msg, list);
+		if (!list_empty(&gss_msg->msg.list))
+			continue;
 		gss_msg->msg.errno = -EPIPE;
 		atomic_inc(&gss_msg->count);
 		__gss_unhash_msg(gss_msg);
 		spin_unlock(&inode->i_lock);
 		gss_release_msg(gss_msg);
-		spin_lock(&inode->i_lock);
+		goto restart;
 	}
 	spin_unlock(&inode->i_lock);
 
diff --git a/net/sunrpc/rpc_pipe.c b/net/sunrpc/rpc_pipe.c
index 95ccbcf..41a762f 100644
--- a/net/sunrpc/rpc_pipe.c
+++ b/net/sunrpc/rpc_pipe.c
@@ -48,7 +48,7 @@ static void rpc_purge_list(struct rpc_inode *rpci, struct list_head *head,
 		return;
 	do {
 		msg = list_entry(head->next, struct rpc_pipe_msg, list);
-		list_del(&msg->list);
+		list_del_init(&msg->list);
 		msg->errno = err;
 		destroy_msg(msg);
 	} while (!list_empty(head));
@@ -208,7 +208,7 @@ rpc_pipe_release(struct inode *inode, struct file *filp)
 	if (msg != NULL) {
 		spin_lock(&inode->i_lock);
 		msg->errno = -EAGAIN;
-		list_del(&msg->list);
+		list_del_init(&msg->list);
 		spin_unlock(&inode->i_lock);
 		rpci->ops->destroy_msg(msg);
 	}
@@ -268,7 +268,7 @@ rpc_pipe_read(struct file *filp, char __user *buf, size_t len, loff_t *offset)
 	if (res < 0 || msg->len == msg->copied) {
 		filp->private_data = NULL;
 		spin_lock(&inode->i_lock);
-		list_del(&msg->list);
+		list_del_init(&msg->list);
 		spin_unlock(&inode->i_lock);
 		rpci->ops->destroy_msg(msg);
 	}