[PATCH] VFS: filename_create(): fix incorrect intent.

[PATCH] VFS: filename_create(): fix incorrect intent.

[NeilBrown]

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;
 	}
-- 
2.35.1

Re: [PATCH] VFS: filename_create(): fix incorrect intent.

[Jeff Layton]

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>

Re: [PATCH] VFS: filename_create(): fix incorrect intent.

[David Disseldorp]

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

Re: [PATCH] VFS: filename_create(): fix incorrect intent.

[NeilBrown]

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

Re: [PATCH] VFS: filename_create(): fix incorrect intent.

[David Disseldorp]

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

Re: [PATCH] VFS: filename_create(): fix incorrect intent.

[NeilBrown]

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