Re: [PATCH] SUNRPC: Fix refcnt leak in rpc_clnt_test_and_add_xprt

[Trond Myklebust <trondmy@hammerspace.com>]

On Mon, 2020-04-20 at 13:45 +0800, Xiyu Yang wrote:
> rpc_clnt_test_and_add_xprt() invokes xprt_switch_get() and
> xprt_get(),
> which returns a reference of the rpc_xprt_switch object to "data-
> >xps"
> and a reference of the rpc_xprt object to "data->xprt" with increased
> refcount.
> 
> When rpc_clnt_test_and_add_xprt() returns, local variable "data" and
> its
> field "xps" as well as "xprt" becomes invalid, so their refcounts
> should
> be decreased to keep refcount balanced.
> 
> The reference counting issue happens in one exception handling paths
> of
> rpc_clnt_test_and_add_xprt(). When rpc_call_null_helper() returns
> IS_ERR, the refcnt increased by xprt_switch_get() and xprt_get() are
> not
> decreased, causing a refcnt leak.
> 
> Fix this issue by calling rpc_cb_add_xprt_release() to decrease
> related
> refcounted fields in "data" and then release it when
> rpc_call_null_helper() returns IS_ERR.
> 
> Fixes: 7f554890587c ("SUNRPC: Allow addition of new transports to a
> struct rpc_clnt")
> Signed-off-by: Xiyu Yang <xiyuyang19@fudan.edu.cn>
> Signed-off-by: Xin Tan <tanxin.ctf@gmail.com>
> ---
>  net/sunrpc/clnt.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/net/sunrpc/clnt.c b/net/sunrpc/clnt.c
> index 7324b21f923e..f86d9ae2167f 100644
> --- a/net/sunrpc/clnt.c
> +++ b/net/sunrpc/clnt.c
> @@ -2803,8 +2803,10 @@ int rpc_clnt_test_and_add_xprt(struct rpc_clnt
> *clnt,
>  	task = rpc_call_null_helper(clnt, xprt, NULL,
>  			RPC_TASK_SOFT|RPC_TASK_SOFTCONN|RPC_TASK_ASYNC|
> RPC_TASK_NULLCREDS,
>  			&rpc_cb_add_xprt_call_ops, data);
> -	if (IS_ERR(task))
> +	if (IS_ERR(task)) {
> +		rpc_cb_add_xprt_release(data);
>  		return PTR_ERR(task);

We should just get rid of the IS_ERR() condition here. It cannot ever
trigger, which is why rpc_run_task() no longer checks for it, and no
longer calls the ->rpc_release() method on error.

> +	}
>  	rpc_put_task(task);
>  success:
>  	return 1;
-- 
Trond Myklebust
Linux NFS client maintainer, Hammerspace
trond.myklebust@hammerspace.com