ocfs2/o2net: incorrect to terminate accepting connections loop upon rejecting an invalid one

From: Tariq Saeed <[email protected]>
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.

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

fs/ocfs2/cluster/tcp.c renamed from fs/ocfs2/cluster/tcp.c~ocfs2-o2net-incorrect-to-terminate-accepting-connections-loop-upon-rejecting-an-invalid-one +3 -1

@@ -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)

Quoted 8 lines · akpm at linux-foundation.org

From: Tariq Saeed <[email protected]>
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.

Quoted 27 lines

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

fs/ocfs2/cluster/tcp.c renamed from fs/ocfs2/cluster/tcp.c~ocfs2-o2net-incorrect-to-terminate-accepting-connections-loop-upon-rejecting-an-invalid-one +3 -1

@@ -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;
 

fs/ocfs2/cluster/tcp.c out: · line 1960

Quoted 13 lines

fs/ocfs2/cluster/tcp.c renamed from fs/ocfs2/cluster/tcp.c~ocfs2-o2net-incorrect-to-terminate-accepting-connections-loop-upon-rejecting-an-invalid-one +7 -1

  	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:

	if (err)
		break;
	/* only trust the value of 'more' when err == 0 */
	if (more)
		break;

Thanks,

	--Mark

--
Mark Fasheh

_______________________________________________
Ocfs2-devel mailing list
Ocfs2-devel at oss.oracle.com
https://oss.oracle.com/mailman/listinfo/ocfs2-devel

Quoted 8 lines · akpm at linux-foundation.org

From: Tariq Saeed <[email protected]>
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.

Quoted 30 lines

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

fs/ocfs2/cluster/tcp.c renamed from fs/ocfs2/cluster/tcp.c~ocfs2-o2net-incorrect-to-terminate-accepting-connections-loop-upon-rejecting-an-invalid-one +3 -1

@@ -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)

fs/ocfs2/cluster/tcp.c out: · line 1960

Quoted 10 lines

fs/ocfs2/cluster/tcp.c renamed from fs/ocfs2/cluster/tcp.c~ocfs2-o2net-incorrect-to-terminate-accepting-connections-loop-upon-rejecting-an-invalid-one +7 -1

  {
  	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:

	if (err)
		break;
	/* only trust the value of 'more' when err == 0 */
	if (more)
		break;

Thanks,

	--Mark

--
Mark Fasheh

Quoted 39 lines · Mark Fasheh

Quoted 8 lines · akpm at linux-foundation.org

From: Tariq Saeed <[email protected]>
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.

Quoted 27 lines

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

fs/ocfs2/cluster/tcp.c renamed from fs/ocfs2/cluster/tcp.c~ocfs2-o2net-incorrect-to-terminate-accepting-connections-loop-upon-rejecting-an-invalid-one +3 -1

@@ -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;
 

Quoted 16 lines

fs/ocfs2/cluster/tcp.c out: · line 1960

Quoted 13 lines

fs/ocfs2/cluster/tcp.c renamed from fs/ocfs2/cluster/tcp.c~ocfs2-o2net-incorrect-to-terminate-accepting-connections-loop-upon-rejecting-an-invalid-one +7 -1

  	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.

Quoted 67 lines · On Fri, 24 Jan 2014 14:22:57 -0800 tariq saeed <[email protected]> wrote:

Quoted 39 lines · Mark Fasheh

Quoted 8 lines · akpm at linux-foundation.org

From: Tariq Saeed <[email protected]>
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.

Quoted 27 lines

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

fs/ocfs2/cluster/tcp.c renamed from fs/ocfs2/cluster/tcp.c~ocfs2-o2net-incorrect-to-terminate-accepting-connections-loop-upon-rejecting-an-invalid-one +3 -1

@@ -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;
 

Quoted 16 lines

fs/ocfs2/cluster/tcp.c out: · line 1960

Quoted 13 lines

fs/ocfs2/cluster/tcp.c renamed from fs/ocfs2/cluster/tcp.c~ocfs2-o2net-incorrect-to-terminate-accepting-connections-loop-upon-rejecting-an-invalid-one +7 -1

   	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. _

Quoted 5 lines

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 <[email protected]>
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 <[email protected]>

Cc Mark Fasheh <[email protected]>

Cc Joel Becker <[email protected]>

Signed-off-by Andrew Morton <[email protected]>

---

  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

fs/ocfs2/cluster/tcp.c renamed from fs/ocfs2/cluster/tcp.c~ocfs2-o2net-incorrect-to-terminate-accepting-connections-loop-upon-rejecting-an-invalid-one +11 -2

@@ -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)

_

Quoted 60 lines

Quoted 59 lines

Quoted 27 lines

Quoted 27 lines

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

fs/ocfs2/cluster/tcp.c renamed from fs/ocfs2/cluster/tcp.c~ocfs2-o2net-incorrect-to-terminate-accepting-connections-loop-upon-rejecting-an-invalid-one +3 -1

@@ -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;
 

Quoted 16 lines

fs/ocfs2/cluster/tcp.c out: · line 1960

Quoted 13 lines

fs/ocfs2/cluster/tcp.c renamed from fs/ocfs2/cluster/tcp.c~ocfs2-o2net-incorrect-to-terminate-accepting-connections-loop-upon-rejecting-an-invalid-one +7 -1

   	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.

Quoted 4 lines

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?

Mark, do you have further questios?

Quoted 1 line

Mark, do you have further questios?

No but we're going to need a comment explaining this above the code in question. It's not entirely clear just by reading it.

	--Mark

--
Mark Fasheh