[pnfs] cl_ex_cred questions
Ricardo Labiaga
ricardo.labiaga at netapp.com
Wed Jun 4 21:40:36 EDT 2008
On Wed, 2008-06-04 at 11:18 -0400, William A. (Andy) Adamson 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. :(
>
>
> 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).
>
I agree, I rather have the function caller undo references that it
obtains itself, instead of having them released obscurely by its
callees. That's always been my feeling, so I'm not sure why I didn't
apply it in this commit. Ugh, apologies...
>
> 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!
I'm not following... nfs4_proc_create_session() indeed calls
nfs4_schedule_state_renewal in the v4.1 path. Otherwise we wouldn't see
the sequence calls sent over and over. Right?
- ricardo
>
> 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
>
>
More information about the pNFS
mailing list