[pnfs] cl_ex_cred questions

Wed Jun 4 11:18:48 EDT 2008

Hi Benny

Good review. Comments and an attached patch that I think fixes the
situation...

-->Andy

On Wed, Jun 4, 2008 at 7:44 AM, Benny Halevy <bhalevy at panasas.com> wrote:

> Hey Andy and Ricardo,
>
> While porting to 2.6.26 I looked into the renewal cred mechanism,
> originally introduced in these commits:
> c7f59a0f "nfs41: sessions exchange_id AUTH_SYS root cred"

OK, I  find this  commit in the current origin/nfs41 tree.

> c55e803a "nfs41: Move NULL credential checking from the reclaimer into
> nfs4_init_clientid()."

But I can't find this commit in the current origin/nfs41 tree. :(

> One problem I saw is that there seems to be a leak of the cred
> the caller passes establish_id().
>
> Previously, reclaimer() passed the creds down to nfs4_init_client
> and then dereferenced the cred using put_rpccred.
>
> Then, commit c55e803a moved the responsibility to call put_rpccred
> from the caller to the callee, via ops->establish_clid. *But*, only
> nfs4_init_clientid is doing that, not _nfs4_proc_exchange_id.

Yes for the renewal case. nfs4_proc_exchange_id differs from
nfs4_init_clientid in that the cred passed in is NULL in the initial case
(the non-renewal case). I'll address the renewal case below.

The cred is NULL in the initial case because the reclaimer calls two
functions to try and get a
cred for establish_clid ,  nfs4_get_renew_cred and
nfs4_get_setclientid_cred; both of these
functions search the struct nfs_client cl_state_owners list which is empty
in the case of exchange_id
and requires at least one cred in the case of nfs4_initclientid which calls
setclient_id which by design is not called until after the first OPEN.

>
> I think this is wrongly coded and the caller should put the creds
> it got.  The callee can always take a ref count on the cred if
> it wishes to keep it (and using get_rpccred, not by atomic_inc of
> cr_count as currently coded).

In the initial case where the cred passed into nfs4_proc_exchange_id is
NULL, the rpc layer looks up the cred for the UID, gets/puts_rpccred.

In the renewal case, once we fix it (see below!), you are correct. we will
need to do a put_rpccred in the case when the cred passed to
nfs4_proc_exchange_id is not NULL.

>
> Second question is what creds should exchange_id use?

The creds that were used to mount - e.g. the creds mapped to UID 0.  In the
AUTH_GSS case, I believe this means machine creds are required.  mountd will
need to pass down AUTH_GSS creds (or the location of the creds via the
keyring) that map to UID 0. Until we have this functionality , only
AUTH_UNIX root credentials are supported.

>
> and what creds it should keep for renewal?

If by renewal you mean re-establishing a clientid via exchange_id, the same
creds - those used to mount.

Currently, it always uses the creds it received from the caller
> to send the exchange_id rpc

The  rpc_cred pointer passed to  _nfs4_proc_exchang_id is NULL in the
initial case.  The rpc code sees the NULL cred, and calls
rpcauth_lookup_cred using the current UID which is 0 due to the fact that
this is the result of a mount system call. This will also work with
AUTH_GSS, since the UID 0 will be mapped to machine credentials.

and if it succeeded and
> clnt->cl_auth->au_flavor == RPC_AUTH_UNIX && current->fsuid == 0
> it gets a new set of creds using rpcauth_lookupcred.

Right. This code set up the 'renewal' cred for future re-establishment of
the clientid - e.g. resend of an exchange_id. When AUTH_GSS is supported,
the RPC_AUTH_UNIX check will be removed.

> Note that we take an extra ref count on these creds

nope. we get a count because we are keeping them around for renewal.

> and that seems to be yet another leak since it's being
> put only once in nfs_free_client...

which is the put that balances the get. so there is no leak here.

>
> Why do we need to call rpcauth_lookupcred(), why not
> just keep the creds used for the successful exchange_id?

 rpcauth_lookupcred looks up the cached credential mapped to the UID. It's
just the official way to do the job. Note that the cred pointer passed into
the initial exchange_id is NULL, and therefore can't be used.

>
> What happens if cl_ex_cred is not NULL on entry to
> _nfs4_proc_exchange_id()?  Can't this happen?

yes! But only for the nfs4_schedule_state_renewal() path (e.g. RENEWD) which
I'm not sure is even called for v4.1. The  nfs4_schedule_state_recovery()
path is indeed broken. Good catch!

Here is where the commit  c7f59a0f "nfs41:sessions exchange_id AUTH_SYS root
cred" had it right. it
replaced the nfs4_renew_state call to nfs4_get_state_renewal with a new
nfs4_state_maintenance_ops operation called get_state_renewal_cred which was
set to nfs4_get_state_renewal for v4.0 and to nfs41_get_state_renewal shown
below for v4.1.

+
+struct rpc_cred *
+nfs41_get_state_renewal_cred(struct nfs_client *clp)
+{
+    if (clp->cl_ex_cred)
+        get_rpccred(clp->cl_ex_cred);
+    return (clp->cl_ex_cred);
+}
+#endif /* CONFIG_NFS_V4_1 */

 This change left all of the 'initial case' described above the same. In the
renewal case, the cl_ex_cred exists, and is passed into
nfs4_proc_exchange_id via the establish_clid operation.

But! this was only done for nfs4_renew_state(), not for the reclaimer. The
reclaimer is called via nfs4_schedule_state_recovery() which reclaims
delegations, locks, opens and is therefore needed by v4.1 as well.

 So we need to replace the call to nfs4_get_renewal_state in the reclaimer
with a call to ops->get_renewal_state.

> Shouldn't we reuse it for sending the exchange_id?

With the above change, we will. We will also (as noted above) need to call
put_rpccred in this case.

> Also, we set cl_ex_cred unconditionally. If it wasn't
> null, we step on the previous value and YAL (yet another leak ;-)

This i agree with. we should not step on the previous value of cl_ex_cred.

>
> Last question is in case the auth_sys condition is false,

> cl_ex_creds may be left as NULL. Then what?
> get_state_renewal_cred will just return NULL and we might
> get into an endless renewal loop.

If the mount UID 0 doesn't map to any credential, the initial exchange_id
will fail. No infinite loop.

-->Andy

>
> Benny
> _______________________________________________
> pNFS mailing list
> pNFS at linux-nfs.org
> http://linux-nfs.org/cgi-bin/mailman/listinfo/pnfs
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://linux-nfs.org/pipermail/pnfs/attachments/20080604/4434485f/attachment-0001.htm 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-nfs41-fix-nfs4-state-recovery-credential.patch
Type: text/x-patch
Size: 2268 bytes
Desc: not available
Url : http://linux-nfs.org/pipermail/pnfs/attachments/20080604/4434485f/attachment-0001.bin 