Race condition in xprt_disconnect

[Olaf Kirch <okir@suse.de>]

[-- Attachment #1: Type: text/plain, Size: 737 bytes --]

Hi,

it seems bad things can happen if two RPC processes call xprt_disconnect
at the same time. I have two bug reports of ppc machines oopsing
somewhere deep in some SELinux functions because inode->i_security
is NULL. inode->i_security is allocated when the inode is created,
and cleared when the inode is destroyed.

The stack looks something like 
xprt_disconnect->sock_release->...->selinux_foofah

The only way I can see this happening is if sock_release is called twice,
and indeed we do not seem to protect against this. Please take a look
at the attached patch. It should prevent these oopses, but I'm not
entirely sure it's safe.

Thanks,
Olaf
-- 
Olaf Kirch     |  The Hardware Gods hate me.
okir@suse.de   |
---------------+ 

[-- Attachment #2: sunrpc-disconnect-race --]
[-- Type: text/plain, Size: 2849 bytes --]

--- linux-2.6.4/net/sunrpc/xprt.c.fix	2004-04-01 18:51:50.000000000 +0200
+++ linux-2.6.4/net/sunrpc/xprt.c	2004-04-01 18:51:03.000000000 +0200
@@ -87,7 +87,7 @@
 static struct rpc_xprt * xprt_setup(int proto, struct sockaddr_in *ap,
 						struct rpc_timeout *to);
 static struct socket *xprt_create_socket(struct rpc_xprt *, int, int);
-static void	xprt_bind_socket(struct rpc_xprt *, struct socket *);
+static int	xprt_bind_socket(struct rpc_xprt *, struct socket *);
 static int      __xprt_get_cong(struct rpc_xprt *, struct rpc_task *);
 
 #ifdef RPC_DEBUG_DATA
@@ -389,26 +389,33 @@
 static void
 xprt_close(struct rpc_xprt *xprt, int reconnecting)
 {
-	struct socket	*sock = xprt->sock;
-	struct sock	*sk = xprt->inet;
-
-	if (!sk)
-		return;
-
-	write_lock_bh(&sk->sk_callback_lock);
+	struct socket	*sock;
+	struct sock	*sk;
+	
+	spin_lock_bh(&xprt->sock_lock);
+	if ((sk = xprt->inet) != NULL)
+		sock_hold(sk);
 	xprt->inet = NULL;
+
+	sock = xprt->sock;
 	xprt->sock = NULL;
+	spin_unlock_bh(&xprt->sock_lock);
 
-	sk->sk_user_data    = NULL;
-	sk->sk_data_ready   = xprt->old_data_ready;
-	sk->sk_state_change = xprt->old_state_change;
-	sk->sk_write_space  = xprt->old_write_space;
-	write_unlock_bh(&sk->sk_callback_lock);
+	if (sk != NULL) {
+		write_lock_bh(&sk->sk_callback_lock);
+		sk->sk_user_data    = NULL;
+		sk->sk_data_ready   = xprt->old_data_ready;
+		sk->sk_state_change = xprt->old_state_change;
+		sk->sk_write_space  = xprt->old_write_space;
+		sk->sk_no_check	 = 0;
+		write_unlock_bh(&sk->sk_callback_lock);
+		sock_put(sk);
+	}
 
 	xprt_disconnect(xprt, reconnecting);
-	sk->sk_no_check	 = 0;
 
-	sock_release(sock);
+	if (sock)
+		sock_release(sock);
 }
 
 static void
@@ -481,7 +488,10 @@
 		goto out_err;
 		return;
 	}
-	xprt_bind_socket(xprt, sock);
+	if (xprt_bind_socket(xprt, sock) < 0) {
+		sock_release(sock);
+		goto out_err;
+	}
 	xprt_sock_setbufsize(xprt);
 
 	if (!xprt->stream)
@@ -1520,13 +1530,17 @@
 	return err;
 }
 
-static void
+static int
 xprt_bind_socket(struct rpc_xprt *xprt, struct socket *sock)
 {
 	struct sock	*sk = sock->sk;
+	int		error = -EBUSY;
 
+	/* Can it happen that xprt->inet is non-null when we
+	 * get here? If so we should indicate an error so
+	 * that the caller can drop the newly created socket */
 	if (xprt->inet)
-		return;
+		goto out;
 
 	write_lock_bh(&sk->sk_callback_lock);
 	sk->sk_user_data = xprt;
@@ -1545,13 +1559,19 @@
 		xprt_clear_connected(xprt);
 	}
 	sk->sk_write_space = xprt_write_space;
+	write_unlock_bh(&sk->sk_callback_lock);
 
 	/* Reset to new socket */
-	xprt->sock = sock;
-	xprt->inet = sk;
-	write_unlock_bh(&sk->sk_callback_lock);
+	spin_lock_bh(&xprt->sock_lock);
+	if (xprt->inet == NULL) {
+		xprt->sock = sock;
+		xprt->inet = sk;
+		error = 0;
+	}
+	spin_unlock_bh(&xprt->sock_lock);
 
-	return;
+out:
+	return error;
 }
 
 /*