| Message ID | 164842900895.6096.10753358086437966517@noble.neil.brown.name (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | VFS: filename_create(): fix incorrect intent. | expand |
On Mon, 2022-03-28 at 11:56 +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 as 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. > > Signed-off-by: NeilBrown <neilb@suse.de> > --- > fs/namei.c | 9 +++++---- > 1 file changed, 5 insertions(+), 4 deletions(-) > > diff --git a/fs/namei.c b/fs/namei.c > index 3f1829b3ab5b..3ffb42e56a8e 100644 > --- a/fs/namei.c > +++ b/fs/namei.c > @@ -3676,7 +3676,6 @@ static struct dentry *filename_create(int dfd, struct filename *name, > int type; > int err2; > int error; > - bool is_dir = (lookup_flags & LOOKUP_DIRECTORY); > > /* > * Note that only LOOKUP_REVAL and LOOKUP_DIRECTORY matter here. Any > @@ -3698,9 +3697,11 @@ 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. Request 'create' only if there is no trailing > + * '/', or if directory is requested. > */ > - lookup_flags |= LOOKUP_CREATE | LOOKUP_EXCL; > + if (!last.name[last.len] || (lookup_flags & LOOKUP_DIRECTORY)) > + lookup_flags |= LOOKUP_CREATE | LOOKUP_EXCL; > inode_lock_nested(path->dentry->d_inode, I_MUTEX_PARENT); > dentry = __lookup_hash(&last, path->dentry, lookup_flags); > if (IS_ERR(dentry)) > @@ -3716,7 +3717,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 (!likely(lookup_flags & LOOKUP_CREATE)) { > error = -ENOENT; > goto fail; > } Seems like a sane enough fix. Nice catch. Reviewed-by: Jeff Layton <jlayton@kernel.org>
Hi Neil, I gave this a spin and was wondering why xfstests wouldn't start with this change... On Mon, 28 Mar 2022 11:56:48 +1100, NeilBrown wrote: ... > > diff --git a/fs/namei.c b/fs/namei.c > index 3f1829b3ab5b..3ffb42e56a8e 100644 > --- a/fs/namei.c > +++ b/fs/namei.c > @@ -3676,7 +3676,6 @@ static struct dentry *filename_create(int dfd, struct filename *name, > int type; > int err2; > int error; > - bool is_dir = (lookup_flags & LOOKUP_DIRECTORY); > > /* > * Note that only LOOKUP_REVAL and LOOKUP_DIRECTORY matter here. Any > @@ -3698,9 +3697,11 @@ 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. Request 'create' only if there is no trailing > + * '/', or if directory is requested. > */ > - lookup_flags |= LOOKUP_CREATE | LOOKUP_EXCL; > + if (!last.name[last.len] || (lookup_flags & LOOKUP_DIRECTORY)) > + lookup_flags |= LOOKUP_CREATE | LOOKUP_EXCL; This doesn't look right, as any LOOKUP_DIRECTORY flag gets dropped via the prior "lookup_flags &= LOOKUP_REVAL;". Cheers, David
On Wed, 30 Mar 2022, David Disseldorp wrote: > Hi Neil, > > I gave this a spin and was wondering why xfstests wouldn't start with > this change... > > On Mon, 28 Mar 2022 11:56:48 +1100, NeilBrown wrote: > ... > > > > diff --git a/fs/namei.c b/fs/namei.c > > index 3f1829b3ab5b..3ffb42e56a8e 100644 > > --- a/fs/namei.c > > +++ b/fs/namei.c > > @@ -3676,7 +3676,6 @@ static struct dentry *filename_create(int dfd, struct filename *name, > > int type; > > int err2; > > int error; > > - bool is_dir = (lookup_flags & LOOKUP_DIRECTORY); > > > > /* > > * Note that only LOOKUP_REVAL and LOOKUP_DIRECTORY matter here. Any > > @@ -3698,9 +3697,11 @@ 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. Request 'create' only if there is no trailing > > + * '/', or if directory is requested. > > */ > > - lookup_flags |= LOOKUP_CREATE | LOOKUP_EXCL; > > + if (!last.name[last.len] || (lookup_flags & LOOKUP_DIRECTORY)) > > + lookup_flags |= LOOKUP_CREATE | LOOKUP_EXCL; > > This doesn't look right, as any LOOKUP_DIRECTORY flag gets dropped via > the prior "lookup_flags &= LOOKUP_REVAL;". Arg.. thanks for testing - I clearly should have tested more broadly. I could leave the "is_dir" variable there I guess. Or maybe the masking statement should be lookup_flags &= LOOKUP_REVAL | LOOKUP_DIRECTORY; as that is a better match for the comment. Thanks, NeilBrown
On Thu, 31 Mar 2022 09:59:48 +1100, NeilBrown wrote: > On Wed, 30 Mar 2022, David Disseldorp wrote: > > Hi Neil, > > > > I gave this a spin and was wondering why xfstests wouldn't start with > > this change... > > > > On Mon, 28 Mar 2022 11:56:48 +1100, NeilBrown wrote: > > ... > > > > > > diff --git a/fs/namei.c b/fs/namei.c > > > index 3f1829b3ab5b..3ffb42e56a8e 100644 > > > --- a/fs/namei.c > > > +++ b/fs/namei.c > > > @@ -3676,7 +3676,6 @@ static struct dentry *filename_create(int dfd, struct filename *name, > > > int type; > > > int err2; > > > int error; > > > - bool is_dir = (lookup_flags & LOOKUP_DIRECTORY); > > > > > > /* > > > * Note that only LOOKUP_REVAL and LOOKUP_DIRECTORY matter here. Any > > > @@ -3698,9 +3697,11 @@ 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. Request 'create' only if there is no trailing > > > + * '/', or if directory is requested. > > > */ > > > - lookup_flags |= LOOKUP_CREATE | LOOKUP_EXCL; > > > + if (!last.name[last.len] || (lookup_flags & LOOKUP_DIRECTORY)) > > > + lookup_flags |= LOOKUP_CREATE | LOOKUP_EXCL; > > > > This doesn't look right, as any LOOKUP_DIRECTORY flag gets dropped via > > the prior "lookup_flags &= LOOKUP_REVAL;". > > Arg.. thanks for testing - I clearly should have tested more broadly. > > I could leave the "is_dir" variable there I guess. > Or maybe the masking statement should be > lookup_flags &= LOOKUP_REVAL | LOOKUP_DIRECTORY; > as that is a better match for the comment. Modifying "lookup_flags" results in changed filename_parentat() and __lookup_hash() parameters, which isn't an intended consequence IIUC. I think retaining "is_dir" would make sense. Cheers, David
On Thu, 31 Mar 2022, David Disseldorp wrote: > On Thu, 31 Mar 2022 09:59:48 +1100, NeilBrown wrote: > > > On Wed, 30 Mar 2022, David Disseldorp wrote: > > > Hi Neil, > > > > > > I gave this a spin and was wondering why xfstests wouldn't start with > > > this change... > > > > > > On Mon, 28 Mar 2022 11:56:48 +1100, NeilBrown wrote: > > > ... > > > > > > > > diff --git a/fs/namei.c b/fs/namei.c > > > > index 3f1829b3ab5b..3ffb42e56a8e 100644 > > > > --- a/fs/namei.c > > > > +++ b/fs/namei.c > > > > @@ -3676,7 +3676,6 @@ static struct dentry *filename_create(int dfd, struct filename *name, > > > > int type; > > > > int err2; > > > > int error; > > > > - bool is_dir = (lookup_flags & LOOKUP_DIRECTORY); > > > > > > > > /* > > > > * Note that only LOOKUP_REVAL and LOOKUP_DIRECTORY matter here. Any > > > > @@ -3698,9 +3697,11 @@ 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. Request 'create' only if there is no trailing > > > > + * '/', or if directory is requested. > > > > */ > > > > - lookup_flags |= LOOKUP_CREATE | LOOKUP_EXCL; > > > > + if (!last.name[last.len] || (lookup_flags & LOOKUP_DIRECTORY)) > > > > + lookup_flags |= LOOKUP_CREATE | LOOKUP_EXCL; > > > > > > This doesn't look right, as any LOOKUP_DIRECTORY flag gets dropped via > > > the prior "lookup_flags &= LOOKUP_REVAL;". > > > > Arg.. thanks for testing - I clearly should have tested more broadly. > > > > I could leave the "is_dir" variable there I guess. > > Or maybe the masking statement should be > > lookup_flags &= LOOKUP_REVAL | LOOKUP_DIRECTORY; > > as that is a better match for the comment. > > Modifying "lookup_flags" results in changed filename_parentat() and > __lookup_hash() parameters, which isn't an intended consequence IIUC. I > think retaining "is_dir" would make sense. I think retaining is_dir is ugly. Given that LOOKUP_DIRECTORY is meaningful, why mask it off? The only flag *ever* passed to filename_parentat() is LOOKUP_REVAL, so maybe it would make sense to change the parameter to be called "reval" to make the meaning more obvious. The only other use of lookup_flags is to pass it to ->lookup(). I guess LOOKUP_DIRECTORY isn't really meaningful there .. though it does say "this lookup is never for a non-directory"... might that be helpful? Maybe I'll have another look in the morning. Thanks, NeilBrown
diff --git a/fs/namei.c b/fs/namei.c index 3f1829b3ab5b..3ffb42e56a8e 100644 --- a/fs/namei.c +++ b/fs/namei.c @@ -3676,7 +3676,6 @@ static struct dentry *filename_create(int dfd, struct filename *name, int type; int err2; int error; - bool is_dir = (lookup_flags & LOOKUP_DIRECTORY); /* * Note that only LOOKUP_REVAL and LOOKUP_DIRECTORY matter here. Any @@ -3698,9 +3697,11 @@ 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. Request 'create' only if there is no trailing + * '/', or if directory is requested. */ - lookup_flags |= LOOKUP_CREATE | LOOKUP_EXCL; + if (!last.name[last.len] || (lookup_flags & LOOKUP_DIRECTORY)) + lookup_flags |= LOOKUP_CREATE | LOOKUP_EXCL; inode_lock_nested(path->dentry->d_inode, I_MUTEX_PARENT); dentry = __lookup_hash(&last, path->dentry, lookup_flags); if (IS_ERR(dentry)) @@ -3716,7 +3717,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 (!likely(lookup_flags & LOOKUP_CREATE)) { 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 as 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. Signed-off-by: NeilBrown <neilb@suse.de> --- fs/namei.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-)