Re: [PATCH] SUNRPC: fix memory leak of peer addresses in XPRT

[Kinglong Mee <kinglongmee@xxxxxxxxx>]

On 01/07/2014 04:20 AM, J. Bruce Fields wrote:
> On Mon, Jan 06, 2014 at 06:24:32PM +0800, Kinglong Mee wrote:
>> If creating xprt failed after xs_format_peer_addresses,
>> sunrpc must free those memory of peer addresses in xprt.
>>
>> Signed-off-by: Kinglong Mee <kinglongmee@xxxxxxxxx>
>> ---
>>  net/sunrpc/xprtsock.c | 11 +++++++++--
>>  1 file changed, 9 insertions(+), 2 deletions(-)
>>
>> diff --git a/net/sunrpc/xprtsock.c b/net/sunrpc/xprtsock.c
>> index 25dbfa9..11ceba3 100644
>> --- a/net/sunrpc/xprtsock.c
>> +++ b/net/sunrpc/xprtsock.c
>> @@ -2725,8 +2725,10 @@ static struct rpc_xprt *xs_setup_local(struct xprt_create *args)
>>  		xprt_set_bound(xprt);
>>  		xs_format_peer_addresses(xprt, "local", RPCBIND_NETID_LOCAL);
>>  		ret = ERR_PTR(xs_local_setup_socket(transport));
>> -		if (ret)
>> +		if (ret) {
>> +			xs_free_peer_addresses(xprt);
>>  			goto out_err;
>> +		}
>>  		break;
>>  	default:
>>  		ret = ERR_PTR(-EAFNOSUPPORT);
>> @@ -2738,6 +2740,8 @@ static struct rpc_xprt *xs_setup_local(struct xprt_create *args)
>>  
>>  	if (try_module_get(THIS_MODULE))
>>  		return xprt;
>> +
>> +	xs_free_peer_addresses(xprt);
>>  	ret = ERR_PTR(-EINVAL);
>>  out_err:
>>  	xprt_free(xprt);
> 
> This is getting a little hairy.... Looks like xprts are alloc'd with
> kzalloc() and xs_free_peer_addresses is a no-op if
> xprt->address_strings[i] is NULL, so it looks safe to call
> unconditionally after out_err?
> 
>> @@ -2816,6 +2820,8 @@ static struct rpc_xprt *xs_setup_udp(struct xprt_create *args)
>>  
>>  	if (try_module_get(THIS_MODULE))
>>  		return xprt;
>> +
>> +	xs_free_peer_addresses(xprt);
>>  	ret = ERR_PTR(-EINVAL);
>>  out_err:
>>  	xprt_free(xprt);
>> @@ -2893,9 +2899,10 @@ static struct rpc_xprt *xs_setup_tcp(struct xprt_create *args)
>>  				xprt->address_strings[RPC_DISPLAY_ADDR],
>>  				xprt->address_strings[RPC_DISPLAY_PROTO]);
>>  
>> -
>>  	if (try_module_get(THIS_MODULE))
>>  		return xprt;
>> +
>> +	xs_free_peer_addresses(xprt);
>>  	ret = ERR_PTR(-EINVAL);
>>  out_err:
>>  	xprt_free(xprt);
>> -- 
>> 1.8.4.2
> 
> And after this we'll end up with
> 
> 	xs_free_peer_addresses(xprt);
> 	xprt_free(xprt);
> 
> in 4 different places (the above plus xs_destroy), so I might define an
> xs_xprt_free() to do that.

I have make a new patch as your suggestion.

thanks,
Kinglong Mee

>From 9a53cfcf2c6c9dafeace28ea0c3ac9b3e786412e Mon Sep 17 00:00:00 2001
From: Kinglong Mee <kinglongmee@xxxxxxxxx>
Date: Tue, 7 Jan 2014 11:43:59 +0800
Subject: [PATCH] SUNRPC: fix memory leak of peer addresses in XPRT

If creating xprt failed after xs_format_peer_addresses,
sunrpc must free those memory of peer addresses in xprt.

Add a helper function for freeing xprt named xs_xprt_free.

Signed-off-by: Kinglong Mee <kinglongmee@xxxxxxxxx>
---
 net/sunrpc/xprtsock.c | 18 +++++++++++-------
 1 file changed, 11 insertions(+), 7 deletions(-)

diff --git a/net/sunrpc/xprtsock.c b/net/sunrpc/xprtsock.c
index 25dbfa9..4fcdf74 100644
--- a/net/sunrpc/xprtsock.c
+++ b/net/sunrpc/xprtsock.c
@@ -905,6 +905,12 @@ static void xs_tcp_close(struct rpc_xprt *xprt)
 		xs_tcp_shutdown(xprt);
 }
 
+static void xs_xprt_free(struct rpc_xprt *xprt)
+{
+	xs_free_peer_addresses(xprt);
+	xprt_free(xprt);
+}
+
 /**
  * xs_destroy - prepare to shutdown a transport
  * @xprt: doomed transport
@@ -915,8 +921,7 @@ static void xs_destroy(struct rpc_xprt *xprt)
 	dprintk("RPC:       xs_destroy xprt %p\n", xprt);
 
 	xs_close(xprt);
-	xs_free_peer_addresses(xprt);
-	xprt_free(xprt);
+	xs_xprt_free(xprt);
 	module_put(THIS_MODULE);
 }
 
@@ -2740,7 +2745,7 @@ static struct rpc_xprt *xs_setup_local(struct xprt_create *args)
 		return xprt;
 	ret = ERR_PTR(-EINVAL);
 out_err:
-	xprt_free(xprt);
+	xs_xprt_free(xprt);
 	return ret;
 }
 
@@ -2818,7 +2823,7 @@ static struct rpc_xprt *xs_setup_udp(struct xprt_create *args)
 		return xprt;
 	ret = ERR_PTR(-EINVAL);
 out_err:
-	xprt_free(xprt);
+	xs_xprt_free(xprt);
 	return ret;
 }
 
@@ -2893,12 +2898,11 @@ static struct rpc_xprt *xs_setup_tcp(struct xprt_create *args)
 				xprt->address_strings[RPC_DISPLAY_ADDR],
 				xprt->address_strings[RPC_DISPLAY_PROTO]);
 
-
 	if (try_module_get(THIS_MODULE))
 		return xprt;
 	ret = ERR_PTR(-EINVAL);
 out_err:
-	xprt_free(xprt);
+	xs_xprt_free(xprt);
 	return ret;
 }
 
@@ -2988,7 +2992,7 @@ static struct rpc_xprt *xs_setup_bc_tcp(struct xprt_create *args)
 	xprt_put(xprt);
 	ret = ERR_PTR(-EINVAL);
 out_err:
-	xprt_free(xprt);
+	xs_xprt_free(xprt);
 	return ret;
 }
 
-- 
1.8.4.2



--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html