[pnfs] cl_ex_cred questions
Benny Halevy
bhalevy at panasas.com
Thu Jun 5 08:19:54 EDT 2008
On Jun. 04, 2008, 18:18 +0300, "William A. (Andy) Adamson" <androsadamson at gmail.com> wrote:
> 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. :(
Hmm, your tree is probably missing the old
nfs41-draft-13-2.6.24-rc5-v0 tag...
>
>
>> 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.
In 2.6.26, where nfs4_get_setclientid_cred tries nfs4_get_machine_cred
first, it isn't.
>
>
>> 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.
So the machine_creds are available in 2.6.26.
>
>
>> 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.
So why not just use it, like this:
if (!status)
clp->cl_ex_cred = get_rpccred(msg.rpc_cred);
rather than calling rpcauth_lookupcred again?
>
> 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.
but rpcauth_lookupcred already takes a ref implicitly (eventually,
down in rpcauth_lookup_credcache).
>
>
>> 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.
the put balances the lookup, I think...
>
>
>> 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
Hmm, in 2.6.26 it's not exported any more, so I'm not sure how official
it is...
> the initial exchange_id is NULL, and therefore can't be used.
Right, but msg.cpc_cred is set in rpc_call_sync/rpc_run_task
and we can just get_rpccred it.
>
>
>> 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.
I see.
>
>
>> 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.
I prefer (and Ricardo agrees, on his reply) that the caller will put the
cred it got if it isn't NULL as it's more robust.
Actually, in 2.6.26 the caller needs the cred after calling
establish_clid so the callee must not dereference it.
>
>
>> 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
>>
>>
>
How about the follow patch?
(applies against 2.6.26, which I'll put on the public git tree
shortly (it's not fully stable yet but at least we could look
at the same code :)
notes:
* replacing cl_ex_cred rather than not stepping on it
seems better as it could be called in the recovery path
after renewal with the old cl_ex_creds in reclaimer()
already failed
diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index 7f62669..2cef88a 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -4339,28 +4339,9 @@ static int _nfs4_proc_exchange_id(struct nfs_client *clp, struct rpc_cred *cred)
break;
}
if (status == 0) {
- struct rpc_clnt *clnt = clp->cl_rpcclient;
-
- /* NFSv4.1 will use machine creds acquired at mount, and will
- * need to remember which creds were used for the EXCHANGE_ID.
- * Add an rpc cred pointer to struct nfs_client to hold
- * the EXCHANGE_ID cred, and enable AUTH_UNIX mounts.
- */
- if (clnt->cl_auth->au_flavor == RPC_AUTH_UNIX &&
- current->fsuid == 0) {
-
- dprintk("%s cl_ex_cred %p [NULL]\n", __func__,
- clp->cl_ex_cred);
-
- clp->cl_ex_cred = rpcauth_lookupcred(clnt->cl_auth, 0);
- atomic_inc(&clp->cl_ex_cred->cr_count);
-
- dprintk("%s set cl_ex_cred %p\n",
- __func__, clp->cl_ex_cred);
- } else {
- dprintk("%s not AUTH_SYS don't save EXCHANGE_ID cred\n",
- __func__);
- }
+ if (clp->cl_ex_cred)
+ put_rpccred(clp->cl_ex_cred);
+ clp->cl_ex_cred = get_rpccred(msg.rpc_cred);
}
dprintk("<-- %s status= %d\n", __func__, status);
diff --git a/fs/nfs/nfs4state.c b/fs/nfs/nfs4state.c
index 872504d..be996c6 100644
--- a/fs/nfs/nfs4state.c
+++ b/fs/nfs/nfs4state.c
@@ -73,7 +73,6 @@ int nfs4_init_clientid(struct nfs_client *clp, struct rpc_cred *cred)
status = nfs4_proc_setclientid_confirm(clp, cred);
if (status == 0)
nfs4_schedule_state_renewal(clp);
- put_rpccred(cred);
out:
return status;
}
@@ -970,6 +969,7 @@ static int reclaimer(void *ptr)
struct nfs4_state_owner *sp;
struct rb_node *pos;
struct nfs4_state_recovery_ops *ops;
+ struct nfs4_state_maintenance_ops *m_ops;
struct rpc_cred *cred;
int status = 0;
@@ -987,8 +987,9 @@ static int reclaimer(void *ptr)
restart_loop:
dprintk("%s: starting loop\n", __func__);
ops = nfs4_network_partition_recovery_ops[clp->cl_minorversion];
+ m_ops = nfs4_state_renewal_ops[clp->cl_minorversion];
/* Are there any open files on this volume? */
- cred = nfs4_get_renew_cred(clp);
+ cred = m_ops->get_state_renewal_cred(clp);
if (cred != NULL) {
/* Yes there are: try to renew the old lease */
status = ops->renew_lease(clp, cred);
@@ -1011,10 +1012,13 @@ restart_loop:
cred = nfs4_get_setclientid_cred(clp);
dprintk("%s: setclientid_cred %p\n", __func__, cred);
status = ops->establish_clid(clp, cred);
- /* Handle case where the user hasn't set up machine creds */
- if (status == -EACCES && cred == clp->cl_machine_cred) {
- nfs4_clear_machine_cred(clp);
- goto restart_loop;
+ if (cred) {
+ put_rpccred(cred);
+ /* Handle case where the user hasn't set up machine creds */
+ if (status == -EACCES && cred == clp->cl_machine_cred) {
+ nfs4_clear_machine_cred(clp);
+ goto restart_loop;
+ }
}
if (status)
goto out_error;
More information about the pNFS
mailing list