* [PATCH 0/3] nfs_instantiate() might succeed leaving dentry negative unhashed
@ 2019-09-13 12:29 Benjamin Coddington
2019-09-13 12:29 ` [PATCH 1/3] NFS: Refactor nfs_instantiate() for dentry referencing callers Benjamin Coddington
` (2 more replies)
0 siblings, 3 replies; 5+ messages in thread
From: Benjamin Coddington @ 2019-09-13 12:29 UTC (permalink / raw)
To: Trond Myklebust, Anna Schumaker; +Cc: Al Viro, linux-nfs
After adding commit b0c6108ecf64 ("nfs_instantiate(): prevent multiple
aliases for directory inode") my NFS client crashes while doing lustre race
tests simultaneously on a local filesystem and the same filesystem exported
via knfsd:
BUG: unable to handle kernel NULL pointer dereference at 0000000000000028
Call Trace:
? iput+0x76/0x200
? d_splice_alias+0x307/0x3c0
? dput.part.31+0x96/0x110
? nfs_instantiate+0x45/0x160 [nfs]
nfs3_proc_setacls+0xa/0x20 [nfsv3]
nfs3_proc_create+0x1cc/0x230 [nfsv3]
nfs_create+0x83/0x160 [nfs]
path_openat+0x11aa/0x14d0
do_filp_open+0x93/0x100
? __check_object_size+0xa3/0x181
do_sys_open+0x184/0x220
do_syscall_64+0x5b/0x1b0
entry_SYSCALL_64_after_hwframe+0x65/0xca
158 static int __nfs3_proc_setacls(struct inode *inode, struct posix_acl *acl,
159 struct posix_acl *dfacl)
160 {
161 struct nfs_server *server = NFS_SERVER(inode);
The 0x28 offset is i_sb in struct inode, we passed a NULL inode to
nfs3_proc_setacls().
After taking this apart, I find the dentry in R12 has a NULL inode after
nfs_instantiate(), which makes sense if we move it to the alias just after
nfs_fhget() (See the referenced commit above). Indeed, on the list of
children is the identical positive dentry that is left behind after
d_splice_alias(). Moving it would usualy be fine for callers, except for
NFSv3 because we want the inode pointer to ride the dentry back up the
stack so we can set ACLs on it and/or set attributes in the case of EXCLUSIVE.
The first patch splits up nfs_instantiate() so that we can have two call
paths, the original - whose callers don't care about what dentry ends up
hashed, and a new one that returns the dentry or the alias.
The second patch modifies NFSv3 to use the latter path for callers that need
to reference the dentry.
The third patch removes a test for positive dentry that seems to be
impossible - I can't find a path for it, and it has never been hit in all my
testing.
Benjamin Coddington (3):
NFS: Refactor nfs_instantiate() for dentry referencing callers
NFSv3: use nfs_add_or_obtain() to create and reference inodes
NFS: remove unused check for negative dentry
fs/nfs/dir.c | 41 +++++++++++++++++++++++---------------
fs/nfs/nfs3proc.c | 45 +++++++++++++++++++++++++++++++++---------
include/linux/nfs_fs.h | 3 +++
3 files changed, 64 insertions(+), 25 deletions(-)
--
2.20.1
^ permalink raw reply [flat|nested] 5+ messages in thread* [PATCH 1/3] NFS: Refactor nfs_instantiate() for dentry referencing callers 2019-09-13 12:29 [PATCH 0/3] nfs_instantiate() might succeed leaving dentry negative unhashed Benjamin Coddington @ 2019-09-13 12:29 ` Benjamin Coddington 2019-09-13 12:29 ` [PATCH 2/3] NFSv3: use nfs_add_or_obtain() to create and reference inodes Benjamin Coddington 2019-09-13 12:29 ` [PATCH 3/3] NFS: remove unused check for negative dentry Benjamin Coddington 2 siblings, 0 replies; 5+ messages in thread From: Benjamin Coddington @ 2019-09-13 12:29 UTC (permalink / raw) To: Trond Myklebust, Anna Schumaker; +Cc: Al Viro, linux-nfs Since commit b0c6108ecf64 ("nfs_instantiate(): prevent multiple aliases for directory inode"), nfs_instantiate() may succeed without actually instantiating the dentry that was passed in. That can be problematic for some callers in NFSv3, so this patch breaks things up so we can get the actual dentry obtained. Signed-off-by: Benjamin Coddington <bcodding@redhat.com> --- fs/nfs/dir.c | 41 +++++++++++++++++++++++++++-------------- include/linux/nfs_fs.h | 3 +++ 2 files changed, 30 insertions(+), 14 deletions(-) diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c index 0adfd8840110..cb6ee1b41ab5 100644 --- a/fs/nfs/dir.c +++ b/fs/nfs/dir.c @@ -1669,24 +1669,23 @@ static int nfs4_lookup_revalidate(struct dentry *dentry, unsigned int flags) #endif /* CONFIG_NFSV4 */ -/* - * Code common to create, mkdir, and mknod. - */ -int nfs_instantiate(struct dentry *dentry, struct nfs_fh *fhandle, +struct dentry * +nfs_add_or_obtain(struct dentry *dentry, struct nfs_fh *fhandle, struct nfs_fattr *fattr, struct nfs4_label *label) { struct dentry *parent = dget_parent(dentry); struct inode *dir = d_inode(parent); struct inode *inode; - struct dentry *d; - int error = -EACCES; + struct dentry *d = NULL; + int error; d_drop(dentry); /* We may have been initialized further down */ if (d_really_is_positive(dentry)) goto out; + if (fhandle->size == 0) { error = NFS_PROTO(dir)->lookup(dir, &dentry->d_name, fhandle, fattr, NULL); if (error) @@ -1702,18 +1701,32 @@ int nfs_instantiate(struct dentry *dentry, struct nfs_fh *fhandle, } inode = nfs_fhget(dentry->d_sb, fhandle, fattr, label); d = d_splice_alias(inode, dentry); - if (IS_ERR(d)) { - error = PTR_ERR(d); - goto out_error; - } - dput(d); out: dput(parent); - return 0; + return d; out_error: nfs_mark_for_revalidate(dir); - dput(parent); - return error; + d = ERR_PTR(error); + goto out; +} +EXPORT_SYMBOL_GPL(nfs_add_or_obtain); + +/* + * Code common to create, mkdir, and mknod. + */ +int nfs_instantiate(struct dentry *dentry, struct nfs_fh *fhandle, + struct nfs_fattr *fattr, + struct nfs4_label *label) +{ + struct dentry *d; + + d = nfs_add_or_obtain(dentry, fhandle, fattr, label); + if (IS_ERR(d)) + return PTR_ERR(d); + + /* Callers don't care */ + dput(d); + return 0; } EXPORT_SYMBOL_GPL(nfs_instantiate); diff --git a/include/linux/nfs_fs.h b/include/linux/nfs_fs.h index 0a11712a80e3..570a60c2f4f4 100644 --- a/include/linux/nfs_fs.h +++ b/include/linux/nfs_fs.h @@ -490,6 +490,9 @@ extern const struct file_operations nfs_dir_operations; extern const struct dentry_operations nfs_dentry_operations; extern void nfs_force_lookup_revalidate(struct inode *dir); +extern struct dentry *nfs_add_or_obtain(struct dentry *dentry, + struct nfs_fh *fh, struct nfs_fattr *fattr, + struct nfs4_label *label); extern int nfs_instantiate(struct dentry *dentry, struct nfs_fh *fh, struct nfs_fattr *fattr, struct nfs4_label *label); extern int nfs_may_open(struct inode *inode, const struct cred *cred, int openflags); -- 2.20.1 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* [PATCH 2/3] NFSv3: use nfs_add_or_obtain() to create and reference inodes 2019-09-13 12:29 [PATCH 0/3] nfs_instantiate() might succeed leaving dentry negative unhashed Benjamin Coddington 2019-09-13 12:29 ` [PATCH 1/3] NFS: Refactor nfs_instantiate() for dentry referencing callers Benjamin Coddington @ 2019-09-13 12:29 ` Benjamin Coddington 2019-09-13 13:02 ` Benjamin Coddington 2019-09-13 12:29 ` [PATCH 3/3] NFS: remove unused check for negative dentry Benjamin Coddington 2 siblings, 1 reply; 5+ messages in thread From: Benjamin Coddington @ 2019-09-13 12:29 UTC (permalink / raw) To: Trond Myklebust, Anna Schumaker; +Cc: Al Viro, linux-nfs --- fs/nfs/nfs3proc.c | 45 ++++++++++++++++++++++++++++++++++++--------- 1 file changed, 36 insertions(+), 9 deletions(-) diff --git a/fs/nfs/nfs3proc.c b/fs/nfs/nfs3proc.c index a3ad2d46fd42..9eb2f1a503ab 100644 --- a/fs/nfs/nfs3proc.c +++ b/fs/nfs/nfs3proc.c @@ -279,15 +279,17 @@ static struct nfs3_createdata *nfs3_alloc_createdata(void) return data; } -static int nfs3_do_create(struct inode *dir, struct dentry *dentry, struct nfs3_createdata *data) +static struct dentry * +nfs3_do_create(struct inode *dir, struct dentry *dentry, struct nfs3_createdata *data) { int status; status = rpc_call_sync(NFS_CLIENT(dir), &data->msg, 0); nfs_post_op_update_inode(dir, data->res.dir_attr); - if (status == 0) - status = nfs_instantiate(dentry, data->res.fh, data->res.fattr, NULL); - return status; + if (status != 0) + return ERR_PTR(status); + + return nfs_add_or_obtain(dentry, data->res.fh, data->res.fattr, NULL); } static void nfs3_free_createdata(struct nfs3_createdata *data) @@ -304,6 +306,7 @@ nfs3_proc_create(struct inode *dir, struct dentry *dentry, struct iattr *sattr, { struct posix_acl *default_acl, *acl; struct nfs3_createdata *data; + struct dentry *d_alias; int status = -ENOMEM; dprintk("NFS call create %pd\n", dentry); @@ -330,7 +333,8 @@ nfs3_proc_create(struct inode *dir, struct dentry *dentry, struct iattr *sattr, goto out; for (;;) { - status = nfs3_do_create(dir, dentry, data); + d_alias = nfs3_do_create(dir, dentry, data); + status = PTR_ERR_OR_ZERO(d_alias); if (status != -ENOTSUPP) break; @@ -355,6 +359,9 @@ nfs3_proc_create(struct inode *dir, struct dentry *dentry, struct iattr *sattr, if (status != 0) goto out_release_acls; + if (d_alias) + dentry = d_alias; + /* When we created the file with exclusive semantics, make * sure we set the attributes afterwards. */ if (data->arg.create.createmode == NFS3_CREATE_EXCLUSIVE) { @@ -372,11 +379,13 @@ nfs3_proc_create(struct inode *dir, struct dentry *dentry, struct iattr *sattr, nfs_post_op_update_inode(d_inode(dentry), data->res.fattr); dprintk("NFS reply setattr (post-create): %d\n", status); if (status != 0) - goto out_release_acls; + goto out_dput; } status = nfs3_proc_setacls(d_inode(dentry), acl, default_acl); +out_dput: + dput(d_alias); out_release_acls: posix_acl_release(acl); posix_acl_release(default_acl); @@ -504,6 +513,7 @@ nfs3_proc_symlink(struct inode *dir, struct dentry *dentry, struct page *page, unsigned int len, struct iattr *sattr) { struct nfs3_createdata *data; + struct dentry *d_alias; int status = -ENOMEM; if (len > NFS3_MAXPATHLEN) @@ -522,7 +532,11 @@ nfs3_proc_symlink(struct inode *dir, struct dentry *dentry, struct page *page, data->arg.symlink.pathlen = len; data->arg.symlink.sattr = sattr; - status = nfs3_do_create(dir, dentry, data); + d_alias = nfs3_do_create(dir, dentry, data); + status = PTR_ERR_OR_ZERO(d_alias); + + if (status == 0) + dput(d_alias); nfs3_free_createdata(data); out: @@ -535,6 +549,7 @@ nfs3_proc_mkdir(struct inode *dir, struct dentry *dentry, struct iattr *sattr) { struct posix_acl *default_acl, *acl; struct nfs3_createdata *data; + struct dentry *d_alias; int status = -ENOMEM; dprintk("NFS call mkdir %pd\n", dentry); @@ -553,12 +568,18 @@ nfs3_proc_mkdir(struct inode *dir, struct dentry *dentry, struct iattr *sattr) data->arg.mkdir.len = dentry->d_name.len; data->arg.mkdir.sattr = sattr; - status = nfs3_do_create(dir, dentry, data); + d_alias = nfs3_do_create(dir, dentry, data); + status = PTR_ERR_OR_ZERO(d_alias); + if (status != 0) goto out_release_acls; + if (d_alias) + dentry = d_alias; + status = nfs3_proc_setacls(d_inode(dentry), acl, default_acl); + dput(d_alias); out_release_acls: posix_acl_release(acl); posix_acl_release(default_acl); @@ -660,6 +681,7 @@ nfs3_proc_mknod(struct inode *dir, struct dentry *dentry, struct iattr *sattr, { struct posix_acl *default_acl, *acl; struct nfs3_createdata *data; + struct dentry *d_alias; int status = -ENOMEM; dprintk("NFS call mknod %pd %u:%u\n", dentry, @@ -698,12 +720,17 @@ nfs3_proc_mknod(struct inode *dir, struct dentry *dentry, struct iattr *sattr, goto out; } - status = nfs3_do_create(dir, dentry, data); + d_alias = nfs3_do_create(dir, dentry, data); + status = PTR_ERR_OR_ZERO(d_alias); if (status != 0) goto out_release_acls; + if (d_alias) + dentry = d_alias; + status = nfs3_proc_setacls(d_inode(dentry), acl, default_acl); + dput(d_alias); out_release_acls: posix_acl_release(acl); posix_acl_release(default_acl); -- 2.20.1 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH 2/3] NFSv3: use nfs_add_or_obtain() to create and reference inodes 2019-09-13 12:29 ` [PATCH 2/3] NFSv3: use nfs_add_or_obtain() to create and reference inodes Benjamin Coddington @ 2019-09-13 13:02 ` Benjamin Coddington 0 siblings, 0 replies; 5+ messages in thread From: Benjamin Coddington @ 2019-09-13 13:02 UTC (permalink / raw) To: Trond Myklebust, Anna Schumaker; +Cc: Al Viro, linux-nfs On 13 Sep 2019, at 8:29, Benjamin Coddington wrote: mm.. not sure what happened to the body here.. maybe I didn't have one. Should at least have: Signed-off-by: Benjamin Coddington <bcodding@redhat.com> > --- > fs/nfs/nfs3proc.c | 45 ++++++++++++++++++++++++++++++++++++--------- > 1 file changed, 36 insertions(+), 9 deletions(-) > > diff --git a/fs/nfs/nfs3proc.c b/fs/nfs/nfs3proc.c > index a3ad2d46fd42..9eb2f1a503ab 100644 > --- a/fs/nfs/nfs3proc.c > +++ b/fs/nfs/nfs3proc.c > @@ -279,15 +279,17 @@ static struct nfs3_createdata > *nfs3_alloc_createdata(void) > return data; > } > > -static int nfs3_do_create(struct inode *dir, struct dentry *dentry, > struct nfs3_createdata *data) > +static struct dentry * > +nfs3_do_create(struct inode *dir, struct dentry *dentry, struct > nfs3_createdata *data) > { > int status; > > status = rpc_call_sync(NFS_CLIENT(dir), &data->msg, 0); > nfs_post_op_update_inode(dir, data->res.dir_attr); > - if (status == 0) > - status = nfs_instantiate(dentry, data->res.fh, data->res.fattr, > NULL); > - return status; > + if (status != 0) > + return ERR_PTR(status); > + > + return nfs_add_or_obtain(dentry, data->res.fh, data->res.fattr, > NULL); > } > > static void nfs3_free_createdata(struct nfs3_createdata *data) > @@ -304,6 +306,7 @@ nfs3_proc_create(struct inode *dir, struct dentry > *dentry, struct iattr *sattr, > { > struct posix_acl *default_acl, *acl; > struct nfs3_createdata *data; > + struct dentry *d_alias; > int status = -ENOMEM; > > dprintk("NFS call create %pd\n", dentry); > @@ -330,7 +333,8 @@ nfs3_proc_create(struct inode *dir, struct dentry > *dentry, struct iattr *sattr, > goto out; > > for (;;) { > - status = nfs3_do_create(dir, dentry, data); > + d_alias = nfs3_do_create(dir, dentry, data); > + status = PTR_ERR_OR_ZERO(d_alias); > > if (status != -ENOTSUPP) > break; > @@ -355,6 +359,9 @@ nfs3_proc_create(struct inode *dir, struct dentry > *dentry, struct iattr *sattr, > if (status != 0) > goto out_release_acls; > > + if (d_alias) > + dentry = d_alias; > + > /* When we created the file with exclusive semantics, make > * sure we set the attributes afterwards. */ > if (data->arg.create.createmode == NFS3_CREATE_EXCLUSIVE) { > @@ -372,11 +379,13 @@ nfs3_proc_create(struct inode *dir, struct > dentry *dentry, struct iattr *sattr, > nfs_post_op_update_inode(d_inode(dentry), data->res.fattr); > dprintk("NFS reply setattr (post-create): %d\n", status); > if (status != 0) > - goto out_release_acls; > + goto out_dput; > } > > status = nfs3_proc_setacls(d_inode(dentry), acl, default_acl); > > +out_dput: > + dput(d_alias); > out_release_acls: > posix_acl_release(acl); > posix_acl_release(default_acl); > @@ -504,6 +513,7 @@ nfs3_proc_symlink(struct inode *dir, struct dentry > *dentry, struct page *page, > unsigned int len, struct iattr *sattr) > { > struct nfs3_createdata *data; > + struct dentry *d_alias; > int status = -ENOMEM; > > if (len > NFS3_MAXPATHLEN) > @@ -522,7 +532,11 @@ nfs3_proc_symlink(struct inode *dir, struct > dentry *dentry, struct page *page, > data->arg.symlink.pathlen = len; > data->arg.symlink.sattr = sattr; > > - status = nfs3_do_create(dir, dentry, data); > + d_alias = nfs3_do_create(dir, dentry, data); > + status = PTR_ERR_OR_ZERO(d_alias); > + > + if (status == 0) > + dput(d_alias); > > nfs3_free_createdata(data); > out: > @@ -535,6 +549,7 @@ nfs3_proc_mkdir(struct inode *dir, struct dentry > *dentry, struct iattr *sattr) > { > struct posix_acl *default_acl, *acl; > struct nfs3_createdata *data; > + struct dentry *d_alias; > int status = -ENOMEM; > > dprintk("NFS call mkdir %pd\n", dentry); > @@ -553,12 +568,18 @@ nfs3_proc_mkdir(struct inode *dir, struct dentry > *dentry, struct iattr *sattr) > data->arg.mkdir.len = dentry->d_name.len; > data->arg.mkdir.sattr = sattr; > > - status = nfs3_do_create(dir, dentry, data); > + d_alias = nfs3_do_create(dir, dentry, data); > + status = PTR_ERR_OR_ZERO(d_alias); > + > if (status != 0) > goto out_release_acls; > > + if (d_alias) > + dentry = d_alias; > + > status = nfs3_proc_setacls(d_inode(dentry), acl, default_acl); > > + dput(d_alias); > out_release_acls: > posix_acl_release(acl); > posix_acl_release(default_acl); > @@ -660,6 +681,7 @@ nfs3_proc_mknod(struct inode *dir, struct dentry > *dentry, struct iattr *sattr, > { > struct posix_acl *default_acl, *acl; > struct nfs3_createdata *data; > + struct dentry *d_alias; > int status = -ENOMEM; > > dprintk("NFS call mknod %pd %u:%u\n", dentry, > @@ -698,12 +720,17 @@ nfs3_proc_mknod(struct inode *dir, struct dentry > *dentry, struct iattr *sattr, > goto out; > } > > - status = nfs3_do_create(dir, dentry, data); > + d_alias = nfs3_do_create(dir, dentry, data); > + status = PTR_ERR_OR_ZERO(d_alias); > if (status != 0) > goto out_release_acls; > > + if (d_alias) > + dentry = d_alias; > + > status = nfs3_proc_setacls(d_inode(dentry), acl, default_acl); > > + dput(d_alias); > out_release_acls: > posix_acl_release(acl); > posix_acl_release(default_acl); > -- > 2.20.1 ^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH 3/3] NFS: remove unused check for negative dentry 2019-09-13 12:29 [PATCH 0/3] nfs_instantiate() might succeed leaving dentry negative unhashed Benjamin Coddington 2019-09-13 12:29 ` [PATCH 1/3] NFS: Refactor nfs_instantiate() for dentry referencing callers Benjamin Coddington 2019-09-13 12:29 ` [PATCH 2/3] NFSv3: use nfs_add_or_obtain() to create and reference inodes Benjamin Coddington @ 2019-09-13 12:29 ` Benjamin Coddington 2 siblings, 0 replies; 5+ messages in thread From: Benjamin Coddington @ 2019-09-13 12:29 UTC (permalink / raw) To: Trond Myklebust, Anna Schumaker; +Cc: Al Viro, linux-nfs This check has been hanging out since we used to have parallel paths to add dentry in nfs_create(), but that hasn't been the case for some years. Signed-off-by: Benjamin Coddington <bcodding@redhat.com> --- fs/nfs/dir.c | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c index cb6ee1b41ab5..e180033e35cf 100644 --- a/fs/nfs/dir.c +++ b/fs/nfs/dir.c @@ -1677,15 +1677,11 @@ nfs_add_or_obtain(struct dentry *dentry, struct nfs_fh *fhandle, struct dentry *parent = dget_parent(dentry); struct inode *dir = d_inode(parent); struct inode *inode; - struct dentry *d = NULL; + struct dentry *d; int error; d_drop(dentry); - /* We may have been initialized further down */ - if (d_really_is_positive(dentry)) - goto out; - if (fhandle->size == 0) { error = NFS_PROTO(dir)->lookup(dir, &dentry->d_name, fhandle, fattr, NULL); if (error) -- 2.20.1 ^ permalink raw reply related [flat|nested] 5+ messages in thread
end of thread, other threads:[~2019-09-13 13:02 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-09-13 12:29 [PATCH 0/3] nfs_instantiate() might succeed leaving dentry negative unhashed Benjamin Coddington 2019-09-13 12:29 ` [PATCH 1/3] NFS: Refactor nfs_instantiate() for dentry referencing callers Benjamin Coddington 2019-09-13 12:29 ` [PATCH 2/3] NFSv3: use nfs_add_or_obtain() to create and reference inodes Benjamin Coddington 2019-09-13 13:02 ` Benjamin Coddington 2019-09-13 12:29 ` [PATCH 3/3] NFS: remove unused check for negative dentry Benjamin Coddington
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).