> On Mar 24, 2022, at 6:51 PM, Trond Myklebust <trondmy@xxxxxxxxxxxxxxx> wrote: > > 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 <chuck.lever@xxxxxxxxxx> >> --- >> 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? My thought was that the "struct file *" has to end up in the filecache anyway, even in the NFSv3 case. If I do this right, I can avoid the subsequent call to nfsd_open_verified(), which seems to be the source of the racy chmod behavior I mentioned above. That might help NFSv3 too, which would need to recreate the "struct file *" in nfsd_read() or nfsd_write() anyway. > 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(). Neither nfsd4_process_open2() or do_open_permission() seem directly interested in the nfsd_file --- it's all under the covers, in nfs4_get_vfs_file(). But yes, a "struct nfsd4_open" is passed to and visible in nfs4_get_vfs_file(), as is the open->op_created boolean. >> + >> set_attr: >> err = nfsd_create_setattr(rqstp, resfhp, iap); >> >> >> > > -- > Trond Myklebust > Linux NFS client maintainer, Hammerspace > trond.myklebust@xxxxxxxxxxxxxxx -- Chuck Lever
