Re: possible module refcount leak with auth_gss

["J. Bruce Fields" <bfields@fieldses.org>]

On Tue, Dec 16, 2008 at 09:40:51PM -0500, Jeff Layton wrote:
> On Tue, 16 Dec 2008 16:45:32 -0500
> Jeff Layton <jlayton@redhat.com> wrote:
> > I took some time today to dig into this. Here's some debug output from
> > the RPC_GSS_PROC_DESTROY request on the client:
> > 
> > RPC:     0 holding RPCSEC_GSS cred ffff880011579bb8
> > RPC:       new task initialized, procpid 2846
> > RPC:       allocated task ffff88001e110000
> > RPC:    11 __rpc_execute flags=0x281
> > RPC:    11 call_start nfs4 proc NULL (async)
> > RPC:    11 call_reserve (status 0)
> > RPC:    11 call_refresh (status 0)
> > RPC:    11 refreshing RPCSEC_GSS cred ffff880011579bb8
> > RPC:    11 call_refreshresult (status -13)
> > RPC:    11 return 0, status -13
> > RPC:    11 release task
> > RPC:    11 releasing RPCSEC_GSS cred ffff880011579bb8
> > 
> > ...looks like we're trying to refresh the credentials before sending the
> > call but that's failing. That's as far as I've gotten with it so far
> > though.
> > 
> 
> I see why this is occuring, but I'm afraid I don't understand the code
> well enough to understand why it's written this way...
> 
> gss_destroying_context() does this:
> 
>         if (gss_cred->gc_ctx == NULL ||
>             test_and_clear_bit(RPCAUTH_CRED_UPTODATE, &cred->cr_flags) == 0)
>                 return 0;
> 
>         gss_cred->gc_ctx->gc_proc = RPC_GSS_PROC_DESTROY;
>         cred->cr_ops = &gss_nullops;
> 
> ...and then it does the rpc NULL call. The problem is that we're clearing
> RPCAUTH_CRED_UPTODATE and then setting cr_ops to gss_nullops. The rpc
> call is done, call_reserve checks the RPCAUTH_CRED_UPTODATE bit, and calls
> call_refresh because it's not set.
> 
> call_refresh calls rpcauth_refreshcred which calls the crrefresh op. That's
> now gss_refresh_null, and that just returns -EACCES. The RPC call then
> fails w/o ever being sent.
> 
> It seems like we either need to not clear the UPTODATE bit for the cred, or
> not set the cr_ops to the nullops before doing the call. Any thoughts?

We certainly shouldn't be really refreshing the cred--that'd end up
creating a new gss context when what we're trying to do is destroy one.
So leaving cr_ops set to gss_credops() doesn't sound right.

Maybe that gss_refresh_null() should just return 0 and pretend the
cred's fine, instead of returning -EACCES?

--b.