[Cluster-devel] [PATCH 14/17] dlm: fix memory leak in tcp_accept_from_sock()

[Cluster-devel] [PATCH 14/17] dlm: fix memory leak in tcp_accept_from_sock()

[tsutomu.owa]

The sk member of the socket generated by sock_create_kern() is overwritten
by ops->accept(). So the previous sk will not be released.
We use kernel_accept() instead of sock_create_kern() and ops->accept().

Signed-off-by: Tadashi Miyauchi <miyauchi@toshiba-tops.co.jp>
Signed-off-by: Tsutomu Owa <tsutomu.owa@toshiba.co.jp>
---
 fs/dlm/lowcomms.c | 21 +++++++--------------
 1 file changed, 7 insertions(+), 14 deletions(-)

diff --git a/fs/dlm/lowcomms.c b/fs/dlm/lowcomms.c
index 74ed41e..3e7c096 100644
--- a/fs/dlm/lowcomms.c
+++ b/fs/dlm/lowcomms.c
@@ -732,22 +732,14 @@ static int tcp_accept_from_sock(struct connection *con)
 	}
 	mutex_unlock(&connections_lock);
 
-	memset(&peeraddr, 0, sizeof(peeraddr));
-	result = sock_create_kern(&init_net, dlm_local_addr[0]->ss_family,
-				  SOCK_STREAM, IPPROTO_TCP, &newsock);
-	if (result < 0)
-		return -ENOMEM;
-
 	mutex_lock_nested(&con->sock_mutex, 0);
 
-	result = -ENOTCONN;
-	if (con->sock == NULL)
-		goto accept_err;
-
-	newsock->type = con->sock->type;
-	newsock->ops = con->sock->ops;
+	if (!con->sock) {
+		mutex_unlock(&con->sock_mutex);
+		return -ENOTCONN;
+	}
 
-	result = con->sock->ops->accept(con->sock, newsock, O_NONBLOCK);
+	result = kernel_accept(con->sock, &newsock, O_NONBLOCK);
 	if (result < 0)
 		goto accept_err;
 
@@ -844,7 +836,8 @@ static int tcp_accept_from_sock(struct connection *con)
 
 accept_err:
 	mutex_unlock(&con->sock_mutex);
-	sock_release(newsock);
+	if (newsock)
+		sock_release(newsock);
 
 	if (result != -EAGAIN)
 		log_print("error accepting connection from node: %d", result);
-- 
2.7.4

[Cluster-devel] [PATCH 14/17] dlm: fix memory leak in tcp_accept_from_sock()

[Steven Whitehouse]

Hi,


On 09/08/17 06:51, tsutomu.owa at toshiba.co.jp wrote:
> The sk member of the socket generated by sock_create_kern() is overwritten
> by ops->accept(). So the previous sk will not be released.
> We use kernel_accept() instead of sock_create_kern() and ops->accept().
Yes, that is a better solution than the open coded fix that was proposed  
recently. Looks good to me,

Steve.

>
> Signed-off-by: Tadashi Miyauchi <miyauchi@toshiba-tops.co.jp>
> Signed-off-by: Tsutomu Owa <tsutomu.owa@toshiba.co.jp>
> ---
>   fs/dlm/lowcomms.c | 21 +++++++--------------
>   1 file changed, 7 insertions(+), 14 deletions(-)
>
> diff --git a/fs/dlm/lowcomms.c b/fs/dlm/lowcomms.c
> index 74ed41e..3e7c096 100644
> --- a/fs/dlm/lowcomms.c
> +++ b/fs/dlm/lowcomms.c
> @@ -732,22 +732,14 @@ static int tcp_accept_from_sock(struct connection *con)
>   	}
>   	mutex_unlock(&connections_lock);
>   
> -	memset(&peeraddr, 0, sizeof(peeraddr));
> -	result = sock_create_kern(&init_net, dlm_local_addr[0]->ss_family,
> -				  SOCK_STREAM, IPPROTO_TCP, &newsock);
> -	if (result < 0)
> -		return -ENOMEM;
> -
>   	mutex_lock_nested(&con->sock_mutex, 0);
>   
> -	result = -ENOTCONN;
> -	if (con->sock == NULL)
> -		goto accept_err;
> -
> -	newsock->type = con->sock->type;
> -	newsock->ops = con->sock->ops;
> +	if (!con->sock) {
> +		mutex_unlock(&con->sock_mutex);
> +		return -ENOTCONN;
> +	}
>   
> -	result = con->sock->ops->accept(con->sock, newsock, O_NONBLOCK);
> +	result = kernel_accept(con->sock, &newsock, O_NONBLOCK);
>   	if (result < 0)
>   		goto accept_err;
>   
> @@ -844,7 +836,8 @@ static int tcp_accept_from_sock(struct connection *con)
>   
>   accept_err:
>   	mutex_unlock(&con->sock_mutex);
> -	sock_release(newsock);
> +	if (newsock)
> +		sock_release(newsock);
>   
>   	if (result != -EAGAIN)
>   		log_print("error accepting connection from node: %d", result);