| Message ID | 1587361519-83687-1-git-send-email-xiyuyang19@fudan.edu.cn |
|---|---|
| State | Not Applicable |
| Delegated to: | David Miller |
| Headers | show |
| Series | SUNRPC: Fix refcnt leak in rpc_clnt_test_and_add_xprt | expand |
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;
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;
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(-)