[PATCH] SUNRPC: Fix refcnt leak in rpc_clnt_test_and_add_xprt

[PATCH] SUNRPC: Fix refcnt leak in rpc_clnt_test_and_add_xprt

[Xiyu Yang]

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);
+	}
 	rpc_put_task(task);
 success:
 	return 1;
-- 
2.7.4

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

[Trond Myklebust]

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