[06/11] ocfs2/o2net: o2net_listen_data_ready should do nothing if socket state is not TCP_LISTEN
diff mbox

Message ID 20140124204705.E43E65A4203@corp2gmr1-2.hot.corp.google.com
State New, archived
Headers show

Commit Message

Andrew Morton Jan. 24, 2014, 8:47 p.m. UTC
From: Tariq Saeed <tariq.x.saeed@oracle.com>
Subject: ocfs2/o2net: o2net_listen_data_ready should do nothing if socket state is not TCP_LISTEN

Orabug: 17330860

When accepting an incomming connection o2net_accept_one clones a child
data socket from the parent listening socket.  It then proceeds to setup
the child with callback o2net_data_ready() and sk_user_data to NULL.  If
data arrives in this window, o2net_listen_data_ready will be called with
some non-deterministic value in sk_user_data (not inherited).  We panic
when we page fault on sk_user_data -- in parent it is sock_def_readable().
 The fix is to recognize that this is a data socket being set up by
looking at the socket state and do nothing.

Signed-off-by: Tariq Saseed <tariq.x.saeed@oracle.com>
Signed-off-by: Srinivas Eeda <srinivas.eeda@oracle.com>
Cc: Joel Becker <jlbec@evilplan.org>
Cc: Mark Fasheh <mfasheh@suse.com>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
---

 fs/ocfs2/cluster/tcp.c |   22 +++++++++++++++++-----
 1 file changed, 17 insertions(+), 5 deletions(-)

Comments

Mark Fasheh Feb. 5, 2014, 11:40 p.m. UTC | #1
On Fri, Jan 24, 2014 at 12:47:05PM -0800, akpm@linux-foundation.org wrote:
> From: Tariq Saeed <tariq.x.saeed@oracle.com>
> Subject: ocfs2/o2net: o2net_listen_data_ready should do nothing if socket state is not TCP_LISTEN
> 
> Orabug: 17330860
> 
> When accepting an incomming connection o2net_accept_one clones a child
> data socket from the parent listening socket.  It then proceeds to setup
> the child with callback o2net_data_ready() and sk_user_data to NULL.  If
> data arrives in this window, o2net_listen_data_ready will be called with
> some non-deterministic value in sk_user_data (not inherited).  We panic
> when we page fault on sk_user_data -- in parent it is sock_def_readable().
>  The fix is to recognize that this is a data socket being set up by
> looking at the socket state and do nothing.
> 
> Signed-off-by: Tariq Saseed <tariq.x.saeed@oracle.com>
> Signed-off-by: Srinivas Eeda <srinivas.eeda@oracle.com>
> Cc: Joel Becker <jlbec@evilplan.org>
> Cc: Mark Fasheh <mfasheh@suse.com>
> Signed-off-by: Andrew Morton <akpm@linux-foundation.org>

Signed-off-by: Mark Fasheh <mfasheh@suse.de>

Thank you for this Tariq,
	--Mark

--
Mark Fasheh
Andrew Morton Feb. 5, 2014, 11:51 p.m. UTC | #2
On Wed, 5 Feb 2014 15:40:55 -0800 Mark Fasheh <mfasheh@suse.de> wrote:

> On Fri, Jan 24, 2014 at 12:47:05PM -0800, akpm@linux-foundation.org wrote:
> > From: Tariq Saeed <tariq.x.saeed@oracle.com>
> > Subject: ocfs2/o2net: o2net_listen_data_ready should do nothing if socket state is not TCP_LISTEN
> > 
> > Orabug: 17330860
> > 
> > When accepting an incomming connection o2net_accept_one clones a child
> > data socket from the parent listening socket.  It then proceeds to setup
> > the child with callback o2net_data_ready() and sk_user_data to NULL.  If
> > data arrives in this window, o2net_listen_data_ready will be called with
> > some non-deterministic value in sk_user_data (not inherited).  We panic
> > when we page fault on sk_user_data -- in parent it is sock_def_readable().
> >  The fix is to recognize that this is a data socket being set up by
> > looking at the socket state and do nothing.
> > 
> > Signed-off-by: Tariq Saseed <tariq.x.saeed@oracle.com>
> > Signed-off-by: Srinivas Eeda <srinivas.eeda@oracle.com>
> > Cc: Joel Becker <jlbec@evilplan.org>
> > Cc: Mark Fasheh <mfasheh@suse.com>
> > Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
> 
> Signed-off-by: Mark Fasheh <mfasheh@suse.de>
> 

