Return-Path: Received: from mx143.netapp.com ([216.240.21.24]:6403 "EHLO mx143.netapp.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932269AbeARTMD (ORCPT ); Thu, 18 Jan 2018 14:12:03 -0500 Subject: Re: [PATCH 02/20] SUNRPC: add 'struct cred *' to auth_cred and rpc_cred To: Anna Schumaker , NeilBrown , Trond Myklebust CC: References: <151538903497.25812.13293229343061416612.stgit@noble> <151538917875.25812.10005878132438571890.stgit@noble> From: Anna Schumaker Message-ID: <3cfc2eb8-68c9-6c26-d5eb-21f50ca921e5@Netapp.com> Date: Thu, 18 Jan 2018 14:11:55 -0500 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Sender: linux-nfs-owner@vger.kernel.org List-ID: On 01/18/2018 01:39 PM, Anna Schumaker wrote: > Hi Neil, > > On 01/08/2018 12:26 AM, NeilBrown wrote: >> The SUNRPC credential framework was put together before >> Linux has 'struct cred'. Now that we have it, it makes sense to >> use it. >> This first step just includes a suitable 'struct cred *' pointer >> in every 'struct auth_cred' and almost every 'struct rpc_cred'. >> >> The rpc_cred used for auth_null has a NULL 'struct cred *' as nothing >> else really makes sense. >> >> For rpc_cred, the pointer is reference counted. >> For auth_cred it isn't. struct auth_cred are either allocated on >> the stack, in which case the thread owns a reference to the auth, >> or are part of 'struct generic_cred' in which case gc_base owns the >> reference and acred shares it. This patch is also causing a kernel panic for me if I mount using sec=krb5, run cthon tests, and then unmount. Here is the log message I'm getting: [ 82.599174] Kernel panic - not syncing: CRED: put_cred_rcu() sees 00000000f5847a57 with usage -1 [ 82.599174] [ 82.600227] CPU: 0 PID: 0 Comm: swapper/0 Not tainted 4.15.0-rc7-ANNA+ #14336 [ 82.600801] Hardware name: Bochs Bochs, BIOS Bochs 01/01/2011 [ 82.601435] Call Trace: [ 82.601639] [ 82.601830] dump_stack+0x5c/0x7e [ 82.602125] panic+0xdf/0x228 [ 82.602383] ? try_to_wake_up+0x24b/0x420 [ 82.602853] put_cred_rcu+0x8a/0x90 [ 82.603183] rcu_process_callbacks+0x1ab/0x4f0 [ 82.603577] __do_softirq+0xcc/0x305 [ 82.603881] irq_exit+0xa9/0xb0 [ 82.604159] smp_apic_timer_interrupt+0x5b/0x140 [ 82.604528] apic_timer_interrupt+0x98/0xa0 [ 82.604892] [ 82.605133] RIP: 0010:native_safe_halt+0x2/0x10 [ 82.605678] RSP: 0018:ffffffff82003ea8 EFLAGS: 00000246 ORIG_RAX: ffffffffffffff11 [ 82.606619] RAX: 0000000080000000 RBX: 0000000000000000 RCX: 0000000000000000 [ 82.607270] RDX: 0000000000000000 RSI: 0000000000000000 RDI: 0000000000000000 [ 82.608077] RBP: 0000000000000000 R08: 0000000000000002 R09: 000000000001ea40 [ 82.609066] R10: 0000000000000001 R11: 0000000000000000 R12: 0000000000000000 [ 82.609951] R13: 0000000000000000 R14: 0000000000000000 R15: 0000000000000000 [ 82.610771] default_idle+0x15/0x120 [ 82.611128] do_idle+0x15c/0x1c0 [ 82.611371] cpu_startup_entry+0x6a/0x70 [ 82.611762] start_kernel+0x445/0x465 [ 82.612104] secondary_startup_64+0xa5/0xb0 [ 82.612561] Kernel Offset: disabled [ 82.612862] ---[ end Kernel panic - not syncing: CRED: put_cred_rcu() sees 00000000f5847a57 with usage -1 [ 82.612862] Thanks, Anna >> >> Signed-off-by: NeilBrown >> --- >> fs/nfs/flexfilelayout/flexfilelayout.c | 17 +++++++++++++++++ >> fs/nfsd/nfs4callback.c | 13 ++++++++++++- >> include/linux/sunrpc/auth.h | 2 ++ >> net/sunrpc/auth.c | 15 +++++++++++++-- >> net/sunrpc/auth_generic.c | 7 ++++++- >> net/sunrpc/auth_gss/auth_gss.c | 1 + >> 6 files changed, 51 insertions(+), 4 deletions(-) >> >> diff --git a/fs/nfs/flexfilelayout/flexfilelayout.c b/fs/nfs/flexfilelayout/flexfilelayout.c >> index c75ad982bcfc..b727579a1508 100644 >> --- a/fs/nfs/flexfilelayout/flexfilelayout.c >> +++ b/fs/nfs/flexfilelayout/flexfilelayout.c >> @@ -9,6 +9,7 @@ >> #include >> #include >> #include >> +#include >> >> #include >> >> @@ -415,6 +416,7 @@ ff_layout_alloc_lseg(struct pnfs_layout_hdr *lh, >> struct nfs4_ff_layout_mirror *mirror; >> struct auth_cred acred = { .group_info = ff_zero_group }; >> struct rpc_cred __rcu *cred; >> + struct cred *kcred; >> u32 ds_count, fh_count, id; >> int j; >> >> @@ -491,8 +493,23 @@ ff_layout_alloc_lseg(struct pnfs_layout_hdr *lh, >> >> acred.gid = make_kgid(&init_user_ns, id); >> >> + if (gfp_flags & __GFP_FS) >> + kcred = prepare_kernel_cred(NULL); >> + else { >> + unsigned int nofs_flags = memalloc_nofs_save(); >> + kcred = prepare_kernel_cred(NULL); >> + memalloc_nofs_restore(nofs_flags); >> + } >> + rc = -ENOMEM; >> + if (!kcred) >> + goto out_err_free; >> + kcred->fsuid = acred.uid; >> + kcred->fsgid = acred.gid; >> + acred.cred = kcred; >> + >> /* find the cred for it */ >> rcu_assign_pointer(cred, rpc_lookup_generic_cred(&acred, 0, gfp_flags)); >> + put_cred(kcred); >> if (IS_ERR(cred)) { >> rc = PTR_ERR(cred); >> goto out_err_free; >> diff --git a/fs/nfsd/nfs4callback.c b/fs/nfsd/nfs4callback.c >> index 49b0a9e7ff18..fc5b38ee6c70 100644 >> --- a/fs/nfsd/nfs4callback.c >> +++ b/fs/nfsd/nfs4callback.c >> @@ -773,10 +773,21 @@ static struct rpc_cred *get_backchannel_cred(struct nfs4_client *clp, struct rpc >> } else { >> struct rpc_auth *auth = client->cl_auth; >> struct auth_cred acred = {}; >> + struct cred *kcred; >> + struct rpc_cred *ret; >> + >> + kcred = prepare_kernel_cred(NULL); >> + if (!acred.cred) >> + return NULL; >> >> acred.uid = ses->se_cb_sec.uid; >> acred.gid = ses->se_cb_sec.gid; >> - return auth->au_ops->lookup_cred(client->cl_auth, &acred, 0); >> + kcred->uid = acred.uid; >> + kcred->gid = acred.gid; >> + acred.cred = kcred; >> + ret = auth->au_ops->lookup_cred(client->cl_auth, &acred, 0); >> + put_cred(kcred); >> + return ret; >> } >> } >> >> diff --git a/include/linux/sunrpc/auth.h b/include/linux/sunrpc/auth.h >> index d9af474a857d..57633e241d4a 100644 >> --- a/include/linux/sunrpc/auth.h >> +++ b/include/linux/sunrpc/auth.h >> @@ -46,6 +46,7 @@ enum { >> >> /* Work around the lack of a VFS credential */ >> struct auth_cred { >> + const struct cred *cred; >> kuid_t uid; >> kgid_t gid; >> struct group_info *group_info; >> @@ -68,6 +69,7 @@ struct rpc_cred { >> unsigned long cr_expire; /* when to gc */ >> unsigned long cr_flags; /* various flags */ >> atomic_t cr_count; /* ref count */ >> + const struct cred *cr_cred; >> >> kuid_t cr_uid; >> >> diff --git a/net/sunrpc/auth.c b/net/sunrpc/auth.c >> index d2623b9f23d6..fd9635dbc17f 100644 >> --- a/net/sunrpc/auth.c >> +++ b/net/sunrpc/auth.c >> @@ -634,6 +634,7 @@ rpcauth_lookupcred(struct rpc_auth *auth, int flags) >> acred.uid = cred->fsuid; >> acred.gid = cred->fsgid; >> acred.group_info = cred->group_info; >> + acred.cred = cred; >> ret = auth->au_ops->lookup_cred(auth, &acred, flags); >> return ret; >> } >> @@ -649,6 +650,7 @@ rpcauth_init_cred(struct rpc_cred *cred, const struct auth_cred *acred, >> cred->cr_auth = auth; >> cred->cr_ops = ops; >> cred->cr_expire = jiffies; >> + cred->cr_cred = get_cred(acred->cred); >> cred->cr_uid = acred->uid; >> } >> EXPORT_SYMBOL_GPL(rpcauth_init_cred); >> @@ -669,11 +671,15 @@ rpcauth_bind_root_cred(struct rpc_task *task, int lookupflags) >> struct auth_cred acred = { >> .uid = GLOBAL_ROOT_UID, >> .gid = GLOBAL_ROOT_GID, >> + .cred = get_task_cred(&init_task), > > Is there a patch somewhere to add "EXPORT_SYMBOL_GPL(get_task_cred)" to kernel/cred.c? > I'm getting: > ERROR: "get_task_cred" [net/sunrpc/sunrpc.ko] undefined! > when I compile. > > Thanks, > Anna > >> }; >> + struct rpc_cred *ret; >> >> dprintk("RPC: %5u looking up %s cred\n", >> task->tk_pid, task->tk_client->cl_auth->au_ops->au_name); >> - return auth->au_ops->lookup_cred(auth, &acred, lookupflags); >> + ret = auth->au_ops->lookup_cred(auth, &acred, lookupflags); >> + put_cred(acred.cred); >> + return ret; >> } >> >> static struct rpc_cred * >> @@ -715,8 +721,11 @@ put_rpccred(struct rpc_cred *cred) >> return; >> /* Fast path for unhashed credentials */ >> if (test_bit(RPCAUTH_CRED_HASHED, &cred->cr_flags) == 0) { >> - if (atomic_dec_and_test(&cred->cr_count)) >> + if (atomic_dec_and_test(&cred->cr_count)) { >> + if (cred->cr_cred) >> + put_cred(cred->cr_cred); >> cred->cr_ops->crdestroy(cred); >> + } >> return; >> } >> >> @@ -739,6 +748,8 @@ put_rpccred(struct rpc_cred *cred) >> } >> } >> spin_unlock(&rpc_credcache_lock); >> + if (cred->cr_cred) >> + put_cred(cred->cr_cred); >> cred->cr_ops->crdestroy(cred); >> return; >> out_nodestroy: >> diff --git a/net/sunrpc/auth_generic.c b/net/sunrpc/auth_generic.c >> index f1df9837f1ac..08bc5fac1865 100644 >> --- a/net/sunrpc/auth_generic.c >> +++ b/net/sunrpc/auth_generic.c >> @@ -61,11 +61,15 @@ struct rpc_cred *rpc_lookup_machine_cred(const char *service_name) >> .gid = RPC_MACHINE_CRED_GROUPID, >> .principal = service_name, >> .machine_cred = 1, >> + .cred = get_task_cred(&init_task), >> }; >> + struct rpc_cred *ret; >> >> dprintk("RPC: looking up machine cred for service %s\n", >> service_name); >> - return generic_auth.au_ops->lookup_cred(&generic_auth, &acred, 0); >> + ret = generic_auth.au_ops->lookup_cred(&generic_auth, &acred, 0); >> + put_cred(acred.cred); >> + return ret; >> } >> EXPORT_SYMBOL_GPL(rpc_lookup_machine_cred); >> >> @@ -110,6 +114,7 @@ generic_create_cred(struct rpc_auth *auth, struct auth_cred *acred, int flags, g >> gcred->acred.uid = acred->uid; >> gcred->acred.gid = acred->gid; >> gcred->acred.group_info = acred->group_info; >> + gcred->acred.cred = gcred->gc_base.cr_cred; >> gcred->acred.ac_flags = 0; >> if (gcred->acred.group_info != NULL) >> get_group_info(gcred->acred.group_info); >> diff --git a/net/sunrpc/auth_gss/auth_gss.c b/net/sunrpc/auth_gss/auth_gss.c >> index 9463af4b32e8..82301105b4f6 100644 >> --- a/net/sunrpc/auth_gss/auth_gss.c >> +++ b/net/sunrpc/auth_gss/auth_gss.c >> @@ -1553,6 +1553,7 @@ static int gss_renew_cred(struct rpc_task *task) >> struct rpc_auth *auth = oldcred->cr_auth; >> struct auth_cred acred = { >> .uid = oldcred->cr_uid, >> + .cred = oldcred->cr_cred, >> .principal = gss_cred->gc_principal, >> .machine_cred = (gss_cred->gc_principal != NULL ? 1 : 0), >> }; >> >> > -- > To unsubscribe from this list: send the line "unsubscribe linux-nfs" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html >