All of lore.kernel.org
 help / color / mirror / Atom feed
* RE: Race condition in xprt_disconnect
@ 2004-04-05 20:39 Trond Myklebust
  2004-04-06  9:24 ` Olaf Kirch
  0 siblings, 1 reply; 12+ messages in thread
From: Trond Myklebust @ 2004-04-05 20:39 UTC (permalink / raw)
  To: Olaf Kirch; +Cc: nfs

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

        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.

I initially thought that patch you sent out was correct, however the
more I think about it, the more I'm convinced it isn't sufficient.

The only way I see that we can have duplicate calls to sock_release() is
if we are scheduling xprt_socket_autoclose() or xprt_socket_connect()
more than once.
Normally xprt_lock_write() should protect us against this, and so AFAICS
any duplicates must imply that xprt_lock_write() is getting lost before
xprt_socket_connect() has had a chance to run. Presumably this is
occurring because xprt->snd_task is timing out and then being woken up
from xprt->pending.
Note that in that case we don't actually *want* to schedule a new
connect(), since the problem is not that the networking operation timed
out, but rather that keventd is being too slow...

The following patch should hopefully protect against this problem.

Cheers,
  Trond



[-- Attachment #2: linux-2.6.5-34-sunrpc_disconnect.dif --]
[-- Type: text/plain, Size: 4253 bytes --]

 include/linux/sunrpc/xprt.h |    5 ++-
 net/sunrpc/xprt.c           |   58 ++++++++++++++++++--------------------------
 2 files changed, 27 insertions(+), 36 deletions(-)

diff -u --recursive --new-file --show-c-function linux-2.6.5-33-grace/include/linux/sunrpc/xprt.h linux-2.6.5-34-sunrpc_disconnect/include/linux/sunrpc/xprt.h
--- linux-2.6.5-33-grace/include/linux/sunrpc/xprt.h	2004-04-05 13:24:48.000000000 -0400
+++ linux-2.6.5-34-sunrpc_disconnect/include/linux/sunrpc/xprt.h	2004-04-05 14:49:48.000000000 -0400
@@ -216,8 +216,9 @@ void			xprt_connect(struct rpc_task *);
 int			xprt_clear_backlog(struct rpc_xprt *);
 void			xprt_sock_setbufsize(struct rpc_xprt *);
 
-#define XPRT_CONNECT	0
-#define XPRT_LOCKED	1
+#define XPRT_LOCKED	0
+#define XPRT_CONNECT	1
+#define XPRT_CONNECTING	2
 
 #define xprt_connected(xp)		(test_bit(XPRT_CONNECT, &(xp)->sockstate))
 #define xprt_set_connected(xp)		(set_bit(XPRT_CONNECT, &(xp)->sockstate))
diff -u --recursive --new-file --show-c-function linux-2.6.5-33-grace/net/sunrpc/xprt.c linux-2.6.5-34-sunrpc_disconnect/net/sunrpc/xprt.c
--- linux-2.6.5-33-grace/net/sunrpc/xprt.c	2004-04-05 13:24:58.000000000 -0400
+++ linux-2.6.5-34-sunrpc_disconnect/net/sunrpc/xprt.c	2004-04-05 15:27:42.000000000 -0400
@@ -448,7 +448,10 @@ xprt_init_autodisconnect(unsigned long d
 		goto out_abort;
 	spin_unlock(&xprt->sock_lock);
 	/* Let keventd close the socket */
-	schedule_work(&xprt->task_cleanup);
+	if (test_bit(XPRT_CONNECTING, &xprt->sockstate) != 0)
+		xprt_release_write(xprt, NULL);
+	else
+		schedule_work(&xprt->task_cleanup);
 	return;
 out_abort:
 	spin_unlock(&xprt->sock_lock);
@@ -460,12 +463,8 @@ static void xprt_socket_connect(void *ar
 	struct socket *sock = xprt->sock;
 	int status = -EIO;
 
-	if (xprt->shutdown) {
-		rpc_wake_up_status(&xprt->pending, -EIO);
-		return;
-	}
-	if (!xprt->addr.sin_port)
-		goto out_err;
+	if (xprt->shutdown || xprt->addr.sin_port == 0)
+		goto out;
 
 	/*
 	 * Start by resetting any existing state
@@ -475,12 +474,12 @@ static void xprt_socket_connect(void *ar
 	if (sock == NULL) {
 		/* couldn't create socket or bind to reserved port;
 		 * this is likely a permanent error, so cause an abort */
-		goto out_err;
-		return;
+		goto out;
 	}
 	xprt_bind_socket(xprt, sock);
 	xprt_sock_setbufsize(xprt);
 
+	status = 0;
 	if (!xprt->stream)
 		goto out;
 
@@ -491,29 +490,22 @@ static void xprt_socket_connect(void *ar
 			sizeof(xprt->addr), O_NONBLOCK);
 	dprintk("RPC: %p  connect status %d connected %d sock state %d\n",
 			xprt, -status, xprt_connected(xprt), sock->sk->sk_state);
-	if (status >= 0)
-		goto out;
-	switch (status) {
-		case -EINPROGRESS:
-		case -EALREADY:
-			return;
-		default:
-			goto out_err;
+	if (status < 0) {
+		switch (status) {
+			case -EINPROGRESS:
+			case -EALREADY:
+				goto out_clear;
+		}
 	}
 out:
-	spin_lock_bh(&xprt->sock_lock);
-	if (xprt->snd_task)
-		rpc_wake_up_task(xprt->snd_task);
-	spin_unlock_bh(&xprt->sock_lock);
-	return;
-out_err:
-	spin_lock_bh(&xprt->sock_lock);
-	if (xprt->snd_task) {
-		xprt->snd_task->tk_status = status;
-		rpc_wake_up_task(xprt->snd_task);
-	} else
-		rpc_wake_up_status(&xprt->pending, -ENOTCONN);
-	spin_unlock_bh(&xprt->sock_lock);
+	if (status < 0)
+		rpc_wake_up_status(&xprt->pending, status);
+	else
+		rpc_wake_up(&xprt->pending);
+out_clear:
+	smp_mb__before_clear_bit();
+	clear_bit(XPRT_CONNECTING, &xprt->sockstate);
+	smp_mb__after_clear_bit();
 }
 
 /*
@@ -545,7 +537,8 @@ void xprt_connect(struct rpc_task *task)
 
 	task->tk_timeout = RPC_CONNECT_TIMEOUT;
 	rpc_sleep_on(&xprt->pending, task, xprt_connect_status, NULL);
-	schedule_work(&xprt->sock_connect);
+	if (!test_and_set_bit(XPRT_CONNECTING, &xprt->sockstate))
+		schedule_work(&xprt->sock_connect);
 	return;
  out_write:
 	xprt_release_write(xprt, task);
@@ -1027,9 +1020,6 @@ tcp_state_change(struct sock *sk)
 			xprt->tcp_reclen = 0;
 			xprt->tcp_copied = 0;
 			xprt->tcp_flags = XPRT_COPY_RECM | XPRT_COPY_XID;
-
-			if (xprt->snd_task)
-				rpc_wake_up_task(xprt->snd_task);
 			rpc_wake_up(&xprt->pending);
 		}
 		spin_unlock_bh(&xprt->sock_lock);

^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2004-04-06 20:06 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2004-04-05 20:39 Race condition in xprt_disconnect Trond Myklebust
2004-04-06  9:24 ` Olaf Kirch
2004-04-06 13:51   ` Trond Myklebust
2004-04-06 14:11   ` Trond Myklebust
2004-04-06 14:26     ` Olaf Kirch
2004-04-06 14:36       ` Trond Myklebust
2004-04-06 14:54         ` Trond Myklebust
2004-04-06 15:23           ` Olaf Kirch
2004-04-06 16:02             ` Trond Myklebust
2004-04-06 16:17               ` Olaf Kirch
2004-04-06 15:27       ` Mounting any other os besides RHEW 3 doesn't work Brent M. Clements
2004-04-06 20:05         ` Steve Dickson

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.