Dear Bruce,
what's the status of this patch?
thanks,
Kinglong Mee
On Tue, Jan 7, 2014 at 11:49 AM, Kinglong Mee <kinglongmee@xxxxxxxxx> wrote:
> 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
