There have been reports of races that cause NFSv4 OPEN(CREATE) to
return an error even though the requested file was created. NFSv4
does not seem to provide a status code for this case.
There are plenty of things that can go wrong between the
vfs_create() call in do_nfsd_create() and nfs4_vfs_get_file(), but
one of them is another client looking up and modifying the file's
mode bits in that window. If that happens, the creator might lose
access to the file before it can be opened.
Instead of opening the newly created file in nfsd4_process_open2(),
open it as soon as the file is created, and leave it sitting in the
file cache.
This patch is not optimal, of course - we should replace the
vfs_create() call in do_nfsd_create() with a call that creates the
file and returns a "struct file *" that can be planted immediately
in nf->nf_file.
But first, I would like to hear opinions about the approach
suggested above.
BugLink: https://bugzilla.linux-nfs.org/show_bug.cgi?id=382
Signed-off-by: Chuck Lever <[email protected]>
---
fs/nfsd/vfs.c | 9 +++++++++
1 file changed, 9 insertions(+)
diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
index 02a544645b52..80b568fa12f1 100644
--- a/fs/nfsd/vfs.c
+++ b/fs/nfsd/vfs.c
@@ -1410,6 +1410,7 @@ do_nfsd_create(struct svc_rqst *rqstp, struct svc_fh *fhp,
__be32 err;
int host_err;
__u32 v_mtime=0, v_atime=0;
+ struct nfsd_file *nf;
/* XXX: Shouldn't this be nfserr_inval? */
err = nfserr_perm;
@@ -1535,6 +1536,14 @@ do_nfsd_create(struct svc_rqst *rqstp, struct svc_fh *fhp,
iap->ia_atime.tv_nsec = 0;
}
+ /* Attempt to instantiate a filecache entry while we still hold
+ * the parent's inode mutex. */
+ err = nfsd_file_acquire(rqstp, resfhp, NFSD_MAY_WRITE, &nf);
+ if (err)
+ /* This would be bad */
+ goto out;
+ nfsd_file_put(nf);
+
set_attr:
err = nfsd_create_setattr(rqstp, resfhp, iap);
On Thu, 2022-03-24 at 18:08 -0400, Chuck Lever wrote:
> There have been reports of races that cause NFSv4 OPEN(CREATE) to
> return an error even though the requested file was created. NFSv4
> does not seem to provide a status code for this case.
>
> There are plenty of things that can go wrong between the
> vfs_create() call in do_nfsd_create() and nfs4_vfs_get_file(), but
> one of them is another client looking up and modifying the file's
> mode bits in that window. If that happens, the creator might lose
> access to the file before it can be opened.
>
> Instead of opening the newly created file in nfsd4_process_open2(),
> open it as soon as the file is created, and leave it sitting in the
> file cache.
>
> This patch is not optimal, of course - we should replace the
> vfs_create() call in do_nfsd_create() with a call that creates the
> file and returns a "struct file *" that can be planted immediately
> in nf->nf_file.
>
> But first, I would like to hear opinions about the approach
> suggested above.
>
> BugLink: https://bugzilla.linux-nfs.org/show_bug.cgi?id=382
> Signed-off-by: Chuck Lever <[email protected]>
> ---
> fs/nfsd/vfs.c | 9 +++++++++
> 1 file changed, 9 insertions(+)
>
> diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
> index 02a544645b52..80b568fa12f1 100644
> --- a/fs/nfsd/vfs.c
> +++ b/fs/nfsd/vfs.c
> @@ -1410,6 +1410,7 @@ do_nfsd_create(struct svc_rqst *rqstp, struct
> svc_fh *fhp,
> __be32 err;
> int host_err;
> __u32 v_mtime=0, v_atime=0;
> + struct nfsd_file *nf;
>
> /* XXX: Shouldn't this be nfserr_inval? */
> err = nfserr_perm;
> @@ -1535,6 +1536,14 @@ do_nfsd_create(struct svc_rqst *rqstp, struct
> svc_fh *fhp,
> iap->ia_atime.tv_nsec = 0;
> }
>
> + /* Attempt to instantiate a filecache entry while we still
> hold
> + * the parent's inode mutex. */
> + err = nfsd_file_acquire(rqstp, resfhp, NFSD_MAY_WRITE, &nf);
> + if (err)
> + /* This would be bad */
> + goto out;
> + nfsd_file_put(nf);
Why rely on the file cache to carry the nfsd_file? Isn't it much easier
just to pass it back directly to the caller?
There are only 2 callers of do_nfsd_create(), so you'd have
nfsd3_proc_create() that will just call nfsd_file_put() as per above,
whereas the NFSv4 specific do_open_lookup() could just stash it in the
struct nfsd4_open so that it can get passed into do_open_permission()
and eventually nfsd4_process_open2().
> +
> set_attr:
> err = nfsd_create_setattr(rqstp, resfhp, iap);
>
>
>
--
Trond Myklebust
Linux NFS client maintainer, Hammerspace
[email protected]