[pnfs] cl_ex_cred questions
Benny Halevy
bhalevy at panasas.com
Thu Jun 5 09:49:15 EDT 2008
On Jun. 05, 2008, 16:27 +0300, "William A. (Andy) Adamson" <androsadamson at gmail.com> wrote:
> On Thu, Jun 5, 2008 at 8:19 AM, Benny Halevy <bhalevy at panasas.com> wrote:
>
>> 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?
>
>
> because in the case where the _nfs4_proc_exchange_id cred parameter is NULL,
> the rpc layer did a get, then a put on the cred, so there is no explicit
> hold on the msg.rpc_cred at this point.
Yeah, got it.
Benny
>
>
>>
>>> 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