Race condition in xprt_disconnect
Subsystems (3)
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
Signature · 3 lines
Olaf Kirch | The Hardware Gods hate me. [email protected] | ---------------+
net/sunrpc/xprt.c renamed from net/sunrpc/xprt.c.fix +42 -22
@@ -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; }/*