[Ocfs2-devel] [patch 03/11] ocfs2/o2net: incorrect to terminate accepting connections loop upon rejecting an invalid one

[Andrew Morton]

On Fri, 24 Jan 2014 14:22:57 -0800 tariq saeed <tariq.x.saeed at oracle.com> wrote:

> On 1/24/2014 1:55 PM, Mark Fasheh wrote:
> > On Fri, Jan 24, 2014 at 12:47:02PM -0800, akpm at linux-foundation.org wrote:
> >> From: Tariq Saeed <tariq.x.saeed at oracle.com>
> >> Subject: ocfs2/o2net: incorrect to terminate accepting connections loop upon rejecting an invalid one
> >>
> >> When o2net-accept-one() rejects an illegal connection, it terminates the
> >> loop picking up the remaining queued connections.  This fix will continue
> >> accepting connections till the queue is emtpy.
> >>
> >> Addresses Orabug 17489469.
> > Thanks for sending this, review comments below.
> >
> >
> >> diff -puN fs/ocfs2/cluster/tcp.c~ocfs2-o2net-incorrect-to-terminate-accepting-connections-loop-upon-rejecting-an-invalid-one fs/ocfs2/cluster/tcp.c
> >> --- a/fs/ocfs2/cluster/tcp.c~ocfs2-o2net-incorrect-to-terminate-accepting-connections-loop-upon-rejecting-an-invalid-one
> >> +++ a/fs/ocfs2/cluster/tcp.c
> >> @@ -1826,7 +1826,7 @@ int o2net_register_hb_callbacks(void)
> >>   
> >>   /* ------------------------------------------------------------ */
> >>   
> >> -static int o2net_accept_one(struct socket *sock)
> >> +static int o2net_accept_one(struct socket *sock, int *more)
> >>   {
> >>   	int ret, slen;
> >>   	struct sockaddr_in sin;
> >> @@ -1837,6 +1837,7 @@ static int o2net_accept_one(struct socke
> >>   	struct o2net_node *nn;
> >>   
> >>   	BUG_ON(sock == NULL);
> >> +	*more = 0;
> >>   	ret = sock_create_lite(sock->sk->sk_family, sock->sk->sk_type,
> >>   			       sock->sk->sk_protocol, &new_sock);
> >>   	if (ret)
> >> @@ -1848,6 +1849,7 @@ static int o2net_accept_one(struct socke
> >>   	if (ret < 0)
> >>   		goto out;
> >>   
> >> +	*more = 1;
> >>   	new_sock->sk->sk_allocation = GFP_ATOMIC;
> >>   
> >>   	ret = o2net_set_nodelay(new_sock);
> >> @@ -1949,8 +1951,15 @@ out:
> >>   static void o2net_accept_many(struct work_struct *work)
> >>   {
> >>   	struct socket *sock = o2net_listen_sock;
> >> -	while (o2net_accept_one(sock) == 0)
> >> +	int	more;
> >> +	int	err;
> >> +
> >> +	for (;;) {
> >> +		err = o2net_accept_one(sock, &more);
> >> +		if (!more)
> >> +			break;
> > We're throwing out 'err' here and trusting the variable 'more'. However, err
> > could be set and more would be 0 regardless of whether there actually are
> > more connections to be had. This makes more sense given when 'more' is set:
>
> Thanks for the comments.
> To understand the consequences of ignoring the err, we need to look at 
> what is going on.
> We get a softIRQ when a connection packet (tcp SYN). It is critical to 
> note that we may not
> get a softIRQ_for every connection s_ince connection packets can arrive
> back-to-back (as happened in this bug). So, one softIRQ could be 
> delivered for > 1 pending accept.
> _This is the KEY point. _
> 
> If we terminate the loop calling o2net_accept_one() upon seeing an 
> error, what happens
> to the rest of the connections in the queue. If no new connection 
> arrives for hours, no new softIRQ
> will be delivered, and the connections will just sit in the queue.

Please note that I had to edit your email to undo the top-posting so I
could reply to it.  Please don't top-post.

Mark, are you now OK with the patch as-is?


From: Tariq Saeed <tariq.x.saeed at oracle.com>
Subject: ocfs2/o2net: incorrect to terminate accepting connections loop upon rejecting an invalid one

When o2net-accept-one() rejects an illegal connection, it terminates the
loop picking up the remaining queued connections.  This fix will continue
accepting connections till the queue is emtpy.

Addresses Orabug 17489469.

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

 fs/ocfs2/cluster/tcp.c |   13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff -puN fs/ocfs2/cluster/tcp.c~ocfs2-o2net-incorrect-to-terminate-accepting-connections-loop-upon-rejecting-an-invalid-one fs/ocfs2/cluster/tcp.c
--- a/fs/ocfs2/cluster/tcp.c~ocfs2-o2net-incorrect-to-terminate-accepting-connections-loop-upon-rejecting-an-invalid-one
+++ a/fs/ocfs2/cluster/tcp.c
@@ -1826,7 +1826,7 @@ int o2net_register_hb_callbacks(void)
 
 /* ------------------------------------------------------------ */
 
-static int o2net_accept_one(struct socket *sock)
+static int o2net_accept_one(struct socket *sock, int *more)
 {
 	int ret, slen;
 	struct sockaddr_in sin;
@@ -1837,6 +1837,7 @@ static int o2net_accept_one(struct socke
 	struct o2net_node *nn;
 
 	BUG_ON(sock == NULL);
+	*more = 0;
 	ret = sock_create_lite(sock->sk->sk_family, sock->sk->sk_type,
 			       sock->sk->sk_protocol, &new_sock);
 	if (ret)
@@ -1848,6 +1849,7 @@ static int o2net_accept_one(struct socke
 	if (ret < 0)
 		goto out;
 
+	*more = 1;
 	new_sock->sk->sk_allocation = GFP_ATOMIC;
 
 	ret = o2net_set_nodelay(new_sock);
@@ -1949,8 +1951,15 @@ out:
 static void o2net_accept_many(struct work_struct *work)
 {
 	struct socket *sock = o2net_listen_sock;
-	while (o2net_accept_one(sock) == 0)
+	int	more;
+	int	err;
+
+	for (;;) {
+		err = o2net_accept_one(sock, &more);
+		if (!more)
+			break;
 		cond_resched();
+	}
 }
 
 static void o2net_listen_data_ready(struct sock *sk, int bytes)
_