On Thu, Mar 13, 2008 at 01:48:08PM -0400, Trond Myklebust wrote:
> The current RPCAUTH_LOOKUP_ROOTCREDS flag only works for AUTH_SYS
> authentication, and then only as a special case in the code. This patch
> removes the auth_sys special casing, and replaces it with generic code.
>
> Signed-off-by: Trond Myklebust <[email protected]>
> ---
>
> include/linux/sunrpc/auth.h | 4 ++-
> net/sunrpc/auth.c | 35 +++++++++++++++------------
> net/sunrpc/auth_unix.c | 56 ++++++++++++++++++-------------------------
> net/sunrpc/sched.c | 4 ++-
> 4 files changed, 49 insertions(+), 50 deletions(-)
>
> diff --git a/include/linux/sunrpc/auth.h b/include/linux/sunrpc/auth.h
> index 84d5f3a..012566a 100644
> --- a/include/linux/sunrpc/auth.h
> +++ b/include/linux/sunrpc/auth.h
> @@ -89,7 +89,6 @@ struct rpc_auth {
>
> /* Flags for rpcauth_lookupcred() */
> #define RPCAUTH_LOOKUP_NEW 0x01 /* Accept an uninitialised cred */
> -#define RPCAUTH_LOOKUP_ROOTCREDS 0x02 /* This really ought to go! */
>
> /*
> * Client authentication ops
> @@ -136,7 +135,8 @@ void rpcauth_release(struct rpc_auth *);
> struct rpc_cred * rpcauth_lookup_credcache(struct rpc_auth *, struct auth_cred *, int);
> void rpcauth_init_cred(struct rpc_cred *, const struct auth_cred *, struct rpc_auth *, const struct rpc_credops *);
> struct rpc_cred * rpcauth_lookupcred(struct rpc_auth *, int);
> -struct rpc_cred * rpcauth_bindcred(struct rpc_task *);
> +void rpcauth_bindcred(struct rpc_task *);
> +void rpcauth_bind_root_cred(struct rpc_task *);
> void rpcauth_holdcred(struct rpc_task *);
> void put_rpccred(struct rpc_cred *);
> void rpcauth_unbindcred(struct rpc_task *);
> diff --git a/net/sunrpc/auth.c b/net/sunrpc/auth.c
> index b38f6ee..b0f2b2e 100644
> --- a/net/sunrpc/auth.c
> +++ b/net/sunrpc/auth.c
> @@ -285,9 +285,6 @@ rpcauth_lookup_credcache(struct rpc_auth *auth, struct auth_cred * acred,
>
> nr = hash_long(acred->uid, RPC_CREDCACHE_HASHBITS);
>
> - if (!(flags & RPCAUTH_LOOKUP_ROOTCREDS))
> - nr = acred->uid & RPC_CREDCACHE_MASK;
> -
> rcu_read_lock();
> hlist_for_each_entry_rcu(entry, pos, &cache->hashtable[nr], cr_hash) {
> if (!entry->cr_ops->crmatch(acred, entry, flags))
> @@ -378,30 +375,38 @@ rpcauth_init_cred(struct rpc_cred *cred, const struct auth_cred *acred,
> }
> EXPORT_SYMBOL_GPL(rpcauth_init_cred);
>
> -struct rpc_cred *
> -rpcauth_bindcred(struct rpc_task *task)
> +void
> +rpcauth_bind_root_cred(struct rpc_task *task)
> {
> struct rpc_auth *auth = task->tk_client->cl_auth;
> struct auth_cred acred = {
> - .uid = current->fsuid,
> - .gid = current->fsgid,
> - .group_info = current->group_info,
> + .uid = 0,
> + .gid = 0,
> };
> struct rpc_cred *ret;
> - int flags = 0;
>
> dprintk("RPC: %5u looking up %s cred\n",
> task->tk_pid, task->tk_client->cl_auth->au_ops->au_name);
> - get_group_info(acred.group_info);
> - if (task->tk_flags & RPC_TASK_ROOTCREDS)
> - flags |= RPCAUTH_LOOKUP_ROOTCREDS;
> - ret = auth->au_ops->lookup_cred(auth, &acred, flags);
> + ret = auth->au_ops->lookup_cred(auth, &acred, 0);
> + if (!IS_ERR(ret))
> + task->tk_msg.rpc_cred = ret;
> + else
> + task->tk_status = PTR_ERR(ret);
> +}
> +
> +void
> +rpcauth_bindcred(struct rpc_task *task)
> +{
> + struct rpc_auth *auth = task->tk_client->cl_auth;
> + struct rpc_cred *ret;
> +
> + dprintk("RPC: %5u looking up %s cred\n",
> + task->tk_pid, auth->au_ops->au_name);
> + ret = rpcauth_lookupcred(auth, 0);
> if (!IS_ERR(ret))
> task->tk_msg.rpc_cred = ret;
> else
> task->tk_status = PTR_ERR(ret);
> - put_group_info(acred.group_info);
> - return ret;
> }
>
> void
> diff --git a/net/sunrpc/auth_unix.c b/net/sunrpc/auth_unix.c
> index 5ed91e5..b763710 100644
> --- a/net/sunrpc/auth_unix.c
> +++ b/net/sunrpc/auth_unix.c
> @@ -60,7 +60,8 @@ static struct rpc_cred *
> unx_create_cred(struct rpc_auth *auth, struct auth_cred *acred, int flags)
> {
> struct unx_cred *cred;
> - int i;
> + unsigned int groups = 0;
> + unsigned int i;
I don't really care, I'm just curious: why bother to make small counter
variables unsigned?
>
> dprintk("RPC: allocating UNIX cred for uid %d gid %d\n",
> acred->uid, acred->gid);
> @@ -70,21 +71,17 @@ unx_create_cred(struct rpc_auth *auth, struct auth_cred *acred, int flags)
>
> rpcauth_init_cred(&cred->uc_base, acred, auth, &unix_credops);
> cred->uc_base.cr_flags = 1UL << RPCAUTH_CRED_UPTODATE;
> - if (flags & RPCAUTH_LOOKUP_ROOTCREDS) {
> - cred->uc_uid = 0;
> - cred->uc_gid = 0;
> - cred->uc_gids[0] = NOGROUP;
> - } else {
> - int groups = acred->group_info->ngroups;
> - if (groups > NFS_NGROUPS)
> - groups = NFS_NGROUPS;
> -
> - cred->uc_gid = acred->gid;
> - for (i = 0; i < groups; i++)
> - cred->uc_gids[i] = GROUP_AT(acred->group_info, i);
> - if (i < NFS_NGROUPS)
> - cred->uc_gids[i] = NOGROUP;
> - }
> +
> + if (acred->group_info != NULL)
> + groups = acred->group_info->ngroups;
> + if (groups > NFS_NGROUPS)
> + groups = NFS_NGROUPS;
> +
> + cred->uc_gid = acred->gid;
> + for (i = 0; i < groups; i++)
> + cred->uc_gids[i] = GROUP_AT(acred->group_info, i);
> + if (i < NFS_NGROUPS)
> + cred->uc_gids[i] = NOGROUP;
>
> return &cred->uc_base;
> }
> @@ -118,26 +115,21 @@ static int
> unx_match(struct auth_cred *acred, struct rpc_cred *rcred, int flags)
> {
> struct unx_cred *cred = container_of(rcred, struct unx_cred, uc_base);
> - int i;
> + unsigned int groups = 0;
> + unsigned int i;
>
> - if (!(flags & RPCAUTH_LOOKUP_ROOTCREDS)) {
> - int groups;
>
> - if (cred->uc_uid != acred->uid
> - || cred->uc_gid != acred->gid)
> - return 0;
> + if (cred->uc_uid != acred->uid || cred->uc_gid != acred->gid)
> + return 0;
>
> + if (acred->group_info != NULL)
> groups = acred->group_info->ngroups;
> - if (groups > NFS_NGROUPS)
> - groups = NFS_NGROUPS;
> - for (i = 0; i < groups ; i++)
> - if (cred->uc_gids[i] != GROUP_AT(acred->group_info, i))
> - return 0;
> - return 1;
> - }
> - return (cred->uc_uid == 0
> - && cred->uc_gid == 0
> - && cred->uc_gids[0] == (gid_t) NOGROUP);
> + if (groups > NFS_NGROUPS)
> + groups = NFS_NGROUPS;
> + for (i = 0; i < groups ; i++)
> + if (cred->uc_gids[i] != GROUP_AT(acred->group_info, i))
> + return 0;
> + return 1;
This changes the behavior slightly in the ROOTCREDS case so it no longer
expects uc_gids[0] to be NOGROUP. I assume that doesn't matter?
(Also, this is unchanged by the patch, but I'm curious whether there's
any particular reason it matches the supplementary groups in the way it
does: it expects them to agree up to the number of groups supplied in
acred, but ignores any others. Is this just arbitrary?)
--b.
> }
>
> /*
> diff --git a/net/sunrpc/sched.c b/net/sunrpc/sched.c
> index cae219c..7db956f 100644
> --- a/net/sunrpc/sched.c
> +++ b/net/sunrpc/sched.c
> @@ -821,8 +821,10 @@ static void rpc_init_task(struct rpc_task *task, const struct rpc_task_setup *ta
> /* Bind the user cred */
> if (task->tk_msg.rpc_cred != NULL)
> rpcauth_holdcred(task);
> - else
> + else if (!(task_setup_data->flags & RPC_TASK_ROOTCREDS))
> rpcauth_bindcred(task);
> + else
> + rpcauth_bind_root_cred(task);
> if (task->tk_action == NULL)
> rpc_call_start(task);
> }
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mar 28, 2008, at 11:32 AM, J. Bruce Fields wrote:
> On Thu, Mar 13, 2008 at 01:48:08PM -0400, Trond Myklebust wrote:
>> The current RPCAUTH_LOOKUP_ROOTCREDS flag only works for AUTH_SYS
>> authentication, and then only as a special case in the code. This
>> patch
>> removes the auth_sys special casing, and replaces it with generic
>> code.
>>
>> Signed-off-by: Trond Myklebust <[email protected]>
>> ---
>>
>> include/linux/sunrpc/auth.h | 4 ++-
>> net/sunrpc/auth.c | 35 +++++++++++++++------------
>> net/sunrpc/auth_unix.c | 56 +++++++++++++++++
>> +-------------------------
>> net/sunrpc/sched.c | 4 ++-
>> 4 files changed, 49 insertions(+), 50 deletions(-)
>>
>> diff --git a/include/linux/sunrpc/auth.h b/include/linux/sunrpc/
>> auth.h
>> index 84d5f3a..012566a 100644
>> --- a/include/linux/sunrpc/auth.h
>> +++ b/include/linux/sunrpc/auth.h
>> @@ -89,7 +89,6 @@ struct rpc_auth {
>>
>> /* Flags for rpcauth_lookupcred() */
>> #define RPCAUTH_LOOKUP_NEW 0x01 /* Accept an uninitialised cred */
>> -#define RPCAUTH_LOOKUP_ROOTCREDS 0x02 /* This really ought to go! */
>>
>> /*
>> * Client authentication ops
>> @@ -136,7 +135,8 @@ void rpcauth_release(struct rpc_auth *);
>> struct rpc_cred * rpcauth_lookup_credcache(struct rpc_auth *,
>> struct auth_cred *, int);
>> void rpcauth_init_cred(struct rpc_cred *, const struct
>> auth_cred *, struct rpc_auth *, const struct rpc_credops *);
>> struct rpc_cred * rpcauth_lookupcred(struct rpc_auth *, int);
>> -struct rpc_cred * rpcauth_bindcred(struct rpc_task *);
>> +void rpcauth_bindcred(struct rpc_task *);
>> +void rpcauth_bind_root_cred(struct rpc_task *);
>> void rpcauth_holdcred(struct rpc_task *);
>> void put_rpccred(struct rpc_cred *);
>> void rpcauth_unbindcred(struct rpc_task *);
>> diff --git a/net/sunrpc/auth.c b/net/sunrpc/auth.c
>> index b38f6ee..b0f2b2e 100644
>> --- a/net/sunrpc/auth.c
>> +++ b/net/sunrpc/auth.c
>> @@ -285,9 +285,6 @@ rpcauth_lookup_credcache(struct rpc_auth
>> *auth, struct auth_cred * acred,
>>
>> nr = hash_long(acred->uid, RPC_CREDCACHE_HASHBITS);
>>
>> - if (!(flags & RPCAUTH_LOOKUP_ROOTCREDS))
>> - nr = acred->uid & RPC_CREDCACHE_MASK;
>> -
>> rcu_read_lock();
>> hlist_for_each_entry_rcu(entry, pos, &cache->hashtable[nr],
>> cr_hash) {
>> if (!entry->cr_ops->crmatch(acred, entry, flags))
>> @@ -378,30 +375,38 @@ rpcauth_init_cred(struct rpc_cred *cred,
>> const struct auth_cred *acred,
>> }
>> EXPORT_SYMBOL_GPL(rpcauth_init_cred);
>>
>> -struct rpc_cred *
>> -rpcauth_bindcred(struct rpc_task *task)
>> +void
>> +rpcauth_bind_root_cred(struct rpc_task *task)
>> {
>> struct rpc_auth *auth = task->tk_client->cl_auth;
>> struct auth_cred acred = {
>> - .uid = current->fsuid,
>> - .gid = current->fsgid,
>> - .group_info = current->group_info,
>> + .uid = 0,
>> + .gid = 0,
>> };
>> struct rpc_cred *ret;
>> - int flags = 0;
>>
>> dprintk("RPC: %5u looking up %s cred\n",
>> task->tk_pid, task->tk_client->cl_auth->au_ops->au_name);
>> - get_group_info(acred.group_info);
>> - if (task->tk_flags & RPC_TASK_ROOTCREDS)
>> - flags |= RPCAUTH_LOOKUP_ROOTCREDS;
>> - ret = auth->au_ops->lookup_cred(auth, &acred, flags);
>> + ret = auth->au_ops->lookup_cred(auth, &acred, 0);
>> + if (!IS_ERR(ret))
>> + task->tk_msg.rpc_cred = ret;
>> + else
>> + task->tk_status = PTR_ERR(ret);
>> +}
>> +
>> +void
>> +rpcauth_bindcred(struct rpc_task *task)
>> +{
>> + struct rpc_auth *auth = task->tk_client->cl_auth;
>> + struct rpc_cred *ret;
>> +
>> + dprintk("RPC: %5u looking up %s cred\n",
>> + task->tk_pid, auth->au_ops->au_name);
>> + ret = rpcauth_lookupcred(auth, 0);
>> if (!IS_ERR(ret))
>> task->tk_msg.rpc_cred = ret;
>> else
>> task->tk_status = PTR_ERR(ret);
>> - put_group_info(acred.group_info);
>> - return ret;
>> }
>>
>> void
>> diff --git a/net/sunrpc/auth_unix.c b/net/sunrpc/auth_unix.c
>> index 5ed91e5..b763710 100644
>> --- a/net/sunrpc/auth_unix.c
>> +++ b/net/sunrpc/auth_unix.c
>> @@ -60,7 +60,8 @@ static struct rpc_cred *
>> unx_create_cred(struct rpc_auth *auth, struct auth_cred *acred,
>> int flags)
>> {
>> struct unx_cred *cred;
>> - int i;
>> + unsigned int groups = 0;
>> + unsigned int i;
>
> I don't really care, I'm just curious: why bother to make small
> counter
> variables unsigned?
If the counter is being compared to an unsigned somewhere, it's
cleaner to make the counter unsigned as well.
--
Chuck Lever
chuck[dot]lever[at]oracle[dot]com