| Message ID | 164878611050.25542.6758961460499392000@noble.neil.brown.name (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v4] VFS: filename_create(): fix incorrect intent. | expand |
On Fri, 01 Apr 2022 15:08:30 +1100, NeilBrown wrote: > When asked to create a path ending '/', but which is not to be a > directory (LOOKUP_DIRECTORY not set), filename_create() will never try > to create the file. If it doesn't exist, -ENOENT is reported. > > However, it still passes LOOKUP_CREATE|LOOKUP_EXCL to the filesystems > ->lookup() function, even though there is no intent to create. This is > misleading and can cause incorrect behaviour. > > If you try > ln -s foo /path/dir/ It'd be helpful if we could run these sorts of tests from the xfstests suite. I wonder whether some sort of other-client ssh backchannel would be useful (for cifs.ko and cephfs too). > where 'dir' is a directory on an NFS filesystem which is not currently > known in the dcache, this will fail with ENOENT. > As the name is not in the dcache, nfs_lookup gets called with > LOOKUP_CREATE|LOOKUP_EXCL and so it returns NULL without performing any > lookup, with the expectation that a subsequent call to create the > target will be made, and the lookup can be combined with the creation. > In the case with a trailing '/' and no LOOKUP_DIRECTORY, that call is never > made. Instead filename_create() sees that the dentry is not (yet) > positive and returns -ENOENT - even though the directory actually > exists. > > So only set LOOKUP_CREATE|LOOKUP_EXCL if there really is an intent > to create, and use the absence of these flags to decide if -ENOENT > should be returned. > > Note that filename_parentat() is only interested in LOOKUP_REVAL, so we > split that out and store it in 'reval_flag'. > __looku_hash() then gets reval_flag combined with whatever create flags > were determined to be needed. nit: __lookup_hash() > > Signed-off-by: NeilBrown <neilb@suse.de> > --- > fs/namei.c | 22 ++++++++++------------ > 1 file changed, 10 insertions(+), 12 deletions(-) > > ARG - v3 had a missing semi-colon. Sorry. > > diff --git a/fs/namei.c b/fs/namei.c > index 3f1829b3ab5b..509657fdf4f5 100644 > --- a/fs/namei.c > +++ b/fs/namei.c > @@ -3673,18 +3673,14 @@ static struct dentry *filename_create(int dfd, struct filename *name, > { > struct dentry *dentry = ERR_PTR(-EEXIST); > struct qstr last; > + bool want_dir = lookup_flags & LOOKUP_DIRECTORY; > + unsigned int reval_flag = lookup_flags & LOOKUP_REVAL; > + unsigned int create_flags = LOOKUP_CREATE | LOOKUP_EXCL; > int type; > int err2; > int error; > - bool is_dir = (lookup_flags & LOOKUP_DIRECTORY); > > - /* > - * Note that only LOOKUP_REVAL and LOOKUP_DIRECTORY matter here. Any > - * other flags passed in are ignored! > - */ > - lookup_flags &= LOOKUP_REVAL; > - > - error = filename_parentat(dfd, name, lookup_flags, path, &last, &type); > + error = filename_parentat(dfd, name, reval_flag, path, &last, &type); > if (error) > return ERR_PTR(error); > > @@ -3698,11 +3694,13 @@ static struct dentry *filename_create(int dfd, struct filename *name, > /* don't fail immediately if it's r/o, at least try to report other errors */ > err2 = mnt_want_write(path->mnt); > /* > - * Do the final lookup. > + * Do the final lookup. Suppress 'create' if there is a trailing > + * '/', and a directory wasn't requested. > */ > - lookup_flags |= LOOKUP_CREATE | LOOKUP_EXCL; > + if (last.name[last.len] && !want_dir) > + create_flags = 0; > inode_lock_nested(path->dentry->d_inode, I_MUTEX_PARENT); > - dentry = __lookup_hash(&last, path->dentry, lookup_flags); > + dentry = __lookup_hash(&last, path->dentry, reval_flag | create_flags); > if (IS_ERR(dentry)) > goto unlock; > > @@ -3716,7 +3714,7 @@ static struct dentry *filename_create(int dfd, struct filename *name, > * all is fine. Let's be bastards - you had / on the end, you've > * been asking for (non-existent) directory. -ENOENT for you. > */ > - if (unlikely(!is_dir && last.name[last.len])) { > + if (unlikely(!create_flags)) { > error = -ENOENT; > goto fail; > } Looks good and works for me. Reviewed-by: David Disseldorp <ddiss@suse.de>
On Fri, 2022-04-01 at 15:08 +1100, NeilBrown wrote: > When asked to create a path ending '/', but which is not to be a > directory (LOOKUP_DIRECTORY not set), filename_create() will never try > to create the file. If it doesn't exist, -ENOENT is reported. > > However, it still passes LOOKUP_CREATE|LOOKUP_EXCL to the filesystems > ->lookup() function, even though there is no intent to create. This is > misleading and can cause incorrect behaviour. > > If you try > ln -s foo /path/dir/ > > where 'dir' is a directory on an NFS filesystem which is not currently > known in the dcache, this will fail with ENOENT. > As the name is not in the dcache, nfs_lookup gets called with > LOOKUP_CREATE|LOOKUP_EXCL and so it returns NULL without performing any > lookup, with the expectation that a subsequent call to create the > target will be made, and the lookup can be combined with the creation. > In the case with a trailing '/' and no LOOKUP_DIRECTORY, that call is never > made. Instead filename_create() sees that the dentry is not (yet) > positive and returns -ENOENT - even though the directory actually > exists. > > So only set LOOKUP_CREATE|LOOKUP_EXCL if there really is an intent > to create, and use the absence of these flags to decide if -ENOENT > should be returned. > > Note that filename_parentat() is only interested in LOOKUP_REVAL, so we > split that out and store it in 'reval_flag'. > __looku_hash() then gets reval_flag combined with whatever create flags > were determined to be needed. > > Signed-off-by: NeilBrown <neilb@suse.de> > --- > fs/namei.c | 22 ++++++++++------------ > 1 file changed, 10 insertions(+), 12 deletions(-) > > ARG - v3 had a missing semi-colon. Sorry. > > diff --git a/fs/namei.c b/fs/namei.c > index 3f1829b3ab5b..509657fdf4f5 100644 > --- a/fs/namei.c > +++ b/fs/namei.c > @@ -3673,18 +3673,14 @@ static struct dentry *filename_create(int dfd, struct filename *name, > { > struct dentry *dentry = ERR_PTR(-EEXIST); > struct qstr last; > + bool want_dir = lookup_flags & LOOKUP_DIRECTORY; > + unsigned int reval_flag = lookup_flags & LOOKUP_REVAL; > + unsigned int create_flags = LOOKUP_CREATE | LOOKUP_EXCL; > int type; > int err2; > int error; > - bool is_dir = (lookup_flags & LOOKUP_DIRECTORY); > > - /* > - * Note that only LOOKUP_REVAL and LOOKUP_DIRECTORY matter here. Any > - * other flags passed in are ignored! > - */ > - lookup_flags &= LOOKUP_REVAL; > - > - error = filename_parentat(dfd, name, lookup_flags, path, &last, &type); > + error = filename_parentat(dfd, name, reval_flag, path, &last, &type); > if (error) > return ERR_PTR(error); > > @@ -3698,11 +3694,13 @@ static struct dentry *filename_create(int dfd, struct filename *name, > /* don't fail immediately if it's r/o, at least try to report other errors */ > err2 = mnt_want_write(path->mnt); > /* > - * Do the final lookup. > + * Do the final lookup. Suppress 'create' if there is a trailing > + * '/', and a directory wasn't requested. > */ > - lookup_flags |= LOOKUP_CREATE | LOOKUP_EXCL; > + if (last.name[last.len] && !want_dir) > + create_flags = 0; > inode_lock_nested(path->dentry->d_inode, I_MUTEX_PARENT); > - dentry = __lookup_hash(&last, path->dentry, lookup_flags); > + dentry = __lookup_hash(&last, path->dentry, reval_flag | create_flags); > if (IS_ERR(dentry)) > goto unlock; > > @@ -3716,7 +3714,7 @@ static struct dentry *filename_create(int dfd, struct filename *name, > * all is fine. Let's be bastards - you had / on the end, you've > * been asking for (non-existent) directory. -ENOENT for you. > */ > - if (unlikely(!is_dir && last.name[last.len])) { > + if (unlikely(!create_flags)) { > error = -ENOENT; > goto fail; > } Reviewed-by: Jeff Layton <jlayton@kernel.org>
diff --git a/fs/namei.c b/fs/namei.c index 3f1829b3ab5b..509657fdf4f5 100644 --- a/fs/namei.c +++ b/fs/namei.c @@ -3673,18 +3673,14 @@ static struct dentry *filename_create(int dfd, struct filename *name, { struct dentry *dentry = ERR_PTR(-EEXIST); struct qstr last; + bool want_dir = lookup_flags & LOOKUP_DIRECTORY; + unsigned int reval_flag = lookup_flags & LOOKUP_REVAL; + unsigned int create_flags = LOOKUP_CREATE | LOOKUP_EXCL; int type; int err2; int error; - bool is_dir = (lookup_flags & LOOKUP_DIRECTORY); - /* - * Note that only LOOKUP_REVAL and LOOKUP_DIRECTORY matter here. Any - * other flags passed in are ignored! - */ - lookup_flags &= LOOKUP_REVAL; - - error = filename_parentat(dfd, name, lookup_flags, path, &last, &type); + error = filename_parentat(dfd, name, reval_flag, path, &last, &type); if (error) return ERR_PTR(error); @@ -3698,11 +3694,13 @@ static struct dentry *filename_create(int dfd, struct filename *name, /* don't fail immediately if it's r/o, at least try to report other errors */ err2 = mnt_want_write(path->mnt); /* - * Do the final lookup. + * Do the final lookup. Suppress 'create' if there is a trailing + * '/', and a directory wasn't requested. */ - lookup_flags |= LOOKUP_CREATE | LOOKUP_EXCL; + if (last.name[last.len] && !want_dir) + create_flags = 0; inode_lock_nested(path->dentry->d_inode, I_MUTEX_PARENT); - dentry = __lookup_hash(&last, path->dentry, lookup_flags); + dentry = __lookup_hash(&last, path->dentry, reval_flag | create_flags); if (IS_ERR(dentry)) goto unlock; @@ -3716,7 +3714,7 @@ static struct dentry *filename_create(int dfd, struct filename *name, * all is fine. Let's be bastards - you had / on the end, you've * been asking for (non-existent) directory. -ENOENT for you. */ - if (unlikely(!is_dir && last.name[last.len])) { + if (unlikely(!create_flags)) { error = -ENOENT; goto fail; }
When asked to create a path ending '/', but which is not to be a directory (LOOKUP_DIRECTORY not set), filename_create() will never try to create the file. If it doesn't exist, -ENOENT is reported. However, it still passes LOOKUP_CREATE|LOOKUP_EXCL to the filesystems ->lookup() function, even though there is no intent to create. This is misleading and can cause incorrect behaviour. If you try ln -s foo /path/dir/ where 'dir' is a directory on an NFS filesystem which is not currently known in the dcache, this will fail with ENOENT. As the name is not in the dcache, nfs_lookup gets called with LOOKUP_CREATE|LOOKUP_EXCL and so it returns NULL without performing any lookup, with the expectation that a subsequent call to create the target will be made, and the lookup can be combined with the creation. In the case with a trailing '/' and no LOOKUP_DIRECTORY, that call is never made. Instead filename_create() sees that the dentry is not (yet) positive and returns -ENOENT - even though the directory actually exists. So only set LOOKUP_CREATE|LOOKUP_EXCL if there really is an intent to create, and use the absence of these flags to decide if -ENOENT should be returned. Note that filename_parentat() is only interested in LOOKUP_REVAL, so we split that out and store it in 'reval_flag'. __looku_hash() then gets reval_flag combined with whatever create flags were determined to be needed. Signed-off-by: NeilBrown <neilb@suse.de> --- fs/namei.c | 22 ++++++++++------------ 1 file changed, 10 insertions(+), 12 deletions(-) ARG - v3 had a missing semi-colon. Sorry.