Received-SPF: pass (google.com: best guess record for domain of linux-nfs-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
From:   "Benjamin Coddington" <bcodding@redhat.com>
To:     "Trond Myklebust" <trondmy@hammerspace.com>
Cc:     linux-nfs@vger.kernel.org, Anna.Schumaker@netapp.com,
        viro@zeniv.linux.org.uk
Subject: Re: [PATCH] NFSv3: nfs_instantiate() might succeed leaving dentry
 negative unhashed
Date:   Wed, 28 Aug 2019 07:05:57 -0400
Message-ID: <19F3F549-6C84-4DD6-A8E9-2562DAE70CF0@redhat.com>
In-Reply-To: <720c018fc83192cdea73f8f26ca737e5ac393902.camel@hammerspace.com>
References: <d2076a27c1f3faa0d732e64d49bcbab054cae23b.1566850914.git.bcodding@redhat.com>
 <e3b9ff47b2b195796ac30e8580764ce549d3c325.camel@hammerspace.com>
 <D44A2F26-920E-427A-90E2-D800606EA748@redhat.com>
 <720c018fc83192cdea73f8f26ca737e5ac393902.camel@hammerspace.com>
MIME-Version: 1.0
Content-Type: text/plain
Sender: linux-nfs-owner@vger.kernel.org
Precedence: bulk

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