2008-04-19 20:50:23

by Myklebust, Trond

[permalink] [raw]
Subject: [PATCH 03/33] SUNRPC: Don't attempt to destroy expired RPCSEC_GSS credentials..

..and always destroy using a 'soft' RPC call. Destroying GSS credentials
isn't mandatory; the server can always cope with a few credentials not
getting destroyed in a timely fashion.

This actually fixes a hang situation. Basically, some servers will decide
that the client is crazy if it tries to destroy an RPC context for which
they have sent an RPCSEC_GSS_CREDPROBLEM, and so will refuse to talk to it
for a while.
The regression therefor probably was introduced by commit
0df7fb74fbb709591301871a38aac7735a1d6583.

Signed-off-by: Trond Myklebust <[email protected]>
---

net/sunrpc/auth_gss/auth_gss.c | 4 ++--
1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/net/sunrpc/auth_gss/auth_gss.c b/net/sunrpc/auth_gss/auth_gss.c
index d34f6df..55948cd 100644
--- a/net/sunrpc/auth_gss/auth_gss.c
+++ b/net/sunrpc/auth_gss/auth_gss.c
@@ -710,7 +710,7 @@ gss_destroying_context(struct rpc_cred *cred)
struct rpc_task *task;

if (gss_cred->gc_ctx == NULL ||
- gss_cred->gc_ctx->gc_proc == RPC_GSS_PROC_DESTROY)
+ test_and_clear_bit(RPCAUTH_CRED_UPTODATE, &cred->cr_flags) == 0)
return 0;

gss_cred->gc_ctx->gc_proc = RPC_GSS_PROC_DESTROY;
@@ -720,7 +720,7 @@ gss_destroying_context(struct rpc_cred *cred)
* by the RPC call or by the put_rpccred() below */
get_rpccred(cred);

- task = rpc_call_null(gss_auth->client, cred, RPC_TASK_ASYNC);
+ task = rpc_call_null(gss_auth->client, cred, RPC_TASK_ASYNC|RPC_TASK_SOFT);
if (!IS_ERR(task))
rpc_put_task(task);




2008-04-21 17:51:14

by Chuck Lever III

[permalink] [raw]
Subject: Re: [PATCH 03/33] SUNRPC: Don't attempt to destroy expired RPCSEC_GSS credentials..

Hi Trond-

On Apr 19, 2008, at 4:40 PM, Trond Myklebust wrote:
> ..and always destroy using a 'soft' RPC call. Destroying GSS
> credentials
> isn't mandatory; the server can always cope with a few credentials not
> getting destroyed in a timely fashion.

These two changes may have different side-effects, but backing out
this patch will revert both changes. Should these be split into
separate patches to facilitate git bisect?

> This actually fixes a hang situation. Basically, some servers will
> decide
> that the client is crazy if it tries to destroy an RPC context for
> which
> they have sent an RPCSEC_GSS_CREDPROBLEM, and so will refuse to talk
> to it
> for a while.

That seems unfriendly. Is this server-side behavior mandated? Why
wouldn't the server just treat an extraneous context destruction
request as a successful no-op?

You definitely should include an explanation of this server-side
behavior in the documenting comment for gss_destroying_context.

> The regression therefor probably was introduced by commit
> 0df7fb74fbb709591301871a38aac7735a1d6583.
>
> Signed-off-by: Trond Myklebust <[email protected]>
> ---
>
> net/sunrpc/auth_gss/auth_gss.c | 4 ++--
> 1 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/net/sunrpc/auth_gss/auth_gss.c b/net/sunrpc/auth_gss/
> auth_gss.c
> index d34f6df..55948cd 100644
> --- a/net/sunrpc/auth_gss/auth_gss.c
> +++ b/net/sunrpc/auth_gss/auth_gss.c
> @@ -710,7 +710,7 @@ gss_destroying_context(struct rpc_cred *cred)
> struct rpc_task *task;
>
> if (gss_cred->gc_ctx == NULL ||
> - gss_cred->gc_ctx->gc_proc == RPC_GSS_PROC_DESTROY)
> + test_and_clear_bit(RPCAUTH_CRED_UPTODATE, &cred->cr_flags) == 0)
> return 0;
>
> gss_cred->gc_ctx->gc_proc = RPC_GSS_PROC_DESTROY;
> @@ -720,7 +720,7 @@ gss_destroying_context(struct rpc_cred *cred)
> * by the RPC call or by the put_rpccred() below */
> get_rpccred(cred);
>
> - task = rpc_call_null(gss_auth->client, cred, RPC_TASK_ASYNC);
> + task = rpc_call_null(gss_auth->client, cred, RPC_TASK_ASYNC|
> RPC_TASK_SOFT);
> if (!IS_ERR(task))
> rpc_put_task(task);
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs"
> in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html

--
Chuck Lever
chuck[dot]lever[at]oracle[dot]com




2008-04-22 00:00:44

by Myklebust, Trond

[permalink] [raw]
Subject: Re: [PATCH 03/33] SUNRPC: Don't attempt to destroy expired RPCSEC_GSS credentials..


On Mon, 2008-04-21 at 13:50 -0400, Chuck Lever wrote:
> Hi Trond-
>
> On Apr 19, 2008, at 4:40 PM, Trond Myklebust wrote:
> > ..and always destroy using a 'soft' RPC call. Destroying GSS
> > credentials
> > isn't mandatory; the server can always cope with a few credentials not
> > getting destroyed in a timely fashion.
>
> These two changes may have different side-effects, but backing out
> this patch will revert both changes. Should these be split into
> separate patches to facilitate git bisect?

See the comment above. Destroying GSS sessions is _not_ mandatory. If
there are any side-effects from not destroying them, then that would be
a server bug, not a client issue.

> > This actually fixes a hang situation. Basically, some servers will
> > decide
> > that the client is crazy if it tries to destroy an RPC context for
> > which
> > they have sent an RPCSEC_GSS_CREDPROBLEM, and so will refuse to talk
> > to it
> > for a while.
>
> That seems unfriendly. Is this server-side behavior mandated? Why
> wouldn't the server just treat an extraneous context destruction
> request as a successful no-op?
>
> You definitely should include an explanation of this server-side
> behavior in the documenting comment for gss_destroying_context.

RFC-2203 states that servers are supposed to silently discard requests
that they don't recognise (see section 5.3.3.1 - Context Management), so
it is correct server behaviour.

--
Trond Myklebust
Linux NFS client maintainer

NetApp
[email protected]
http://www.netapp.com