Currently, when we try to mount and get back NFS4ERR_CLID_IN_USE or
NFS4ERR_WRONGSEC, we create a new rpc_clnt and then try the call again.
There is no guarantee that doing so will work however, so we can end up
retrying the call in an infinite loop.
Worse yet, we create the new client using rpc_clone_client_set_auth,
which creates the new client as a child of the old one. Thus, we can end
up with a *very* long lineage of rpc_clnts. When we go to put all of the
references to them, we can end up with a long call chain that can smash
the stack as each rpc_free_client() call can recurse back into itself.
This patch fixes this by simply ensuring that the SETCLIENTID call will
only be retried in this situation if the last attempt did not use
RPC_AUTH_UNIX.
Cc: [email protected] # v3.10+
Cc: Weston Andros Adamson <[email protected]>
Cc: Chuck Lever <[email protected]>
Signed-off-by: Jeff Layton <[email protected]>
---
fs/nfs/nfs4state.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/fs/nfs/nfs4state.c b/fs/nfs/nfs4state.c
index c8e729d..4c26c01 100644
--- a/fs/nfs/nfs4state.c
+++ b/fs/nfs/nfs4state.c
@@ -2097,6 +2097,11 @@ again:
break;
case -NFS4ERR_CLID_INUSE:
case -NFS4ERR_WRONGSEC:
+ /* No point in retrying if we already used RPC_AUTH_UNIX */
+ if (clnt->cl_auth->au_flavor == RPC_AUTH_UNIX) {
+ status = -EPERM;
+ break;
+ }
clnt = rpc_clone_client_set_auth(clnt, RPC_AUTH_UNIX);
if (IS_ERR(clnt)) {
status = PTR_ERR(clnt);
--
1.8.3.1
On Nov 12, 2013, at 4:30 PM, Jeff Layton <[email protected]> wrote:
> Currently, when we try to mount and get back NFS4ERR_CLID_IN_USE or
> NFS4ERR_WRONGSEC, we create a new rpc_clnt and then try the call again.
> There is no guarantee that doing so will work however, so we can end up
> retrying the call in an infinite loop.
>
> Worse yet, we create the new client using rpc_clone_client_set_auth,
> which creates the new client as a child of the old one. Thus, we can end
> up with a *very* long lineage of rpc_clnts. When we go to put all of the
> references to them, we can end up with a long call chain that can smash
> the stack as each rpc_free_client() call can recurse back into itself.
>
> This patch fixes this by simply ensuring that the SETCLIENTID call will
> only be retried in this situation if the last attempt did not use
> RPC_AUTH_UNIX.
>
> Cc: [email protected] # v3.10+
> Cc: Weston Andros Adamson <[email protected]>
> Cc: Chuck Lever <[email protected]>
> Signed-off-by: Jeff Layton <[email protected]>
> ---
> fs/nfs/nfs4state.c | 5 +++++
> 1 file changed, 5 insertions(+)
>
> diff --git a/fs/nfs/nfs4state.c b/fs/nfs/nfs4state.c
> index c8e729d..4c26c01 100644
> --- a/fs/nfs/nfs4state.c
> +++ b/fs/nfs/nfs4state.c
> @@ -2097,6 +2097,11 @@ again:
> break;
Note that the -EACCES case falls through. I guess it is safe to use the new logic for that case?
There is an "i++" in the EACCES case that is supposed to prevent the looping you describe. Maybe that should just be removed, now that there is a more robust test here.
> case -NFS4ERR_CLID_INUSE:
> case -NFS4ERR_WRONGSEC:
> + /* No point in retrying if we already used RPC_AUTH_UNIX */
> + if (clnt->cl_auth->au_flavor == RPC_AUTH_UNIX) {
> + status = -EPERM;
> + break;
> + }
> clnt = rpc_clone_client_set_auth(clnt, RPC_AUTH_UNIX);
> if (IS_ERR(clnt)) {
> status = PTR_ERR(clnt);
--
Chuck Lever
chuck[dot]lever[at]oracle[dot]com
On Nov 12, 2013, at 16:30, Jeff Layton <[email protected]> wrote:
> Currently, when we try to mount and get back NFS4ERR_CLID_IN_USE or
> NFS4ERR_WRONGSEC, we create a new rpc_clnt and then try the call again.
> There is no guarantee that doing so will work however, so we can end up
> retrying the call in an infinite loop.
>
> Worse yet, we create the new client using rpc_clone_client_set_auth,
> which creates the new client as a child of the old one. Thus, we can end
> up with a *very* long lineage of rpc_clnts. When we go to put all of the
> references to them, we can end up with a long call chain that can smash
> the stack as each rpc_free_client() call can recurse back into itself.
Note that we should perhaps also try to fix rpc_free_client. I have a patch for that...
Cheers
Trond
Tested-by/Acked-by: Weston Andros Adamson <[email protected]>
I can?t reproduce the BUG() with this patch applied.
Besides the patch making complete sense to me, I have more evidence to back your explanation of the bug:
Without this patch I can run a ton of mount/umounts and eventually hit the crash. Before the crash there will be *many* (10s of thousands) pipes in /var/lib/nfs/rpc_pipefs/nfs/, which kills gssd (too many fds).
-dros
On Nov 12, 2013, at 4:30 PM, Jeff Layton <[email protected]> wrote:
> Currently, when we try to mount and get back NFS4ERR_CLID_IN_USE or
> NFS4ERR_WRONGSEC, we create a new rpc_clnt and then try the call again.
> There is no guarantee that doing so will work however, so we can end up
> retrying the call in an infinite loop.
>
> Worse yet, we create the new client using rpc_clone_client_set_auth,
> which creates the new client as a child of the old one. Thus, we can end
> up with a *very* long lineage of rpc_clnts. When we go to put all of the
> references to them, we can end up with a long call chain that can smash
> the stack as each rpc_free_client() call can recurse back into itself.
>
> This patch fixes this by simply ensuring that the SETCLIENTID call will
> only be retried in this situation if the last attempt did not use
> RPC_AUTH_UNIX.
>
> Cc: [email protected] # v3.10+
> Cc: Weston Andros Adamson <[email protected]>
> Cc: Chuck Lever <[email protected]>
> Signed-off-by: Jeff Layton <[email protected]>
> ---
> fs/nfs/nfs4state.c | 5 +++++
> 1 file changed, 5 insertions(+)
>
> diff --git a/fs/nfs/nfs4state.c b/fs/nfs/nfs4state.c
> index c8e729d..4c26c01 100644
> --- a/fs/nfs/nfs4state.c
> +++ b/fs/nfs/nfs4state.c
> @@ -2097,6 +2097,11 @@ again:
> break;
> case -NFS4ERR_CLID_INUSE:
> case -NFS4ERR_WRONGSEC:
> + /* No point in retrying if we already used RPC_AUTH_UNIX */
> + if (clnt->cl_auth->au_flavor == RPC_AUTH_UNIX) {
> + status = -EPERM;
> + break;
> + }
> clnt = rpc_clone_client_set_auth(clnt, RPC_AUTH_UNIX);
> if (IS_ERR(clnt)) {
> status = PTR_ERR(clnt);
> --
> 1.8.3.1
>
On Tue, 12 Nov 2013 17:08:53 -0500
Chuck Lever <[email protected]> wrote:
>
> On Nov 12, 2013, at 4:30 PM, Jeff Layton <[email protected]> wrote:
>
> > Currently, when we try to mount and get back NFS4ERR_CLID_IN_USE or
> > NFS4ERR_WRONGSEC, we create a new rpc_clnt and then try the call again.
> > There is no guarantee that doing so will work however, so we can end up
> > retrying the call in an infinite loop.
> >
> > Worse yet, we create the new client using rpc_clone_client_set_auth,
> > which creates the new client as a child of the old one. Thus, we can end
> > up with a *very* long lineage of rpc_clnts. When we go to put all of the
> > references to them, we can end up with a long call chain that can smash
> > the stack as each rpc_free_client() call can recurse back into itself.
> >
> > This patch fixes this by simply ensuring that the SETCLIENTID call will
> > only be retried in this situation if the last attempt did not use
> > RPC_AUTH_UNIX.
> >
> > Cc: [email protected] # v3.10+
> > Cc: Weston Andros Adamson <[email protected]>
> > Cc: Chuck Lever <[email protected]>
> > Signed-off-by: Jeff Layton <[email protected]>
> > ---
> > fs/nfs/nfs4state.c | 5 +++++
> > 1 file changed, 5 insertions(+)
> >
> > diff --git a/fs/nfs/nfs4state.c b/fs/nfs/nfs4state.c
> > index c8e729d..4c26c01 100644
> > --- a/fs/nfs/nfs4state.c
> > +++ b/fs/nfs/nfs4state.c
> > @@ -2097,6 +2097,11 @@ again:
> > break;
>
> Note that the -EACCES case falls through. I guess it is safe to use the new logic for that case?
>
> There is an "i++" in the EACCES case that is supposed to prevent the looping you describe. Maybe that should just be removed, now that there is a more robust test here.
>
Ahh thanks, I didn't notice that. I'll fix and resend...
Is it kosher to turn -EACCES into -EPERM there, or do we need to
preserve it?
> > case -NFS4ERR_CLID_INUSE:
> > case -NFS4ERR_WRONGSEC:
> > + /* No point in retrying if we already used RPC_AUTH_UNIX */
> > + if (clnt->cl_auth->au_flavor == RPC_AUTH_UNIX) {
> > + status = -EPERM;
> > + break;
> > + }
> > clnt = rpc_clone_client_set_auth(clnt, RPC_AUTH_UNIX);
> > if (IS_ERR(clnt)) {
> > status = PTR_ERR(clnt);
>
--
Jeff Layton <[email protected]>