[22/23] SUNRPC: simplify auth_unix.
diff mbox series

Message ID 154156395162.24086.1172828418426764708.stgit@noble
State Superseded
Headers show
Series
  • NFS: Remove generic RPC credentials.
Related show

Commit Message

NeilBrown Nov. 7, 2018, 4:12 a.m. UTC
1/ discard 'struct unx_cred'.  We don't need any data that
   is not already in 'struct rpc_cred'.
2/ Don't keep these creds in a hash table.  When a credential
   is needed, simply allocate it.  When not needed, discard it.
   This can easily be faster than performing a lookup on
   a shared hash table.
   As the lookup can happen during write-out, use a mempool
   to ensure forward progress.
   This means that we cannot compare two credentials for
   equality by comparing the pointers, but we never do that anyway.

Signed-off-by: NeilBrown <neilb@suse.com>
---
 net/sunrpc/auth.c      |    1 
 net/sunrpc/auth_unix.c |  101 +++++++++++++++---------------------------------
 2 files changed, 32 insertions(+), 70 deletions(-)

Comments

Chuck Lever Nov. 7, 2018, 3:19 p.m. UTC | #1
Hi Neil-


> On Nov 6, 2018, at 11:12 PM, NeilBrown <neilb@suse.com> wrote:
> 
> 1/ discard 'struct unx_cred'.  We don't need any data that
>   is not already in 'struct rpc_cred'.
> 2/ Don't keep these creds in a hash table.  When a credential
>   is needed, simply allocate it.  When not needed, discard it.
>   This can easily be faster than performing a lookup on
>   a shared hash table.

What's the basis for this claim? A memory allocation disables and
enables IRQs. That definitely hits a resource that is globally
shared.

In addition, the comment near unx_marshal suggests we should
cache the marshaled on-the-wire version of the credential instead
of building it in the RPC Call buffer every time. That would
require keeping the creds around.

Have you measured a significant difference in throughput with
this patch? Have you considered improving the lookup speed of
the hash table by making the buckets into rb-trees, for example?


