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

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

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:

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

Thanks,
	--Mark

--
Mark Fasheh