Signed-off-by is unexpected.  It means that your either contributed to
this patch or transmitted it to someone else.  I suspect that
Reviewed-by: is the appropriate tag in this case?
Mark Fasheh Feb. 6, 2014, 11:36 p.m. UTC | #3
On Wed, Feb 05, 2014 at 03:51:04PM -0800, Andrew Morton wrote:
> On Wed, 5 Feb 2014 15:40:55 -0800 Mark Fasheh <mfasheh@suse.de> wrote:
> 
> > On Fri, Jan 24, 2014 at 12:47:05PM -0800, akpm@linux-foundation.org wrote:
> > > From: Tariq Saeed <tariq.x.saeed@oracle.com>
> > > Subject: ocfs2/o2net: o2net_listen_data_ready should do nothing if socket state is not TCP_LISTEN
> > > 
> > > Orabug: 17330860
> > > 
> > > When accepting an incomming connection o2net_accept_one clones a child
> > > data socket from the parent listening socket.  It then proceeds to setup
> > > the child with callback o2net_data_ready() and sk_user_data to NULL.  If
> > > data arrives in this window, o2net_listen_data_ready will be called with
> > > some non-deterministic value in sk_user_data (not inherited).  We panic
> > > when we page fault on sk_user_data -- in parent it is sock_def_readable().
> > >  The fix is to recognize that this is a data socket being set up by
> > > looking at the socket state and do nothing.
> > > 
> > > Signed-off-by: Tariq Saseed <tariq.x.saeed@oracle.com>
> > > Signed-off-by: Srinivas Eeda <srinivas.eeda@oracle.com>
> > > Cc: Joel Becker <jlbec@evilplan.org>
> > > Cc: Mark Fasheh <mfasheh@suse.com>
> > > Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
> > 
> > Signed-off-by: Mark Fasheh <mfasheh@suse.de>
> > 
> 
> Signed-off-by is unexpected.  It means that your either contributed to
> this patch or transmitted it to someone else.  I suspect that
> Reviewed-by: is the appropriate tag in this case?

Yes, sorry bad habit I need to fix.

Reviewed-by: Mark Fasheh <mfasheh@suse.de>

would be fine. I'll use Reviewed-by: for when I review a patch from now on.
	--Mark

--
Mark Fasheh

Patch
diff mbox

diff -puN fs/ocfs2/cluster/tcp.c~ocfs2-o2net-o2net_listen_data_ready-should-do-nothing-if-socket-state-is-not-tcp_listen fs/ocfs2/cluster/tcp.c
--- a/fs/ocfs2/cluster/tcp.c~ocfs2-o2net-o2net_listen_data_ready-should-do-nothing-if-socket-state-is-not-tcp_listen
+++ a/fs/ocfs2/cluster/tcp.c
@@ -1973,18 +1973,30 @@  static void o2net_listen_data_ready(stru
 		goto out;
 	}
 
-	/* ->sk_data_ready is also called for a newly established child socket
-	 * before it has been accepted and the acceptor has set up their
-	 * data_ready.. we only want to queue listen work for our listening
-	 * socket */
+	/* This callback may called twice when a new connection
+	 * is  being established as a child socket inherits everything
+	 * from a parent LISTEN socket, including the data_ready cb of
+	 * the parent. This leads to a hazard. In o2net_accept_one()
+	 * we are still initializing the child socket but have not
+	 * changed the inherited data_ready callback yet when
+	 * data starts arriving.
+	 * We avoid this hazard by checking the state.
+	 * For the listening socket,  the state will be TCP_LISTEN; for the new
+	 * socket, will be  TCP_ESTABLISHED. Also, in this case,
+	 * sk->sk_user_data is not a valid function pointer.
+	 */
+
 	if (sk->sk_state == TCP_LISTEN) {
 		mlog(ML_TCP, "bytes: %d\n", bytes);
 		queue_work(o2net_wq, &o2net_listen_work);
+	} else {
+		ready = NULL;
 	}
 
 out:
 	read_unlock(&sk->sk_callback_lock);
-	ready(sk, bytes);
+	if (ready != NULL)
+		ready(sk, bytes);
 }
 
 static int o2net_open_listening_sock(__be32 addr, __be16 port)