>   As the lookup can happen during write-out, use a mempool
>   to ensure forward progress.
>   This means that we cannot compare two credentials for
>   equality by comparing the pointers, but we never do that anyway.
> 
> Signed-off-by: NeilBrown <neilb@suse.com>
> ---
> net/sunrpc/auth.c      |    1 
> net/sunrpc/auth_unix.c |  101 +++++++++++++++---------------------------------
> 2 files changed, 32 insertions(+), 70 deletions(-)
> 
> diff --git a/net/sunrpc/auth.c b/net/sunrpc/auth.c
> index 867ea9834bde..a07a7c59d3a4 100644
> --- a/net/sunrpc/auth.c
> +++ b/net/sunrpc/auth.c
> @@ -651,6 +651,7 @@ rpcauth_init_cred(struct rpc_cred *cred, const struct auth_cred *acred,
> 	INIT_LIST_HEAD(&cred->cr_lru);
> 	refcount_set(&cred->cr_count, 1);
> 	cred->cr_auth = auth;
> +	cred->cr_flags = 0;
> 	cred->cr_ops = ops;
> 	cred->cr_expire = jiffies;
> 	cred->cr_cred = get_cred(acred->cred);
> diff --git a/net/sunrpc/auth_unix.c b/net/sunrpc/auth_unix.c
> index bff113a411e0..387f6b3ffbea 100644
> --- a/net/sunrpc/auth_unix.c
> +++ b/net/sunrpc/auth_unix.c
> @@ -11,16 +11,11 @@
> #include <linux/types.h>
> #include <linux/sched.h>
> #include <linux/module.h>
> +#include <linux/mempool.h>
> #include <linux/sunrpc/clnt.h>
> #include <linux/sunrpc/auth.h>
> #include <linux/user_namespace.h>
> 
> -struct unx_cred {
> -	struct rpc_cred		uc_base;
> -	kgid_t			uc_gid;
> -	kgid_t			uc_gids[UNX_NGROUPS];
> -};
> -#define uc_uid			uc_base.cr_uid
> 
> #if IS_ENABLED(CONFIG_SUNRPC_DEBUG)
> # define RPCDBG_FACILITY	RPCDBG_AUTH
> @@ -28,6 +23,7 @@ struct unx_cred {
> 
> static struct rpc_auth		unix_auth;
> static const struct rpc_credops	unix_credops;
> +static mempool_t		*unix_pool;
> 
> static struct rpc_auth *
> unx_create(const struct rpc_auth_create_args *args, struct rpc_clnt *clnt)
> @@ -42,15 +38,6 @@ static void
> unx_destroy(struct rpc_auth *auth)
> {
> 	dprintk("RPC:       destroying UNIX authenticator %p\n", auth);
> -	rpcauth_clear_credcache(auth->au_credcache);
> -}
> -
> -static int
> -unx_hash_cred(struct auth_cred *acred, unsigned int hashbits)
> -{
> -	return hash_64(from_kgid(&init_user_ns, acred->cred->fsgid) |
> -		((u64)from_kuid(&init_user_ns, acred->cred->fsuid) <<
> -			(sizeof(gid_t) * 8)), hashbits);
> }
> 
> /*
> @@ -59,53 +46,24 @@ unx_hash_cred(struct auth_cred *acred, unsigned int hashbits)
> static struct rpc_cred *
> unx_lookup_cred(struct rpc_auth *auth, struct auth_cred *acred, int flags)
> {
> -	return rpcauth_lookup_credcache(auth, acred, flags, GFP_NOFS);
> -}
> -
> -static struct rpc_cred *
> -unx_create_cred(struct rpc_auth *auth, struct auth_cred *acred, int flags, gfp_t gfp)
> -{
> -	struct unx_cred	*cred;
> -	unsigned int groups = 0;
> -	unsigned int i;
> +	struct rpc_cred *ret = mempool_alloc(unix_pool, GFP_NOFS);
> 
> 	dprintk("RPC:       allocating UNIX cred for uid %d gid %d\n",
> 			from_kuid(&init_user_ns, acred->cred->fsuid),
> 			from_kgid(&init_user_ns, acred->cred->fsgid));
> 
> -	if (!(cred = kmalloc(sizeof(*cred), gfp)))
> -		return ERR_PTR(-ENOMEM);
> -
> -	rpcauth_init_cred(&cred->uc_base, acred, auth, &unix_credops);
> -	cred->uc_base.cr_flags = 1UL << RPCAUTH_CRED_UPTODATE;
> -
> -	if (acred->cred && acred->cred->group_info != NULL)
> -		groups = acred->cred->group_info->ngroups;
> -	if (groups > UNX_NGROUPS)
> -		groups = UNX_NGROUPS;
> -
> -	cred->uc_gid = acred->cred->fsgid;
> -	for (i = 0; i < groups; i++)
> -		cred->uc_gids[i] = acred->cred->group_info->gid[i];
> -	if (i < UNX_NGROUPS)
> -		cred->uc_gids[i] = INVALID_GID;
> -
> -	return &cred->uc_base;
> -}
> -
> -static void
> -unx_free_cred(struct unx_cred *unx_cred)
> -{
> -	dprintk("RPC:       unx_free_cred %p\n", unx_cred);
> -	put_cred(unx_cred->uc_base.cr_cred);
> -	kfree(unx_cred);
> +	rpcauth_init_cred(ret, acred, auth, &unix_credops);
> +	ret->cr_flags = 1UL << RPCAUTH_CRED_UPTODATE;
> +	return ret;
> }
> 
> static void
> unx_free_cred_callback(struct rcu_head *head)
> {
> -	struct unx_cred *unx_cred = container_of(head, struct unx_cred, uc_base.cr_rcu);
> -	unx_free_cred(unx_cred);
> +	struct rpc_cred *rpc_cred = container_of(head, struct rpc_cred, cr_rcu);
> +	dprintk("RPC:       unx_free_cred %p\n", rpc_cred);
> +	put_cred(rpc_cred->cr_cred);
> +	mempool_free(rpc_cred, unix_pool);
> }
> 
> static void
> @@ -115,30 +73,32 @@ unx_destroy_cred(struct rpc_cred *cred)
> }
> 
> /*
> - * Match credentials against current process creds.
> - * The root_override argument takes care of cases where the caller may
> - * request root creds (e.g. for NFS swapping).
> + * Match credentials against current the auth_cred.
>  */
> static int
> -unx_match(struct auth_cred *acred, struct rpc_cred *rcred, int flags)
> +unx_match(struct auth_cred *acred, struct rpc_cred *cred, int flags)
> {
> -	struct unx_cred	*cred = container_of(rcred, struct unx_cred, uc_base);
> 	unsigned int groups = 0;
> 	unsigned int i;
> 
> +	if (cred->cr_cred == acred->cred)
> +		return 1;
> 
> -	if (!uid_eq(cred->uc_uid, acred->cred->fsuid) || !gid_eq(cred->uc_gid, acred->cred->fsgid))
> +	if (!uid_eq(cred->cr_cred->fsuid, acred->cred->fsuid) || !gid_eq(cred->cr_cred->fsgid, acred->cred->fsgid))
> 		return 0;
> 
> 	if (acred->cred && acred->cred->group_info != NULL)
> 		groups = acred->cred->group_info->ngroups;
> 	if (groups > UNX_NGROUPS)
> 		groups = UNX_NGROUPS;
> +	if (cred->cr_cred->group_info == NULL)
> +		return groups == 0;
> +	if (groups != cred->cr_cred->group_info->ngroups)
> +		return 0;
> +
> 	for (i = 0; i < groups ; i++)
> -		if (!gid_eq(cred->uc_gids[i], acred->cred->group_info->gid[i]))
> +		if (!gid_eq(cred->cr_cred->group_info->gid[i], acred->cred->group_info->gid[i]))
> 			return 0;
> -	if (groups < UNX_NGROUPS && gid_valid(cred->uc_gids[groups]))
> -		return 0;
> 	return 1;
> }
> 
> @@ -150,9 +110,10 @@ static __be32 *
> unx_marshal(struct rpc_task *task, __be32 *p)
> {
> 	struct rpc_clnt	*clnt = task->tk_client;
> -	struct unx_cred	*cred = container_of(task->tk_rqstp->rq_cred, struct unx_cred, uc_base);
> +	struct rpc_cred	*cred = task->tk_rqstp->rq_cred;
> 	__be32		*base, *hold;
> 	int		i;
> +	struct group_info *gi = cred->cr_cred->group_info;
> 
> 	*p++ = htonl(RPC_AUTH_UNIX);
> 	base = p++;
> @@ -163,11 +124,12 @@ unx_marshal(struct rpc_task *task, __be32 *p)
> 	 */
> 	p = xdr_encode_array(p, clnt->cl_nodename, clnt->cl_nodelen);
> 
> -	*p++ = htonl((u32) from_kuid(&init_user_ns, cred->uc_uid));
> -	*p++ = htonl((u32) from_kgid(&init_user_ns, cred->uc_gid));
> +	*p++ = htonl((u32) from_kuid(&init_user_ns, cred->cr_cred->fsuid));
> +	*p++ = htonl((u32) from_kgid(&init_user_ns, cred->cr_cred->fsgid));
> 	hold = p++;
> -	for (i = 0; i < UNX_NGROUPS && gid_valid(cred->uc_gids[i]); i++)
> -		*p++ = htonl((u32) from_kgid(&init_user_ns, cred->uc_gids[i]));
> +	if (gi)
> +		for (i = 0; i < UNX_NGROUPS && i < gi->ngroups; i++)
> +			*p++ = htonl((u32) from_kgid(&init_user_ns, gi->gid[i]));
> 	*hold = htonl(p - hold - 1);		/* gid array length */
> 	*base = htonl((p - base - 1) << 2);	/* cred length */
> 
> @@ -214,12 +176,13 @@ unx_validate(struct rpc_task *task, __be32 *p)
> 
> int __init rpc_init_authunix(void)
> {
> -	return rpcauth_init_credcache(&unix_auth);
> +	unix_pool = mempool_create_kmalloc_pool(16, sizeof(struct rpc_cred));
> +	return unix_pool ? 0 : -ENOMEM;
> }
> 
> void rpc_destroy_authunix(void)
> {
> -	rpcauth_destroy_credcache(&unix_auth);
> +	mempool_destroy(unix_pool);
> }
> 
> const struct rpc_authops authunix_ops = {
> @@ -228,9 +191,7 @@ const struct rpc_authops authunix_ops = {
> 	.au_name	= "UNIX",
> 	.create		= unx_create,
> 	.destroy	= unx_destroy,
> -	.hash_cred	= unx_hash_cred,
> 	.lookup_cred	= unx_lookup_cred,
> -	.crcreate	= unx_create_cred,
> };
> 
> static
> 
> 

--
Chuck Lever
NeilBrown Nov. 8, 2018, 1:41 a.m. UTC | #2
On Wed, Nov 07 2018, Chuck Lever wrote:

> Hi Neil-
>
>
>> On Nov 6, 2018, at 11:12 PM, NeilBrown <neilb@suse.com> wrote:
>> 
>> 1/ discard 'struct unx_cred'.  We don't need any data that
>>   is not already in 'struct rpc_cred'.
>> 2/ Don't keep these creds in a hash table.  When a credential
>>   is needed, simply allocate it.  When not needed, discard it.
>>   This can easily be faster than performing a lookup on
>>   a shared hash table.

Thanks for the review Chuck!

>
> What's the basis for this claim? A memory allocation disables and
> enables IRQs. That definitely hits a resource that is globally
> shared.

My basis is not rock solid, but I was convinced :-)

kmem_cache_alloc() does disable local irqs when slab.c is used.
slub.c doesn't disable them in the fast path which I *think* should be
reasonably common.
slob always takes a spinlock as well as disabling interrupts.

I think slob is only recommended for tiny machines, and slub is
generally preferred, so I think that when performance matters, it will
still be delivered.

It isn't clear to me why you consider a local irq to be "globally
shared" - assuming that is what you mean.
Disabling local interrupts is not without cost, but I don't think the
cost increases with the number of CPUs, while the cost of accessing
shared memory (even without a spinlock) does.

>
> In addition, the comment near unx_marshal suggests we should
> cache the marshaled on-the-wire version of the credential instead
> of building it in the RPC Call buffer every time. That would
> require keeping the creds around.

That comment has been there since 2.1.32 and has not be acted on.  There
seems little reason to expect that to change.  Caching doesn't seem to
have been found to be necessary in practice.

>
> Have you measured a significant difference in throughput with
> this patch? Have you considered improving the lookup speed of
> the hash table by making the buckets into rb-trees, for example?

No, I haven't measured.
I might have briefly considered changing to an rb-tree, but as the
current hashtable doesn't actually contain anything of value, I would
have quickly discarded the idea.

If I wanted to further improve performance, I would look at ways to
bypass the "lookup_cred" step completely.
unx_marshal only needs the generic "struct cred", so there should be no
need to have a 'struct rpc_cred' at all.

If this series is accepted, I'll (hopefully) look into seeing how
practical that is.

Thanks again,
NeilBrown


>
>
>>   As the lookup can happen during write-out, use a mempool
>>   to ensure forward progress.
>>   This means that we cannot compare two credentials for
>>   equality by comparing the pointers, but we never do that anyway.
>> 
>> Signed-off-by: NeilBrown <neilb@suse.com>
>> ---
>> net/sunrpc/auth.c      |    1 
>> net/sunrpc/auth_unix.c |  101 +++++++++++++++---------------------------------
>> 2 files changed, 32 insertions(+), 70 deletions(-)
>> 
>> diff --git a/net/sunrpc/auth.c b/net/sunrpc/auth.c
>> index 867ea9834bde..a07a7c59d3a4 100644
>> --- a/net/sunrpc/auth.c
>> +++ b/net/sunrpc/auth.c
>> @@ -651,6 +651,7 @@ rpcauth_init_cred(struct rpc_cred *cred, const struct auth_cred *acred,
>> 	INIT_LIST_HEAD(&cred->cr_lru);
>> 	refcount_set(&cred->cr_count, 1);
>> 	cred->cr_auth = auth;
>> +	cred->cr_flags = 0;
>> 	cred->cr_ops = ops;
>> 	cred->cr_expire = jiffies;
>> 	cred->cr_cred = get_cred(acred->cred);
>> diff --git a/net/sunrpc/auth_unix.c b/net/sunrpc/auth_unix.c
>> index bff113a411e0..387f6b3ffbea 100644
>> --- a/net/sunrpc/auth_unix.c
>> +++ b/net/sunrpc/auth_unix.c
>> @@ -11,16 +11,11 @@
>> #include <linux/types.h>
>> #include <linux/sched.h>
>> #include <linux/module.h>
>> +#include <linux/mempool.h>
>> #include <linux/sunrpc/clnt.h>
>> #include <linux/sunrpc/auth.h>
>> #include <linux/user_namespace.h>
>> 
>> -struct unx_cred {
>> -	struct rpc_cred		uc_base;
>> -	kgid_t			uc_gid;
>> -	kgid_t			uc_gids[UNX_NGROUPS];
>> -};
>> -#define uc_uid			uc_base.cr_uid
>> 
>> #if IS_ENABLED(CONFIG_SUNRPC_DEBUG)
>> # define RPCDBG_FACILITY	RPCDBG_AUTH
>> @@ -28,6 +23,7 @@ struct unx_cred {
>> 
>> static struct rpc_auth		unix_auth;
>> static const struct rpc_credops	unix_credops;
>> +static mempool_t		*unix_pool;
>> 
>> static struct rpc_auth *
>> unx_create(const struct rpc_auth_create_args *args, struct rpc_clnt *clnt)
>> @@ -42,15 +38,6 @@ static void
>> unx_destroy(struct rpc_auth *auth)
>> {
>> 	dprintk("RPC:       destroying UNIX authenticator %p\n", auth);
>> -	rpcauth_clear_credcache(auth->au_credcache);
>> -}
>> -
>> -static int
>> -unx_hash_cred(struct auth_cred *acred, unsigned int hashbits)
>> -{
>> -	return hash_64(from_kgid(&init_user_ns, acred->cred->fsgid) |
>> -		((u64)from_kuid(&init_user_ns, acred->cred->fsuid) <<
>> -			(sizeof(gid_t) * 8)), hashbits);
>> }
>> 
>> /*
>> @@ -59,53 +46,24 @@ unx_hash_cred(struct auth_cred *acred, unsigned int hashbits)
>> static struct rpc_cred *
>> unx_lookup_cred(struct rpc_auth *auth, struct auth_cred *acred, int flags)
>> {
>> -	return rpcauth_lookup_credcache(auth, acred, flags, GFP_NOFS);
>> -}
>> -
>> -static struct rpc_cred *
>> -unx_create_cred(struct rpc_auth *auth, struct auth_cred *acred, int flags, gfp_t gfp)
>> -{
>> -	struct unx_cred	*cred;
>> -	unsigned int groups = 0;
>> -	unsigned int i;
>> +	struct rpc_cred *ret = mempool_alloc(unix_pool, GFP_NOFS);
>> 
>> 	dprintk("RPC:       allocating UNIX cred for uid %d gid %d\n",
>> 			from_kuid(&init_user_ns, acred->cred->fsuid),
>> 			from_kgid(&init_user_ns, acred->cred->fsgid));
>> 
>> -	if (!(cred = kmalloc(sizeof(*cred), gfp)))
>> -		return ERR_PTR(-ENOMEM);
>> -
>> -	rpcauth_init_cred(&cred->uc_base, acred, auth, &unix_credops);
>> -	cred->uc_base.cr_flags = 1UL << RPCAUTH_CRED_UPTODATE;
>> -
>> -	if (acred->cred && acred->cred->group_info != NULL)
>> -		groups = acred->cred->group_info->ngroups;
>> -	if (groups > UNX_NGROUPS)
>> -		groups = UNX_NGROUPS;
>> -
>> -	cred->uc_gid = acred->cred->fsgid;
>> -	for (i = 0; i < groups; i++)
>> -		cred->uc_gids[i] = acred->cred->group_info->gid[i];
>> -	if (i < UNX_NGROUPS)
>> -		cred->uc_gids[i] = INVALID_GID;
>> -
>> -	return &cred->uc_base;
>> -}
>> -
>> -static void
>> -unx_free_cred(struct unx_cred *unx_cred)
>> -{
>> -	dprintk("RPC:       unx_free_cred %p\n", unx_cred);
>> -	put_cred(unx_cred->uc_base.cr_cred);
>> -	kfree(unx_cred);
>> +	rpcauth_init_cred(ret, acred, auth, &unix_credops);
>> +	ret->cr_flags = 1UL << RPCAUTH_CRED_UPTODATE;
>> +	return ret;
>> }
>> 
>> static void
>> unx_free_cred_callback(struct rcu_head *head)
>> {
>> -	struct unx_cred *unx_cred = container_of(head, struct unx_cred, uc_base.cr_rcu);
>> -	unx_free_cred(unx_cred);
>> +	struct rpc_cred *rpc_cred = container_of(head, struct rpc_cred, cr_rcu);
>> +	dprintk("RPC:       unx_free_cred %p\n", rpc_cred);
>> +	put_cred(rpc_cred->cr_cred);
>> +	mempool_free(rpc_cred, unix_pool);
>> }
>> 
>> static void
>> @@ -115,30 +73,32 @@ unx_destroy_cred(struct rpc_cred *cred)
>> }
>> 
>> /*
>> - * Match credentials against current process creds.
>> - * The root_override argument takes care of cases where the caller may
>> - * request root creds (e.g. for NFS swapping).
>> + * Match credentials against current the auth_cred.
>>  */
>> static int
>> -unx_match(struct auth_cred *acred, struct rpc_cred *rcred, int flags)
>> +unx_match(struct auth_cred *acred, struct rpc_cred *cred, int flags)
>> {
>> -	struct unx_cred	*cred = container_of(rcred, struct unx_cred, uc_base);
>> 	unsigned int groups = 0;
>> 	unsigned int i;
>> 
>> +	if (cred->cr_cred == acred->cred)
>> +		return 1;
>> 
>> -	if (!uid_eq(cred->uc_uid, acred->cred->fsuid) || !gid_eq(cred->uc_gid, acred->cred->fsgid))
>> +	if (!uid_eq(cred->cr_cred->fsuid, acred->cred->fsuid) || !gid_eq(cred->cr_cred->fsgid, acred->cred->fsgid))
>> 		return 0;
>> 
>> 	if (acred->cred && acred->cred->group_info != NULL)
>> 		groups = acred->cred->group_info->ngroups;
>> 	if (groups > UNX_NGROUPS)
>> 		groups = UNX_NGROUPS;
>> +	if (cred->cr_cred->group_info == NULL)
>> +		return groups == 0;
>> +	if (groups != cred->cr_cred->group_info->ngroups)
>> +		return 0;
>> +
>> 	for (i = 0; i < groups ; i++)
>> -		if (!gid_eq(cred->uc_gids[i], acred->cred->group_info->gid[i]))
>> +		if (!gid_eq(cred->cr_cred->group_info->gid[i], acred->cred->group_info->gid[i]))
>> 			return 0;
>> -	if (groups < UNX_NGROUPS && gid_valid(cred->uc_gids[groups]))
>> -		return 0;
>> 	return 1;
>> }
>> 
>> @@ -150,9 +110,10 @@ static __be32 *
>> unx_marshal(struct rpc_task *task, __be32 *p)
>> {
>> 	struct rpc_clnt	*clnt = task->tk_client;
>> -	struct unx_cred	*cred = container_of(task->tk_rqstp->rq_cred, struct unx_cred, uc_base);
>> +	struct rpc_cred	*cred = task->tk_rqstp->rq_cred;
>> 	__be32		*base, *hold;
>> 	int		i;
>> +	struct group_info *gi = cred->cr_cred->group_info;
>> 
>> 	*p++ = htonl(RPC_AUTH_UNIX);
>> 	base = p++;
>> @@ -163,11 +124,12 @@ unx_marshal(struct rpc_task *task, __be32 *p)
>> 	 */
>> 	p = xdr_encode_array(p, clnt->cl_nodename, clnt->cl_nodelen);
>> 
>> -	*p++ = htonl((u32) from_kuid(&init_user_ns, cred->uc_uid));
>> -	*p++ = htonl((u32) from_kgid(&init_user_ns, cred->uc_gid));
>> +	*p++ = htonl((u32) from_kuid(&init_user_ns, cred->cr_cred->fsuid));
>> +	*p++ = htonl((u32) from_kgid(&init_user_ns, cred->cr_cred->fsgid));
>> 	hold = p++;
>> -	for (i = 0; i < UNX_NGROUPS && gid_valid(cred->uc_gids[i]); i++)
>> -		*p++ = htonl((u32) from_kgid(&init_user_ns, cred->uc_gids[i]));
>> +	if (gi)
>> +		for (i = 0; i < UNX_NGROUPS && i < gi->ngroups; i++)
>> +			*p++ = htonl((u32) from_kgid(&init_user_ns, gi->gid[i]));
>> 	*hold = htonl(p - hold - 1);		/* gid array length */
>> 	*base = htonl((p - base - 1) << 2);	/* cred length */
>> 
>> @@ -214,12 +176,13 @@ unx_validate(struct rpc_task *task, __be32 *p)
>> 
>> int __init rpc_init_authunix(void)
>> {
>> -	return rpcauth_init_credcache(&unix_auth);
>> +	unix_pool = mempool_create_kmalloc_pool(16, sizeof(struct rpc_cred));
>> +	return unix_pool ? 0 : -ENOMEM;
>> }
>> 
>> void rpc_destroy_authunix(void)
>> {
>> -	rpcauth_destroy_credcache(&unix_auth);
>> +	mempool_destroy(unix_pool);
>> }
>> 
>> const struct rpc_authops authunix_ops = {
>> @@ -228,9 +191,7 @@ const struct rpc_authops authunix_ops = {
>> 	.au_name	= "UNIX",
>> 	.create		= unx_create,
>> 	.destroy	= unx_destroy,
>> -	.hash_cred	= unx_hash_cred,
>> 	.lookup_cred	= unx_lookup_cred,
>> -	.crcreate	= unx_create_cred,
>> };
>> 
>> static
>> 
>> 
>
> --
> Chuck Lever
Chuck Lever Nov. 8, 2018, 3:54 p.m. UTC | #3
> On Nov 7, 2018, at 8:41 PM, NeilBrown <neilb@suse.com> wrote:
> 
> On Wed, Nov 07 2018, Chuck Lever wrote:
> 
>> Hi Neil-
>> 
>> 
>>> On Nov 6, 2018, at 11:12 PM, NeilBrown <neilb@suse.com> wrote:
>>> 
>>> 1/ discard 'struct unx_cred'.  We don't need any data that
>>>  is not already in 'struct rpc_cred'.
>>> 2/ Don't keep these creds in a hash table.  When a credential
>>>  is needed, simply allocate it.  When not needed, discard it.
>>>  This can easily be faster than performing a lookup on
>>>  a shared hash table.
> 
> Thanks for the review Chuck!
> 
>> 
>> What's the basis for this claim? A memory allocation disables and
>> enables IRQs. That definitely hits a resource that is globally
>> shared.
> 
> My basis is not rock solid, but I was convinced :-)
> 
> kmem_cache_alloc() does disable local irqs when slab.c is used.
> slub.c doesn't disable them in the fast path which I *think* should be
> reasonably common.
> slob always takes a spinlock as well as disabling interrupts.
> 
> I think slob is only recommended for tiny machines, and slub is
> generally preferred, so I think that when performance matters, it will
> still be delivered.
> 
> It isn't clear to me why you consider a local irq to be "globally
> shared" - assuming that is what you mean.

Globally-shared in this case can be construed somewhat narrowly.
If we assume slub, kmem_cache_alloc() touches resources that are
used by all tasks running on that CPU, at least in the slow path.


> Disabling local interrupts is not without cost, but I don't think the
> cost increases with the number of CPUs, while the cost of accessing
> shared memory (even without a spinlock) does.

Point taken, but having a single mempool for all RPC transports
and users is also going to be a shared resource that can
bottleneck.

I guess that's an argument in favor of building creds on demand
rather than keeping them in a hash table, isn't it. :-)


>> In addition, the comment near unx_marshal suggests we should
>> cache the marshaled on-the-wire version of the credential instead
>> of building it in the RPC Call buffer every time. That would
>> require keeping the creds around.
> 
> That comment has been there since 2.1.32 and has not be acted on.  There
> seems little reason to expect that to change.  Caching doesn't seem to
> have been found to be necessary in practice.
> 
>> 
>> Have you measured a significant difference in throughput with
>> this patch? Have you considered improving the lookup speed of
>> the hash table by making the buckets into rb-trees, for example?
> 
> No, I haven't measured.
> I might have briefly considered changing to an rb-tree, but as the
> current hashtable doesn't actually contain anything of value, I would
> have quickly discarded the idea.
> 
> If I wanted to further improve performance, I would look at ways to
> bypass the "lookup_cred" step completely.
> unx_marshal only needs the generic "struct cred", so there should be no
> need to have a 'struct rpc_cred' at all.
> 
> If this series is accepted, I'll (hopefully) look into seeing how
> practical that is.
> 
> Thanks again,
> NeilBrown
> 
> 
>> 
>> 
>>>  As the lookup can happen during write-out, use a mempool
>>>  to ensure forward progress.
>>>  This means that we cannot compare two credentials for
>>>  equality by comparing the pointers, but we never do that anyway.
>>> 
>>> Signed-off-by: NeilBrown <neilb@suse.com>
>>> ---
>>> net/sunrpc/auth.c      |    1 
>>> net/sunrpc/auth_unix.c |  101 +++++++++++++++---------------------------------
>>> 2 files changed, 32 insertions(+), 70 deletions(-)
>>> 
>>> diff --git a/net/sunrpc/auth.c b/net/sunrpc/auth.c
>>> index 867ea9834bde..a07a7c59d3a4 100644
>>> --- a/net/sunrpc/auth.c
>>> +++ b/net/sunrpc/auth.c
>>> @@ -651,6 +651,7 @@ rpcauth_init_cred(struct rpc_cred *cred, const struct auth_cred *acred,
>>> 	INIT_LIST_HEAD(&cred->cr_lru);
>>> 	refcount_set(&cred->cr_count, 1);
>>> 	cred->cr_auth = auth;
>>> +	cred->cr_flags = 0;
>>> 	cred->cr_ops = ops;
>>> 	cred->cr_expire = jiffies;
>>> 	cred->cr_cred = get_cred(acred->cred);
>>> diff --git a/net/sunrpc/auth_unix.c b/net/sunrpc/auth_unix.c
>>> index bff113a411e0..387f6b3ffbea 100644
>>> --- a/net/sunrpc/auth_unix.c
>>> +++ b/net/sunrpc/auth_unix.c
>>> @@ -11,16 +11,11 @@
>>> #include <linux/types.h>
>>> #include <linux/sched.h>
>>> #include <linux/module.h>
>>> +#include <linux/mempool.h>
>>> #include <linux/sunrpc/clnt.h>
>>> #include <linux/sunrpc/auth.h>
>>> #include <linux/user_namespace.h>
>>> 
>>> -struct unx_cred {
>>> -	struct rpc_cred		uc_base;
>>> -	kgid_t			uc_gid;
>>> -	kgid_t			uc_gids[UNX_NGROUPS];
>>> -};
>>> -#define uc_uid			uc_base.cr_uid
>>> 
>>> #if IS_ENABLED(CONFIG_SUNRPC_DEBUG)
>>> # define RPCDBG_FACILITY	RPCDBG_AUTH
>>> @@ -28,6 +23,7 @@ struct unx_cred {
>>> 
>>> static struct rpc_auth		unix_auth;
>>> static const struct rpc_credops	unix_credops;
>>> +static mempool_t		*unix_pool;
>>> 
>>> static struct rpc_auth *
>>> unx_create(const struct rpc_auth_create_args *args, struct rpc_clnt *clnt)
>>> @@ -42,15 +38,6 @@ static void
>>> unx_destroy(struct rpc_auth *auth)
>>> {
>>> 	dprintk("RPC:       destroying UNIX authenticator %p\n", auth);
>>> -	rpcauth_clear_credcache(auth->au_credcache);
>>> -}
>>> -
>>> -static int
>>> -unx_hash_cred(struct auth_cred *acred, unsigned int hashbits)
>>> -{
>>> -	return hash_64(from_kgid(&init_user_ns, acred->cred->fsgid) |
>>> -		((u64)from_kuid(&init_user_ns, acred->cred->fsuid) <<
>>> -			(sizeof(gid_t) * 8)), hashbits);
>>> }
>>> 
>>> /*
>>> @@ -59,53 +46,24 @@ unx_hash_cred(struct auth_cred *acred, unsigned int hashbits)
>>> static struct rpc_cred *
>>> unx_lookup_cred(struct rpc_auth *auth, struct auth_cred *acred, int flags)
>>> {
>>> -	return rpcauth_lookup_credcache(auth, acred, flags, GFP_NOFS);
>>> -}
>>> -
>>> -static struct rpc_cred *
>>> -unx_create_cred(struct rpc_auth *auth, struct auth_cred *acred, int flags, gfp_t gfp)
>>> -{
>>> -	struct unx_cred	*cred;
>>> -	unsigned int groups = 0;
>>> -	unsigned int i;
>>> +	struct rpc_cred *ret = mempool_alloc(unix_pool, GFP_NOFS);
>>> 
>>> 	dprintk("RPC:       allocating UNIX cred for uid %d gid %d\n",
>>> 			from_kuid(&init_user_ns, acred->cred->fsuid),
>>> 			from_kgid(&init_user_ns, acred->cred->fsgid));
>>> 
>>> -	if (!(cred = kmalloc(sizeof(*cred), gfp)))
>>> -		return ERR_PTR(-ENOMEM);
>>> -
>>> -	rpcauth_init_cred(&cred->uc_base, acred, auth, &unix_credops);
>>> -	cred->uc_base.cr_flags = 1UL << RPCAUTH_CRED_UPTODATE;
>>> -
>>> -	if (acred->cred && acred->cred->group_info != NULL)
>>> -		groups = acred->cred->group_info->ngroups;
>>> -	if (groups > UNX_NGROUPS)
>>> -		groups = UNX_NGROUPS;
>>> -
>>> -	cred->uc_gid = acred->cred->fsgid;
>>> -	for (i = 0; i < groups; i++)
>>> -		cred->uc_gids[i] = acred->cred->group_info->gid[i];
>>> -	if (i < UNX_NGROUPS)
>>> -		cred->uc_gids[i] = INVALID_GID;
>>> -
>>> -	return &cred->uc_base;
>>> -}
>>> -
>>> -static void
>>> -unx_free_cred(struct unx_cred *unx_cred)
>>> -{
>>> -	dprintk("RPC:       unx_free_cred %p\n", unx_cred);
>>> -	put_cred(unx_cred->uc_base.cr_cred);
>>> -	kfree(unx_cred);
>>> +	rpcauth_init_cred(ret, acred, auth, &unix_credops);
>>> +	ret->cr_flags = 1UL << RPCAUTH_CRED_UPTODATE;
>>> +	return ret;
>>> }
>>> 
>>> static void
>>> unx_free_cred_callback(struct rcu_head *head)
>>> {
>>> -	struct unx_cred *unx_cred = container_of(head, struct unx_cred, uc_base.cr_rcu);
>>> -	unx_free_cred(unx_cred);
>>> +	struct rpc_cred *rpc_cred = container_of(head, struct rpc_cred, cr_rcu);
>>> +	dprintk("RPC:       unx_free_cred %p\n", rpc_cred);
>>> +	put_cred(rpc_cred->cr_cred);
>>> +	mempool_free(rpc_cred, unix_pool);
>>> }
>>> 
>>> static void
>>> @@ -115,30 +73,32 @@ unx_destroy_cred(struct rpc_cred *cred)
>>> }
>>> 
>>> /*
>>> - * Match credentials against current process creds.
>>> - * The root_override argument takes care of cases where the caller may
>>> - * request root creds (e.g. for NFS swapping).
>>> + * Match credentials against current the auth_cred.
>>> */
>>> static int
>>> -unx_match(struct auth_cred *acred, struct rpc_cred *rcred, int flags)
>>> +unx_match(struct auth_cred *acred, struct rpc_cred *cred, int flags)
>>> {
>>> -	struct unx_cred	*cred = container_of(rcred, struct unx_cred, uc_base);
>>> 	unsigned int groups = 0;
>>> 	unsigned int i;
>>> 
>>> +	if (cred->cr_cred == acred->cred)
>>> +		return 1;
>>> 
>>> -	if (!uid_eq(cred->uc_uid, acred->cred->fsuid) || !gid_eq(cred->uc_gid, acred->cred->fsgid))
>>> +	if (!uid_eq(cred->cr_cred->fsuid, acred->cred->fsuid) || !gid_eq(cred->cr_cred->fsgid, acred->cred->fsgid))
>>> 		return 0;
>>> 
>>> 	if (acred->cred && acred->cred->group_info != NULL)
>>> 		groups = acred->cred->group_info->ngroups;
>>> 	if (groups > UNX_NGROUPS)
>>> 		groups = UNX_NGROUPS;
>>> +	if (cred->cr_cred->group_info == NULL)
>>> +		return groups == 0;
>>> +	if (groups != cred->cr_cred->group_info->ngroups)
>>> +		return 0;
>>> +
>>> 	for (i = 0; i < groups ; i++)
>>> -		if (!gid_eq(cred->uc_gids[i], acred->cred->group_info->gid[i]))
>>> +		if (!gid_eq(cred->cr_cred->group_info->gid[i], acred->cred->group_info->gid[i]))
>>> 			return 0;
>>> -	if (groups < UNX_NGROUPS && gid_valid(cred->uc_gids[groups]))
>>> -		return 0;
>>> 	return 1;
>>> }
>>> 
>>> @@ -150,9 +110,10 @@ static __be32 *
>>> unx_marshal(struct rpc_task *task, __be32 *p)
>>> {
>>> 	struct rpc_clnt	*clnt = task->tk_client;
>>> -	struct unx_cred	*cred = container_of(task->tk_rqstp->rq_cred, struct unx_cred, uc_base);
>>> +	struct rpc_cred	*cred = task->tk_rqstp->rq_cred;
>>> 	__be32		*base, *hold;
>>> 	int		i;
>>> +	struct group_info *gi = cred->cr_cred->group_info;
>>> 
>>> 	*p++ = htonl(RPC_AUTH_UNIX);
>>> 	base = p++;
>>> @@ -163,11 +124,12 @@ unx_marshal(struct rpc_task *task, __be32 *p)
>>> 	 */
>>> 	p = xdr_encode_array(p, clnt->cl_nodename, clnt->cl_nodelen);
>>> 
>>> -	*p++ = htonl((u32) from_kuid(&init_user_ns, cred->uc_uid));
>>> -	*p++ = htonl((u32) from_kgid(&init_user_ns, cred->uc_gid));
>>> +	*p++ = htonl((u32) from_kuid(&init_user_ns, cred->cr_cred->fsuid));
>>> +	*p++ = htonl((u32) from_kgid(&init_user_ns, cred->cr_cred->fsgid));
>>> 	hold = p++;
>>> -	for (i = 0; i < UNX_NGROUPS && gid_valid(cred->uc_gids[i]); i++)
>>> -		*p++ = htonl((u32) from_kgid(&init_user_ns, cred->uc_gids[i]));
>>> +	if (gi)
>>> +		for (i = 0; i < UNX_NGROUPS && i < gi->ngroups; i++)
>>> +			*p++ = htonl((u32) from_kgid(&init_user_ns, gi->gid[i]));
>>> 	*hold = htonl(p - hold - 1);		/* gid array length */
>>> 	*base = htonl((p - base - 1) << 2);	/* cred length */
>>> 
>>> @@ -214,12 +176,13 @@ unx_validate(struct rpc_task *task, __be32 *p)
>>> 
>>> int __init rpc_init_authunix(void)
>>> {
>>> -	return rpcauth_init_credcache(&unix_auth);
>>> +	unix_pool = mempool_create_kmalloc_pool(16, sizeof(struct rpc_cred));
>>> +	return unix_pool ? 0 : -ENOMEM;
>>> }
>>> 
>>> void rpc_destroy_authunix(void)
>>> {
>>> -	rpcauth_destroy_credcache(&unix_auth);
>>> +	mempool_destroy(unix_pool);
>>> }
>>> 
>>> const struct rpc_authops authunix_ops = {
>>> @@ -228,9 +191,7 @@ const struct rpc_authops authunix_ops = {
>>> 	.au_name	= "UNIX",
>>> 	.create		= unx_create,
>>> 	.destroy	= unx_destroy,
>>> -	.hash_cred	= unx_hash_cred,
>>> 	.lookup_cred	= unx_lookup_cred,
>>> -	.crcreate	= unx_create_cred,
>>> };
>>> 
>>> static
>>> 
>>> 
>> 
>> --
>> Chuck Lever

--
Chuck Lever
NeilBrown Nov. 9, 2018, 12:45 a.m. UTC | #4
On Thu, Nov 08 2018, Chuck Lever wrote:
>
> Point taken, but having a single mempool for all RPC transports
> and users is also going to be a shared resource that can
> bottleneck.

Agreed.
mempools will only access the pre-allocated memory if a regular
kmalloc(GFP_NOWAIT) fails.  I asked an mm developer about this once as
it seemed backwards, and he pointed out that kmalloc is much more
optimised than mempools.  It often only accesses per-cpu resources,
which is much faster than taking a spinlock.  Learning that helped me
understand some of this stuff better.

>
> I guess that's an argument in favor of building creds on demand
> rather than keeping them in a hash table, isn't it. :-)

I think it is.  Thanks!!!

NeilBrown


>
>
>>> In addition, the comment near unx_marshal suggests we should
>>> cache the marshaled on-the-wire version of the credential instead
>>> of building it in the RPC Call buffer every time. That would
>>> require keeping the creds around.
>> 
>> That comment has been there since 2.1.32 and has not be acted on.  There
>> seems little reason to expect that to change.  Caching doesn't seem to
>> have been found to be necessary in practice.
>> 
>>> 
>>> Have you measured a significant difference in throughput with
>>> this patch? Have you considered improving the lookup speed of
>>> the hash table by making the buckets into rb-trees, for example?
>> 
>> No, I haven't measured.
>> I might have briefly considered changing to an rb-tree, but as the
>> current hashtable doesn't actually contain anything of value, I would
>> have quickly discarded the idea.
>> 
>> If I wanted to further improve performance, I would look at ways to
>> bypass the "lookup_cred" step completely.
>> unx_marshal only needs the generic "struct cred", so there should be no
>> need to have a 'struct rpc_cred' at all.
>> 
>> If this series is accepted, I'll (hopefully) look into seeing how
>> practical that is.
>> 
>> Thanks again,
>> NeilBrown
>> 
>> 
>>> 
>>> 
>>>>  As the lookup can happen during write-out, use a mempool
>>>>  to ensure forward progress.
>>>>  This means that we cannot compare two credentials for
>>>>  equality by comparing the pointers, but we never do that anyway.
>>>> 
>>>> Signed-off-by: NeilBrown <neilb@suse.com>
>>>> ---
>>>> net/sunrpc/auth.c      |    1 
>>>> net/sunrpc/auth_unix.c |  101 +++++++++++++++---------------------------------
>>>> 2 files changed, 32 insertions(+), 70 deletions(-)
>>>> 
>>>> diff --git a/net/sunrpc/auth.c b/net/sunrpc/auth.c
>>>> index 867ea9834bde..a07a7c59d3a4 100644
>>>> --- a/net/sunrpc/auth.c
>>>> +++ b/net/sunrpc/auth.c
>>>> @@ -651,6 +651,7 @@ rpcauth_init_cred(struct rpc_cred *cred, const struct auth_cred *acred,
>>>> 	INIT_LIST_HEAD(&cred->cr_lru);
>>>> 	refcount_set(&cred->cr_count, 1);
>>>> 	cred->cr_auth = auth;
>>>> +	cred->cr_flags = 0;
>>>> 	cred->cr_ops = ops;
>>>> 	cred->cr_expire = jiffies;
>>>> 	cred->cr_cred = get_cred(acred->cred);
>>>> diff --git a/net/sunrpc/auth_unix.c b/net/sunrpc/auth_unix.c
>>>> index bff113a411e0..387f6b3ffbea 100644
>>>> --- a/net/sunrpc/auth_unix.c
>>>> +++ b/net/sunrpc/auth_unix.c
>>>> @@ -11,16 +11,11 @@
>>>> #include <linux/types.h>
>>>> #include <linux/sched.h>
>>>> #include <linux/module.h>
>>>> +#include <linux/mempool.h>
>>>> #include <linux/sunrpc/clnt.h>
>>>> #include <linux/sunrpc/auth.h>
>>>> #include <linux/user_namespace.h>
>>>> 
>>>> -struct unx_cred {
>>>> -	struct rpc_cred		uc_base;
>>>> -	kgid_t			uc_gid;
>>>> -	kgid_t			uc_gids[UNX_NGROUPS];
>>>> -};
>>>> -#define uc_uid			uc_base.cr_uid
>>>> 
>>>> #if IS_ENABLED(CONFIG_SUNRPC_DEBUG)
>>>> # define RPCDBG_FACILITY	RPCDBG_AUTH
>>>> @@ -28,6 +23,7 @@ struct unx_cred {
>>>> 
>>>> static struct rpc_auth		unix_auth;
>>>> static const struct rpc_credops	unix_credops;
>>>> +static mempool_t		*unix_pool;
>>>> 
>>>> static struct rpc_auth *
>>>> unx_create(const struct rpc_auth_create_args *args, struct rpc_clnt *clnt)
>>>> @@ -42,15 +38,6 @@ static void
>>>> unx_destroy(struct rpc_auth *auth)
>>>> {
>>>> 	dprintk("RPC:       destroying UNIX authenticator %p\n", auth);
>>>> -	rpcauth_clear_credcache(auth->au_credcache);
>>>> -}
>>>> -
>>>> -static int
>>>> -unx_hash_cred(struct auth_cred *acred, unsigned int hashbits)
>>>> -{
>>>> -	return hash_64(from_kgid(&init_user_ns, acred->cred->fsgid) |
>>>> -		((u64)from_kuid(&init_user_ns, acred->cred->fsuid) <<
>>>> -			(sizeof(gid_t) * 8)), hashbits);
>>>> }
>>>> 
>>>> /*
>>>> @@ -59,53 +46,24 @@ unx_hash_cred(struct auth_cred *acred, unsigned int hashbits)
>>>> static struct rpc_cred *
>>>> unx_lookup_cred(struct rpc_auth *auth, struct auth_cred *acred, int flags)
>>>> {
>>>> -	return rpcauth_lookup_credcache(auth, acred, flags, GFP_NOFS);
>>>> -}
>>>> -
>>>> -static struct rpc_cred *
>>>> -unx_create_cred(struct rpc_auth *auth, struct auth_cred *acred, int flags, gfp_t gfp)
>>>> -{
>>>> -	struct unx_cred	*cred;
>>>> -	unsigned int groups = 0;
>>>> -	unsigned int i;
>>>> +	struct rpc_cred *ret = mempool_alloc(unix_pool, GFP_NOFS);
>>>> 
>>>> 	dprintk("RPC:       allocating UNIX cred for uid %d gid %d\n",
>>>> 			from_kuid(&init_user_ns, acred->cred->fsuid),
>>>> 			from_kgid(&init_user_ns, acred->cred->fsgid));
>>>> 
>>>> -	if (!(cred = kmalloc(sizeof(*cred), gfp)))
>>>> -		return ERR_PTR(-ENOMEM);
>>>> -
>>>> -	rpcauth_init_cred(&cred->uc_base, acred, auth, &unix_credops);
>>>> -	cred->uc_base.cr_flags = 1UL << RPCAUTH_CRED_UPTODATE;
>>>> -
>>>> -	if (acred->cred && acred->cred->group_info != NULL)
>>>> -		groups = acred->cred->group_info->ngroups;
>>>> -	if (groups > UNX_NGROUPS)
>>>> -		groups = UNX_NGROUPS;
>>>> -
>>>> -	cred->uc_gid = acred->cred->fsgid;
>>>> -	for (i = 0; i < groups; i++)
>>>> -		cred->uc_gids[i] = acred->cred->group_info->gid[i];
>>>> -	if (i < UNX_NGROUPS)
>>>> -		cred->uc_gids[i] = INVALID_GID;
>>>> -
>>>> -	return &cred->uc_base;
>>>> -}
>>>> -
>>>> -static void
>>>> -unx_free_cred(struct unx_cred *unx_cred)
>>>> -{
>>>> -	dprintk("RPC:       unx_free_cred %p\n", unx_cred);
>>>> -	put_cred(unx_cred->uc_base.cr_cred);
>>>> -	kfree(unx_cred);
>>>> +	rpcauth_init_cred(ret, acred, auth, &unix_credops);
>>>> +	ret->cr_flags = 1UL << RPCAUTH_CRED_UPTODATE;
>>>> +	return ret;
>>>> }
>>>> 
>>>> static void
>>>> unx_free_cred_callback(struct rcu_head *head)
>>>> {
>>>> -	struct unx_cred *unx_cred = container_of(head, struct unx_cred, uc_base.cr_rcu);
>>>> -	unx_free_cred(unx_cred);
>>>> +	struct rpc_cred *rpc_cred = container_of(head, struct rpc_cred, cr_rcu);
>>>> +	dprintk("RPC:       unx_free_cred %p\n", rpc_cred);
>>>> +	put_cred(rpc_cred->cr_cred);
>>>> +	mempool_free(rpc_cred, unix_pool);
>>>> }
>>>> 
>>>> static void
>>>> @@ -115,30 +73,32 @@ unx_destroy_cred(struct rpc_cred *cred)
>>>> }
>>>> 
>>>> /*
>>>> - * Match credentials against current process creds.
>>>> - * The root_override argument takes care of cases where the caller may
>>>> - * request root creds (e.g. for NFS swapping).
>>>> + * Match credentials against current the auth_cred.
>>>> */
>>>> static int
>>>> -unx_match(struct auth_cred *acred, struct rpc_cred *rcred, int flags)
>>>> +unx_match(struct auth_cred *acred, struct rpc_cred *cred, int flags)
>>>> {
>>>> -	struct unx_cred	*cred = container_of(rcred, struct unx_cred, uc_base);
>>>> 	unsigned int groups = 0;
>>>> 	unsigned int i;
>>>> 
>>>> +	if (cred->cr_cred == acred->cred)
>>>> +		return 1;
>>>> 
>>>> -	if (!uid_eq(cred->uc_uid, acred->cred->fsuid) || !gid_eq(cred->uc_gid, acred->cred->fsgid))
>>>> +	if (!uid_eq(cred->cr_cred->fsuid, acred->cred->fsuid) || !gid_eq(cred->cr_cred->fsgid, acred->cred->fsgid))
>>>> 		return 0;
>>>> 
>>>> 	if (acred->cred && acred->cred->group_info != NULL)
>>>> 		groups = acred->cred->group_info->ngroups;
>>>> 	if (groups > UNX_NGROUPS)
>>>> 		groups = UNX_NGROUPS;
>>>> +	if (cred->cr_cred->group_info == NULL)
>>>> +		return groups == 0;
>>>> +	if (groups != cred->cr_cred->group_info->ngroups)
>>>> +		return 0;
>>>> +
>>>> 	for (i = 0; i < groups ; i++)
>>>> -		if (!gid_eq(cred->uc_gids[i], acred->cred->group_info->gid[i]))
>>>> +		if (!gid_eq(cred->cr_cred->group_info->gid[i], acred->cred->group_info->gid[i]))
>>>> 			return 0;
>>>> -	if (groups < UNX_NGROUPS && gid_valid(cred->uc_gids[groups]))
>>>> -		return 0;
>>>> 	return 1;
>>>> }
>>>> 
>>>> @@ -150,9 +110,10 @@ static __be32 *
>>>> unx_marshal(struct rpc_task *task, __be32 *p)
>>>> {
>>>> 	struct rpc_clnt	*clnt = task->tk_client;
>>>> -	struct unx_cred	*cred = container_of(task->tk_rqstp->rq_cred, struct unx_cred, uc_base);
>>>> +	struct rpc_cred	*cred = task->tk_rqstp->rq_cred;
>>>> 	__be32		*base, *hold;
>>>> 	int		i;
>>>> +	struct group_info *gi = cred->cr_cred->group_info;
>>>> 
>>>> 	*p++ = htonl(RPC_AUTH_UNIX);
>>>> 	base = p++;
>>>> @@ -163,11 +124,12 @@ unx_marshal(struct rpc_task *task, __be32 *p)
>>>> 	 */
>>>> 	p = xdr_encode_array(p, clnt->cl_nodename, clnt->cl_nodelen);
>>>> 
>>>> -	*p++ = htonl((u32) from_kuid(&init_user_ns, cred->uc_uid));
>>>> -	*p++ = htonl((u32) from_kgid(&init_user_ns, cred->uc_gid));
>>>> +	*p++ = htonl((u32) from_kuid(&init_user_ns, cred->cr_cred->fsuid));
>>>> +	*p++ = htonl((u32) from_kgid(&init_user_ns, cred->cr_cred->fsgid));
>>>> 	hold = p++;
>>>> -	for (i = 0; i < UNX_NGROUPS && gid_valid(cred->uc_gids[i]); i++)
>>>> -		*p++ = htonl((u32) from_kgid(&init_user_ns, cred->uc_gids[i]));
>>>> +	if (gi)
>>>> +		for (i = 0; i < UNX_NGROUPS && i < gi->ngroups; i++)
>>>> +			*p++ = htonl((u32) from_kgid(&init_user_ns, gi->gid[i]));
>>>> 	*hold = htonl(p - hold - 1);		/* gid array length */
>>>> 	*base = htonl((p - base - 1) << 2);	/* cred length */
>>>> 
>>>> @@ -214,12 +176,13 @@ unx_validate(struct rpc_task *task, __be32 *p)
>>>> 
>>>> int __init rpc_init_authunix(void)
>>>> {
>>>> -	return rpcauth_init_credcache(&unix_auth);
>>>> +	unix_pool = mempool_create_kmalloc_pool(16, sizeof(struct rpc_cred));
>>>> +	return unix_pool ? 0 : -ENOMEM;
>>>> }
>>>> 
>>>> void rpc_destroy_authunix(void)
>>>> {
>>>> -	rpcauth_destroy_credcache(&unix_auth);
>>>> +	mempool_destroy(unix_pool);
>>>> }
>>>> 
>>>> const struct rpc_authops authunix_ops = {
>>>> @@ -228,9 +191,7 @@ const struct rpc_authops authunix_ops = {
>>>> 	.au_name	= "UNIX",
>>>> 	.create		= unx_create,
>>>> 	.destroy	= unx_destroy,
>>>> -	.hash_cred	= unx_hash_cred,
>>>> 	.lookup_cred	= unx_lookup_cred,
>>>> -	.crcreate	= unx_create_cred,
>>>> };
>>>> 
>>>> static
>>>> 
>>>> 
>>> 
>>> --
>>> Chuck Lever
>
> --
> Chuck Lever

Patch
diff mbox series

diff --git a/net/sunrpc/auth.c b/net/sunrpc/auth.c
index 867ea9834bde..a07a7c59d3a4 100644
--- a/net/sunrpc/auth.c
+++ b/net/sunrpc/auth.c
@@ -651,6 +651,7 @@  rpcauth_init_cred(struct rpc_cred *cred, const struct auth_cred *acred,
 	INIT_LIST_HEAD(&cred->cr_lru);
 	refcount_set(&cred->cr_count, 1);
 	cred->cr_auth = auth;
+	cred->cr_flags = 0;
 	cred->cr_ops = ops;
 	cred->cr_expire = jiffies;
 	cred->cr_cred = get_cred(acred->cred);
diff --git a/net/sunrpc/auth_unix.c b/net/sunrpc/auth_unix.c
index bff113a411e0..387f6b3ffbea 100644
--- a/net/sunrpc/auth_unix.c
+++ b/net/sunrpc/auth_unix.c
@@ -11,16 +11,11 @@ 
 #include <linux/types.h>
 #include <linux/sched.h>
 #include <linux/module.h>
+#include <linux/mempool.h>
 #include <linux/sunrpc/clnt.h>
 #include <linux/sunrpc/auth.h>
 #include <linux/user_namespace.h>
 
-struct unx_cred {
-	struct rpc_cred		uc_base;
-	kgid_t			uc_gid;
-	kgid_t			uc_gids[UNX_NGROUPS];
-};
-#define uc_uid			uc_base.cr_uid
 
 #if IS_ENABLED(CONFIG_SUNRPC_DEBUG)
 # define RPCDBG_FACILITY	RPCDBG_AUTH
@@ -28,6 +23,7 @@  struct unx_cred {
 
 static struct rpc_auth		unix_auth;
 static const struct rpc_credops	unix_credops;
+static mempool_t		*unix_pool;
 
 static struct rpc_auth *
 unx_create(const struct rpc_auth_create_args *args, struct rpc_clnt *clnt)
@@ -42,15 +38,6 @@  static void
 unx_destroy(struct rpc_auth *auth)
 {
 	dprintk("RPC:       destroying UNIX authenticator %p\n", auth);
-	rpcauth_clear_credcache(auth->au_credcache);
-}
-
-static int
-unx_hash_cred(struct auth_cred *acred, unsigned int hashbits)
-{
-	return hash_64(from_kgid(&init_user_ns, acred->cred->fsgid) |
-		((u64)from_kuid(&init_user_ns, acred->cred->fsuid) <<
-			(sizeof(gid_t) * 8)), hashbits);
 }
 
 /*
@@ -59,53 +46,24 @@  unx_hash_cred(struct auth_cred *acred, unsigned int hashbits)
 static struct rpc_cred *
 unx_lookup_cred(struct rpc_auth *auth, struct auth_cred *acred, int flags)
 {
-	return rpcauth_lookup_credcache(auth, acred, flags, GFP_NOFS);
-}
-
-static struct rpc_cred *
-unx_create_cred(struct rpc_auth *auth, struct auth_cred *acred, int flags, gfp_t gfp)
-{
-	struct unx_cred	*cred;
-	unsigned int groups = 0;
-	unsigned int i;
+	struct rpc_cred *ret = mempool_alloc(unix_pool, GFP_NOFS);
 
 	dprintk("RPC:       allocating UNIX cred for uid %d gid %d\n",
 			from_kuid(&init_user_ns, acred->cred->fsuid),
 			from_kgid(&init_user_ns, acred->cred->fsgid));
 
-	if (!(cred = kmalloc(sizeof(*cred), gfp)))
-		return ERR_PTR(-ENOMEM);
-
-	rpcauth_init_cred(&cred->uc_base, acred, auth, &unix_credops);
-	cred->uc_base.cr_flags = 1UL << RPCAUTH_CRED_UPTODATE;
-
-	if (acred->cred && acred->cred->group_info != NULL)
-		groups = acred->cred->group_info->ngroups;
-	if (groups > UNX_NGROUPS)
-		groups = UNX_NGROUPS;
-
-	cred->uc_gid = acred->cred->fsgid;
-	for (i = 0; i < groups; i++)
-		cred->uc_gids[i] = acred->cred->group_info->gid[i];
-	if (i < UNX_NGROUPS)
-		cred->uc_gids[i] = INVALID_GID;
-
-	return &cred->uc_base;
-}
-
-static void
-unx_free_cred(struct unx_cred *unx_cred)
-{
-	dprintk("RPC:       unx_free_cred %p\n", unx_cred);
-	put_cred(unx_cred->uc_base.cr_cred);
-	kfree(unx_cred);
+	rpcauth_init_cred(ret, acred, auth, &unix_credops);
+	ret->cr_flags = 1UL << RPCAUTH_CRED_UPTODATE;
+	return ret;
 }
 
 static void
 unx_free_cred_callback(struct rcu_head *head)
 {
-	struct unx_cred *unx_cred = container_of(head, struct unx_cred, uc_base.cr_rcu);
-	unx_free_cred(unx_cred);
+	struct rpc_cred *rpc_cred = container_of(head, struct rpc_cred, cr_rcu);
+	dprintk("RPC:       unx_free_cred %p\n", rpc_cred);
+	put_cred(rpc_cred->cr_cred);
+	mempool_free(rpc_cred, unix_pool);
 }
 
 static void
@@ -115,30 +73,32 @@  unx_destroy_cred(struct rpc_cred *cred)
 }
 
 /*
- * Match credentials against current process creds.
- * The root_override argument takes care of cases where the caller may
- * request root creds (e.g. for NFS swapping).
+ * Match credentials against current the auth_cred.
  */
 static int
-unx_match(struct auth_cred *acred, struct rpc_cred *rcred, int flags)
+unx_match(struct auth_cred *acred, struct rpc_cred *cred, int flags)
 {
-	struct unx_cred	*cred = container_of(rcred, struct unx_cred, uc_base);
 	unsigned int groups = 0;
 	unsigned int i;
 
+	if (cred->cr_cred == acred->cred)
+		return 1;
 
-	if (!uid_eq(cred->uc_uid, acred->cred->fsuid) || !gid_eq(cred->uc_gid, acred->cred->fsgid))
+	if (!uid_eq(cred->cr_cred->fsuid, acred->cred->fsuid) || !gid_eq(cred->cr_cred->fsgid, acred->cred->fsgid))
 		return 0;
 
 	if (acred->cred && acred->cred->group_info != NULL)
 		groups = acred->cred->group_info->ngroups;
 	if (groups > UNX_NGROUPS)
 		groups = UNX_NGROUPS;
+	if (cred->cr_cred->group_info == NULL)
+		return groups == 0;
+	if (groups != cred->cr_cred->group_info->ngroups)
+		return 0;
+
 	for (i = 0; i < groups ; i++)
-		if (!gid_eq(cred->uc_gids[i], acred->cred->group_info->gid[i]))
+		if (!gid_eq(cred->cr_cred->group_info->gid[i], acred->cred->group_info->gid[i]))
 			return 0;
-	if (groups < UNX_NGROUPS && gid_valid(cred->uc_gids[groups]))
-		return 0;
 	return 1;
 }
 
@@ -150,9 +110,10 @@  static __be32 *
 unx_marshal(struct rpc_task *task, __be32 *p)
 {
 	struct rpc_clnt	*clnt = task->tk_client;
-	struct unx_cred	*cred = container_of(task->tk_rqstp->rq_cred, struct unx_cred, uc_base);
+	struct rpc_cred	*cred = task->tk_rqstp->rq_cred;
 	__be32		*base, *hold;
 	int		i;
+	struct group_info *gi = cred->cr_cred->group_info;
 
 	*p++ = htonl(RPC_AUTH_UNIX);
 	base = p++;
@@ -163,11 +124,12 @@  unx_marshal(struct rpc_task *task, __be32 *p)
 	 */
 	p = xdr_encode_array(p, clnt->cl_nodename, clnt->cl_nodelen);
 
-	*p++ = htonl((u32) from_kuid(&init_user_ns, cred->uc_uid));
-	*p++ = htonl((u32) from_kgid(&init_user_ns, cred->uc_gid));
+	*p++ = htonl((u32) from_kuid(&init_user_ns, cred->cr_cred->fsuid));
+	*p++ = htonl((u32) from_kgid(&init_user_ns, cred->cr_cred->fsgid));
 	hold = p++;
-	for (i = 0; i < UNX_NGROUPS && gid_valid(cred->uc_gids[i]); i++)
-		*p++ = htonl((u32) from_kgid(&init_user_ns, cred->uc_gids[i]));
+	if (gi)
+		for (i = 0; i < UNX_NGROUPS && i < gi->ngroups; i++)
+			*p++ = htonl((u32) from_kgid(&init_user_ns, gi->gid[i]));
 	*hold = htonl(p - hold - 1);		/* gid array length */
 	*base = htonl((p - base - 1) << 2);	/* cred length */
 
@@ -214,12 +176,13 @@  unx_validate(struct rpc_task *task, __be32 *p)
 
 int __init rpc_init_authunix(void)
 {
-	return rpcauth_init_credcache(&unix_auth);
+	unix_pool = mempool_create_kmalloc_pool(16, sizeof(struct rpc_cred));
+	return unix_pool ? 0 : -ENOMEM;
 }
 
 void rpc_destroy_authunix(void)
 {
-	rpcauth_destroy_credcache(&unix_auth);
+	mempool_destroy(unix_pool);
 }
 
 const struct rpc_authops authunix_ops = {
@@ -228,9 +191,7 @@  const struct rpc_authops authunix_ops = {
 	.au_name	= "UNIX",
 	.create		= unx_create,
 	.destroy	= unx_destroy,
-	.hash_cred	= unx_hash_cred,
 	.lookup_cred	= unx_lookup_cred,
-	.crcreate	= unx_create_cred,
 };
 
 static