Return-Path: linux-nfs-owner@vger.kernel.org Received: from mail-pa0-f54.google.com ([209.85.220.54]:36908 "EHLO mail-pa0-f54.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1756615AbaAGDtU (ORCPT ); Mon, 6 Jan 2014 22:49:20 -0500 Received: by mail-pa0-f54.google.com with SMTP id rd3so19499656pab.13 for ; Mon, 06 Jan 2014 19:49:19 -0800 (PST) Message-ID: <52CB793A.1040501@gmail.com> Date: Tue, 07 Jan 2014 11:49:14 +0800 From: Kinglong Mee MIME-Version: 1.0 To: "J. Bruce Fields" CC: Trond Myklebust , Linux NFS Mailing List Subject: Re: [PATCH] SUNRPC: fix memory leak of peer addresses in XPRT References: <52CA8460.6030206@gmail.com> <20140106202024.GD31764@fieldses.org> In-Reply-To: <20140106202024.GD31764@fieldses.org> Content-Type: text/plain; charset=ISO-8859-1 Sender: linux-nfs-owner@vger.kernel.org List-ID: 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 >> --- >> 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 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 --- 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