| Message ID | 151538917875.25812.10005878132438571890.stgit@noble (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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. > > Signed-off-by: NeilBrown <neilb@suse.com> > --- > 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 <linux/nfs_fs.h> > #include <linux/nfs_page.h> > #include <linux/module.h> > +#include <linux/sched/mm.h> > > #include <linux/sunrpc/metrics.h> > > @@ -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
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] <IRQ> [ 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] </IRQ> [ 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 <neilb@suse.com> >> --- >> 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 <linux/nfs_fs.h> >> #include <linux/nfs_page.h> >> #include <linux/module.h> >> +#include <linux/sched/mm.h> >> >> #include <linux/sunrpc/metrics.h> >> >> @@ -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 > -- 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
On Thu, Jan 18 2018, 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. >> >> Signed-off-by: NeilBrown <neilb@suse.com> ... >> @@ -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 for finding that - I don't often compile things as modules :-) I'll separate out the core cred.[ch] patches, add this, and send those for an ACK to the relevant maintainer. thanks, NeilBrown
On Thu, Jan 18 2018, Anna Schumaker wrote: > 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] <IRQ> > [ 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] </IRQ> > [ 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] > That's not good. I've just read through the patches again and didn't find anything that could cause this, so I must have missed something. You say "This patch is also causing", but I assume it is the whole patch set rather than just this one patch - is that correct? Also, have you run tests without sec=krb5 and not had the error? I'll try to set up some more thorough testing myself. Thanks, NeilBrown
On Mon, Jan 29 2018, NeilBrown wrote: > On Thu, Jan 18 2018, Anna Schumaker wrote: > >> 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] <IRQ> >> [ 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] </IRQ> >> [ 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] >> > > That's not good. > > I've just read through the patches again and didn't find anything that > could cause this, so I must have missed something. > > You say "This patch is also causing", but I assume it is the whole patch > set rather than just this one patch - is that correct? > > Also, have you run tests without sec=krb5 and not had the error? > > I'll try to set up some more thorough testing myself. I found the problem. The crdestroy for auth_gss takes a new reference on the auth, and then releases it again. As I was calling put_auth() on ->cr_auth before calling crdestrory, it got put twice. I've move the responsibility for calling put_auth() into the crdestroy function. It now passes connectathon with krb5 and krb5p and without, etc. I'll resend the series sometime next week, hopefully after getting some sort of response to the cred-improvement patches I posted. Thanks for your help, NeilBrown
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 <linux/nfs_fs.h> #include <linux/nfs_page.h> #include <linux/module.h> +#include <linux/sched/mm.h> #include <linux/sunrpc/metrics.h> @@ -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), }; + 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), };
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. Signed-off-by: NeilBrown <neilb@suse.com> --- 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(-) -- 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