Re: [PATCH] NFSv3: nfs_instantiate() might succeed leaving dentry negative unhashed

["Benjamin Coddington" <bcodding@redhat.com>]

On 27 Aug 2019, at 7:46, Trond Myklebust wrote:
> On Tue, 2019-08-27 at 06:33 -0400, Benjamin Coddington wrote:
>> On 26 Aug 2019, at 16:39, Trond Myklebust wrote:
>>> If this is a consequence of a race in nfs_instantiate, then why are
>>> we
>>> not fix it there? Won't we otherwise end up having to duplicate the
>>> above code in all the other callers?
>>>
>>> IOW: why not simply modify nfs_instantiate() to return the dentry
>>> from
>>> d_splice_alias(), much like we already do for nfs_lookup()?
>>
>> None of the other callers care about the dentry and it seemed more
>> invasive.
>> It is also an accepted pattern for VFS - that's why Al justified it
>> in
>> b0c6108ecf64.
>
> It is racy, though. Nothing guarantees that a dentry for that file is
> still hashed under the same name when you look it up again, so it is
> better to pass it back directly from the d_splice_alias() call.
>
>> If you'd rather change all the callers, let me know and I can send
>> that.
>
> If you'd prefer not to have to change all the callers, then perhaps
> split the function into two parts:
> - The inner part that returns the dentry from d_splice_alias() on
> success, and which can be called directly from nfs3_do_create().
> - Then a wrapper that works like nfs_instantiate() by dput()ing the
> valid dentry (and returning 0) or otherwise converting the ERR_PTR()
> and returning that.

Ok, sounds fine.

One thing that strikes me as odd is the d_drop() at the top of
nfs_instantiate().  That seems wrong if the next check for positive bypasses
the work of hashing it again.

Can you give me a hint as to why the paths are that way?  Otherwise, I think
it should change as:

diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c
index 8d501093660f..7720a19b38d3 100644
--- a/fs/nfs/dir.c
+++ b/fs/nfs/dir.c
@@ -1682,11 +1682,12 @@ int nfs_instantiate(struct dentry *dentry, struct
nfs_fh *fhandle,
        struct dentry *d;
        int error = -EACCES;

-       d_drop(dentry);
-
        /* We may have been initialized further down */
        if (d_really_is_positive(dentry))
                goto out;
+
+       d_drop(dentry);

        if (fhandle->size == 0) {
                error = NFS_PROTO(dir)->lookup(dir, &dentry->d_name,
fhandle, fattr, NULL);
                if (error)

Ben