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@xxxxxxx>
> ---
> 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@xxxxxxxxxx>
