* [PATCH] SUNRPC: Avoid deep recursion in rpc_release_client
@ 2013-11-12 22:36 Trond Myklebust
2013-11-12 22:50 ` [PATCH v2] " Trond Myklebust
0 siblings, 1 reply; 3+ messages in thread
From: Trond Myklebust @ 2013-11-12 22:36 UTC (permalink / raw)
To: linux-nfs; +Cc: Jeff Layton, Weston Andros Adamson
In cases where an rpc client has a parent hierarchy, then
rpc_free_client may end up calling rpc_release_client() on the
parent, thus recursing back into rpc_free_client. If the hierarchy
is deep enough, then we can get into situations where the stack
simply overflows.
The fix is to have rpc_release_client() loop so that it can take
care of the parent rpc client hierarchy without needing to
recurse.
Reported-by: Jeff Layton <jlayton@redhat.com>
Reported-by: Weston Andros Adamson <dros@netapp.com>
Cc: stable@vger.kernel.org
Signed-off-by: Trond Myklebust <Trond.Myklebust@netapp.com>
---
net/sunrpc/clnt.c | 29 +++++++++++++++++------------
1 file changed, 17 insertions(+), 12 deletions(-)
diff --git a/net/sunrpc/clnt.c b/net/sunrpc/clnt.c
index dab09dac8fc7..fb9830903213 100644
--- a/net/sunrpc/clnt.c
+++ b/net/sunrpc/clnt.c
@@ -750,14 +750,16 @@ EXPORT_SYMBOL_GPL(rpc_shutdown_client);
/*
* Free an RPC client
*/
-static void
+static struct rpc_clnt *
rpc_free_client(struct rpc_clnt *clnt)
{
+ struct rpc_clnt *parent = NULL;
+
dprintk_rcu("RPC: destroying %s client for %s\n",
clnt->cl_program->name,
rcu_dereference(clnt->cl_xprt)->servername);
if (clnt->cl_parent != clnt)
- rpc_release_client(clnt->cl_parent);
+ parent = clnt->cl_parent;
rpc_clnt_remove_pipedir(clnt);
rpc_unregister_client(clnt);
rpc_free_iostats(clnt->cl_metrics);
@@ -766,18 +768,17 @@ rpc_free_client(struct rpc_clnt *clnt)
rpciod_down();
rpc_free_clid(clnt);
kfree(clnt);
+ return parent;
}
/*
* Free an RPC client
*/
-static void
+static struct rpc_clnt *
rpc_free_auth(struct rpc_clnt *clnt)
{
- if (clnt->cl_auth == NULL) {
- rpc_free_client(clnt);
- return;
- }
+ if (clnt->cl_auth == NULL)
+ return rpc_free_client(clnt);
/*
* Note: RPCSEC_GSS may need to send NULL RPC calls in order to
@@ -788,7 +789,8 @@ rpc_free_auth(struct rpc_clnt *clnt)
rpcauth_release(clnt->cl_auth);
clnt->cl_auth = NULL;
if (atomic_dec_and_test(&clnt->cl_count))
- rpc_free_client(clnt);
+ return rpc_free_client(clnt);
+ return NULL;
}
/*
@@ -799,10 +801,13 @@ rpc_release_client(struct rpc_clnt *clnt)
{
dprintk("RPC: rpc_release_client(%p)\n", clnt);
- if (list_empty(&clnt->cl_tasks))
- wake_up(&destroy_wait);
- if (atomic_dec_and_test(&clnt->cl_count))
- rpc_free_auth(clnt);
+ for (;;) {
+ if (list_empty(&clnt->cl_tasks))
+ wake_up(&destroy_wait);
+ if (!atomic_dec_and_test(&clnt->cl_count))
+ break;
+ clnt = rpc_free_auth(clnt);
+ }
}
EXPORT_SYMBOL_GPL(rpc_release_client);
--
1.8.3.1
^ permalink raw reply related [flat|nested] 3+ messages in thread
* [PATCH v2] SUNRPC: Avoid deep recursion in rpc_release_client
2013-11-12 22:36 [PATCH] SUNRPC: Avoid deep recursion in rpc_release_client Trond Myklebust
@ 2013-11-12 22:50 ` Trond Myklebust
2013-11-14 11:13 ` Jeff Layton
0 siblings, 1 reply; 3+ messages in thread
From: Trond Myklebust @ 2013-11-12 22:50 UTC (permalink / raw)
To: linux-nfs; +Cc: Jeff Layton, Weston Andros Adamson
In cases where an rpc client has a parent hierarchy, then
rpc_free_client may end up calling rpc_release_client() on the
parent, thus recursing back into rpc_free_client. If the hierarchy
is deep enough, then we can get into situations where the stack
simply overflows.
The fix is to have rpc_release_client() loop so that it can take
care of the parent rpc client hierarchy without needing to
recurse.
Reported-by: Jeff Layton <jlayton@redhat.com>
Reported-by: Weston Andros Adamson <dros@netapp.com>
Cc: stable@vger.kernel.org
Signed-off-by: Trond Myklebust <Trond.Myklebust@netapp.com>
---
v2: Don't recurse if parent == NULL (Doh!)
net/sunrpc/clnt.c | 29 +++++++++++++++++------------
1 file changed, 17 insertions(+), 12 deletions(-)
diff --git a/net/sunrpc/clnt.c b/net/sunrpc/clnt.c
index dab09dac8fc7..f09b7db2c492 100644
--- a/net/sunrpc/clnt.c
+++ b/net/sunrpc/clnt.c
@@ -750,14 +750,16 @@ EXPORT_SYMBOL_GPL(rpc_shutdown_client);
/*
* Free an RPC client
*/
-static void
+static struct rpc_clnt *
rpc_free_client(struct rpc_clnt *clnt)
{
+ struct rpc_clnt *parent = NULL;
+
dprintk_rcu("RPC: destroying %s client for %s\n",
clnt->cl_program->name,
rcu_dereference(clnt->cl_xprt)->servername);
if (clnt->cl_parent != clnt)
- rpc_release_client(clnt->cl_parent);
+ parent = clnt->cl_parent;
rpc_clnt_remove_pipedir(clnt);
rpc_unregister_client(clnt);
rpc_free_iostats(clnt->cl_metrics);
@@ -766,18 +768,17 @@ rpc_free_client(struct rpc_clnt *clnt)
rpciod_down();
rpc_free_clid(clnt);
kfree(clnt);
+ return parent;
}
/*
* Free an RPC client
*/
-static void
+static struct rpc_clnt *
rpc_free_auth(struct rpc_clnt *clnt)
{
- if (clnt->cl_auth == NULL) {
- rpc_free_client(clnt);
- return;
- }
+ if (clnt->cl_auth == NULL)
+ return rpc_free_client(clnt);
/*
* Note: RPCSEC_GSS may need to send NULL RPC calls in order to
@@ -788,7 +789,8 @@ rpc_free_auth(struct rpc_clnt *clnt)
rpcauth_release(clnt->cl_auth);
clnt->cl_auth = NULL;
if (atomic_dec_and_test(&clnt->cl_count))
- rpc_free_client(clnt);
+ return rpc_free_client(clnt);
+ return NULL;
}
/*
@@ -799,10 +801,13 @@ rpc_release_client(struct rpc_clnt *clnt)
{
dprintk("RPC: rpc_release_client(%p)\n", clnt);
- if (list_empty(&clnt->cl_tasks))
- wake_up(&destroy_wait);
- if (atomic_dec_and_test(&clnt->cl_count))
- rpc_free_auth(clnt);
+ do {
+ if (list_empty(&clnt->cl_tasks))
+ wake_up(&destroy_wait);
+ if (!atomic_dec_and_test(&clnt->cl_count))
+ break;
+ clnt = rpc_free_auth(clnt);
+ } while (clnt != NULL);
}
EXPORT_SYMBOL_GPL(rpc_release_client);
--
1.8.3.1
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH v2] SUNRPC: Avoid deep recursion in rpc_release_client
2013-11-12 22:50 ` [PATCH v2] " Trond Myklebust
@ 2013-11-14 11:13 ` Jeff Layton
0 siblings, 0 replies; 3+ messages in thread
From: Jeff Layton @ 2013-11-14 11:13 UTC (permalink / raw)
To: Trond Myklebust; +Cc: linux-nfs, Weston Andros Adamson
On Tue, 12 Nov 2013 17:50:15 -0500
Trond Myklebust <Trond.Myklebust@netapp.com> wrote:
> In cases where an rpc client has a parent hierarchy, then
> rpc_free_client may end up calling rpc_release_client() on the
> parent, thus recursing back into rpc_free_client. If the hierarchy
> is deep enough, then we can get into situations where the stack
> simply overflows.
>
> The fix is to have rpc_release_client() loop so that it can take
> care of the parent rpc client hierarchy without needing to
> recurse.
>
> Reported-by: Jeff Layton <jlayton@redhat.com>
> Reported-by: Weston Andros Adamson <dros@netapp.com>
> Cc: stable@vger.kernel.org
> Signed-off-by: Trond Myklebust <Trond.Myklebust@netapp.com>
> ---
> v2: Don't recurse if parent == NULL (Doh!)
>
>
> net/sunrpc/clnt.c | 29 +++++++++++++++++------------
> 1 file changed, 17 insertions(+), 12 deletions(-)
>
> diff --git a/net/sunrpc/clnt.c b/net/sunrpc/clnt.c
> index dab09dac8fc7..f09b7db2c492 100644
> --- a/net/sunrpc/clnt.c
> +++ b/net/sunrpc/clnt.c
> @@ -750,14 +750,16 @@ EXPORT_SYMBOL_GPL(rpc_shutdown_client);
> /*
> * Free an RPC client
> */
> -static void
> +static struct rpc_clnt *
> rpc_free_client(struct rpc_clnt *clnt)
> {
> + struct rpc_clnt *parent = NULL;
> +
> dprintk_rcu("RPC: destroying %s client for %s\n",
> clnt->cl_program->name,
> rcu_dereference(clnt->cl_xprt)->servername);
> if (clnt->cl_parent != clnt)
> - rpc_release_client(clnt->cl_parent);
> + parent = clnt->cl_parent;
> rpc_clnt_remove_pipedir(clnt);
> rpc_unregister_client(clnt);
> rpc_free_iostats(clnt->cl_metrics);
> @@ -766,18 +768,17 @@ rpc_free_client(struct rpc_clnt *clnt)
> rpciod_down();
> rpc_free_clid(clnt);
> kfree(clnt);
> + return parent;
> }
>
> /*
> * Free an RPC client
> */
> -static void
> +static struct rpc_clnt *
> rpc_free_auth(struct rpc_clnt *clnt)
> {
> - if (clnt->cl_auth == NULL) {
> - rpc_free_client(clnt);
> - return;
> - }
> + if (clnt->cl_auth == NULL)
> + return rpc_free_client(clnt);
>
> /*
> * Note: RPCSEC_GSS may need to send NULL RPC calls in order to
> @@ -788,7 +789,8 @@ rpc_free_auth(struct rpc_clnt *clnt)
> rpcauth_release(clnt->cl_auth);
> clnt->cl_auth = NULL;
> if (atomic_dec_and_test(&clnt->cl_count))
> - rpc_free_client(clnt);
> + return rpc_free_client(clnt);
> + return NULL;
> }
>
> /*
> @@ -799,10 +801,13 @@ rpc_release_client(struct rpc_clnt *clnt)
> {
> dprintk("RPC: rpc_release_client(%p)\n", clnt);
>
> - if (list_empty(&clnt->cl_tasks))
> - wake_up(&destroy_wait);
> - if (atomic_dec_and_test(&clnt->cl_count))
> - rpc_free_auth(clnt);
> + do {
> + if (list_empty(&clnt->cl_tasks))
> + wake_up(&destroy_wait);
> + if (!atomic_dec_and_test(&clnt->cl_count))
> + break;
> + clnt = rpc_free_auth(clnt);
> + } while (clnt != NULL);
> }
> EXPORT_SYMBOL_GPL(rpc_release_client);
>
Much better!
Reviewed-by: Jeff Layton <jlayton@redhat.com>
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2013-11-14 11:13 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-11-12 22:36 [PATCH] SUNRPC: Avoid deep recursion in rpc_release_client Trond Myklebust
2013-11-12 22:50 ` [PATCH v2] " Trond Myklebust
2013-11-14 11:13 ` Jeff Layton
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).