| Message ID | 20250123195242.1378601-3-cel@kernel.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Chuck Lever |
| Headers | show |
| Series | Avoid returning NFS4ERR_FILE_OPEN when not appropriate | expand |
On Thu, 2025-01-23 at 14:52 -0500, cel@kernel.org wrote: > From: Chuck Lever <chuck.lever@oracle.com> > > RFC 8881 Section 18.25.4 paragraph 5 tells us that the server > should return NFS4ERR_FILE_OPEN only if the target object is an > opened file. This suggests that returning this status when removing > a directory will confuse NFS clients. > > This is a version-specific issue; nfsd_proc_remove/rmdir() and > nfsd3_proc_remove/rmdir() already return nfserr_access as > appropriate. > > Unfortunately there is no quick way for nfsd4_remove() to determine > whether the target object is a file or not, so the check is done in > to nfsd_unlink() for now. > > Reported-by: Trond Myklebust <trondmy@hammerspace.com> > Fixes: 466e16f0920f ("nfsd: check for EBUSY from vfs_rmdir/vfs_unink.") > Signed-off-by: Chuck Lever <chuck.lever@oracle.com> > --- > fs/nfsd/vfs.c | 24 ++++++++++++++++++------ > 1 file changed, 18 insertions(+), 6 deletions(-) > > diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c > index 2d8e27c225f9..3ead7fb3bf04 100644 > --- a/fs/nfsd/vfs.c > +++ b/fs/nfsd/vfs.c > @@ -1931,9 +1931,17 @@ nfsd_rename(struct svc_rqst *rqstp, struct svc_fh *ffhp, char *fname, int flen, > return err; > } > > -/* > - * Unlink a file or directory > - * N.B. After this call fhp needs an fh_put > +/** > + * nfsd_unlink - remove a directory entry > + * @rqstp: RPC transaction context > + * @fhp: the file handle of the parent directory to be modified > + * @type: enforced file type of the object to be removed > + * @fname: the name of directory entry to be removed > + * @flen: length of @fname in octets > + * > + * After this call fhp needs an fh_put. > + * > + * Returns a generic NFS status code in network byte-order. > */ > __be32 > nfsd_unlink(struct svc_rqst *rqstp, struct svc_fh *fhp, int type, > @@ -2007,10 +2015,14 @@ nfsd_unlink(struct svc_rqst *rqstp, struct svc_fh *fhp, int type, > fh_drop_write(fhp); > out_nfserr: > if (host_err == -EBUSY) { > - /* name is mounted-on. There is no perfect > - * error status. > + /* > + * See RFC 8881 Section 18.25.4 para 4: NFSv4 REMOVE > + * distinguishes between reg file and dir. > */ > - err = nfserr_file_open; > + if (type != S_IFDIR) Should that be "if (type == S_ISREG)" instead? What if the inode is a named pipe or device file? I'm not sure you can ever get EBUSY with those, but in case you can, what's the right error in those cases? > + err = nfserr_file_open; > + else > + err = nfserr_acces; > } > out: > return err != nfs_ok ? err : nfserrno(host_err);
On 1/23/25 3:43 PM, Jeff Layton wrote: > On Thu, 2025-01-23 at 14:52 -0500, cel@kernel.org wrote: >> From: Chuck Lever <chuck.lever@oracle.com> >> >> RFC 8881 Section 18.25.4 paragraph 5 tells us that the server >> should return NFS4ERR_FILE_OPEN only if the target object is an >> opened file. This suggests that returning this status when removing >> a directory will confuse NFS clients. >> >> This is a version-specific issue; nfsd_proc_remove/rmdir() and >> nfsd3_proc_remove/rmdir() already return nfserr_access as >> appropriate. >> >> Unfortunately there is no quick way for nfsd4_remove() to determine >> whether the target object is a file or not, so the check is done in >> to nfsd_unlink() for now. >> >> Reported-by: Trond Myklebust <trondmy@hammerspace.com> >> Fixes: 466e16f0920f ("nfsd: check for EBUSY from vfs_rmdir/vfs_unink.") >> Signed-off-by: Chuck Lever <chuck.lever@oracle.com> >> --- >> fs/nfsd/vfs.c | 24 ++++++++++++++++++------ >> 1 file changed, 18 insertions(+), 6 deletions(-) >> >> diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c >> index 2d8e27c225f9..3ead7fb3bf04 100644 >> --- a/fs/nfsd/vfs.c >> +++ b/fs/nfsd/vfs.c >> @@ -1931,9 +1931,17 @@ nfsd_rename(struct svc_rqst *rqstp, struct svc_fh *ffhp, char *fname, int flen, >> return err; >> } >> >> -/* >> - * Unlink a file or directory >> - * N.B. After this call fhp needs an fh_put >> +/** >> + * nfsd_unlink - remove a directory entry >> + * @rqstp: RPC transaction context >> + * @fhp: the file handle of the parent directory to be modified >> + * @type: enforced file type of the object to be removed >> + * @fname: the name of directory entry to be removed >> + * @flen: length of @fname in octets >> + * >> + * After this call fhp needs an fh_put. >> + * >> + * Returns a generic NFS status code in network byte-order. >> */ >> __be32 >> nfsd_unlink(struct svc_rqst *rqstp, struct svc_fh *fhp, int type, >> @@ -2007,10 +2015,14 @@ nfsd_unlink(struct svc_rqst *rqstp, struct svc_fh *fhp, int type, >> fh_drop_write(fhp); >> out_nfserr: >> if (host_err == -EBUSY) { >> - /* name is mounted-on. There is no perfect >> - * error status. >> + /* >> + * See RFC 8881 Section 18.25.4 para 4: NFSv4 REMOVE >> + * distinguishes between reg file and dir. >> */ >> - err = nfserr_file_open; >> + if (type != S_IFDIR) > > Should that be "if (type == S_ISREG)" instead? What if the inode is a > named pipe or device file? I'm not sure you can ever get EBUSY with > those, but in case you can, what's the right error in those cases? Check out nfsd_unlink()'s callers to see what they pass as the type parameter. Unfortunately we have to compare against S_IFDIR here. >> + err = nfserr_file_open; >> + else >> + err = nfserr_acces; >> } >> out: >> return err != nfs_ok ? err : nfserrno(host_err); >
On Thu, Jan 23, 2025 at 10:07 PM Chuck Lever <chuck.lever@oracle.com> wrote: > > On 1/23/25 3:43 PM, Jeff Layton wrote: > > On Thu, 2025-01-23 at 14:52 -0500, cel@kernel.org wrote: > >> From: Chuck Lever <chuck.lever@oracle.com> > >> > >> RFC 8881 Section 18.25.4 paragraph 5 tells us that the server > >> should return NFS4ERR_FILE_OPEN only if the target object is an > >> opened file. This suggests that returning this status when removing > >> a directory will confuse NFS clients. > >> > >> This is a version-specific issue; nfsd_proc_remove/rmdir() and > >> nfsd3_proc_remove/rmdir() already return nfserr_access as > >> appropriate. > >> > >> Unfortunately there is no quick way for nfsd4_remove() to determine > >> whether the target object is a file or not, so the check is done in > >> to nfsd_unlink() for now. > >> > >> Reported-by: Trond Myklebust <trondmy@hammerspace.com> > >> Fixes: 466e16f0920f ("nfsd: check for EBUSY from vfs_rmdir/vfs_unink.") > >> Signed-off-by: Chuck Lever <chuck.lever@oracle.com> > >> --- > >> fs/nfsd/vfs.c | 24 ++++++++++++++++++------ > >> 1 file changed, 18 insertions(+), 6 deletions(-) > >> > >> diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c > >> index 2d8e27c225f9..3ead7fb3bf04 100644 > >> --- a/fs/nfsd/vfs.c > >> +++ b/fs/nfsd/vfs.c > >> @@ -1931,9 +1931,17 @@ nfsd_rename(struct svc_rqst *rqstp, struct svc_fh *ffhp, char *fname, int flen, > >> return err; > >> } > >> > >> -/* > >> - * Unlink a file or directory > >> - * N.B. After this call fhp needs an fh_put > >> +/** > >> + * nfsd_unlink - remove a directory entry > >> + * @rqstp: RPC transaction context > >> + * @fhp: the file handle of the parent directory to be modified > >> + * @type: enforced file type of the object to be removed > >> + * @fname: the name of directory entry to be removed > >> + * @flen: length of @fname in octets > >> + * > >> + * After this call fhp needs an fh_put. > >> + * > >> + * Returns a generic NFS status code in network byte-order. > >> */ > >> __be32 > >> nfsd_unlink(struct svc_rqst *rqstp, struct svc_fh *fhp, int type, > >> @@ -2007,10 +2015,14 @@ nfsd_unlink(struct svc_rqst *rqstp, struct svc_fh *fhp, int type, > >> fh_drop_write(fhp); > >> out_nfserr: > >> if (host_err == -EBUSY) { > >> - /* name is mounted-on. There is no perfect > >> - * error status. > >> + /* > >> + * See RFC 8881 Section 18.25.4 para 4: NFSv4 REMOVE > >> + * distinguishes between reg file and dir. > >> */ > >> - err = nfserr_file_open; > >> + if (type != S_IFDIR) > > > > Should that be "if (type == S_ISREG)" instead? What if the inode is a > > named pipe or device file? I'm not sure you can ever get EBUSY with > > those, but in case you can, what's the right error in those cases? > > Check out nfsd_unlink()'s callers to see what they pass as the type > parameter. Unfortunately we have to compare against S_IFDIR here. > Not exactly. nfsd4_remove() is the only caller that needs to get nfserr_file_open and this caller calls with type = 0, so type here is going to be the actual type of the inode and (type == S_ISREG) would be correct. No? Thanks, Amir.
On 1/24/25 5:42 AM, Amir Goldstein wrote: > On Thu, Jan 23, 2025 at 10:07 PM Chuck Lever <chuck.lever@oracle.com> wrote: >> >> On 1/23/25 3:43 PM, Jeff Layton wrote: >>> On Thu, 2025-01-23 at 14:52 -0500, cel@kernel.org wrote: >>>> From: Chuck Lever <chuck.lever@oracle.com> >>>> >>>> RFC 8881 Section 18.25.4 paragraph 5 tells us that the server >>>> should return NFS4ERR_FILE_OPEN only if the target object is an >>>> opened file. This suggests that returning this status when removing >>>> a directory will confuse NFS clients. >>>> >>>> This is a version-specific issue; nfsd_proc_remove/rmdir() and >>>> nfsd3_proc_remove/rmdir() already return nfserr_access as >>>> appropriate. >>>> >>>> Unfortunately there is no quick way for nfsd4_remove() to determine >>>> whether the target object is a file or not, so the check is done in >>>> to nfsd_unlink() for now. >>>> >>>> Reported-by: Trond Myklebust <trondmy@hammerspace.com> >>>> Fixes: 466e16f0920f ("nfsd: check for EBUSY from vfs_rmdir/vfs_unink.") >>>> Signed-off-by: Chuck Lever <chuck.lever@oracle.com> >>>> --- >>>> fs/nfsd/vfs.c | 24 ++++++++++++++++++------ >>>> 1 file changed, 18 insertions(+), 6 deletions(-) >>>> >>>> diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c >>>> index 2d8e27c225f9..3ead7fb3bf04 100644 >>>> --- a/fs/nfsd/vfs.c >>>> +++ b/fs/nfsd/vfs.c >>>> @@ -1931,9 +1931,17 @@ nfsd_rename(struct svc_rqst *rqstp, struct svc_fh *ffhp, char *fname, int flen, >>>> return err; >>>> } >>>> >>>> -/* >>>> - * Unlink a file or directory >>>> - * N.B. After this call fhp needs an fh_put >>>> +/** >>>> + * nfsd_unlink - remove a directory entry >>>> + * @rqstp: RPC transaction context >>>> + * @fhp: the file handle of the parent directory to be modified >>>> + * @type: enforced file type of the object to be removed >>>> + * @fname: the name of directory entry to be removed >>>> + * @flen: length of @fname in octets >>>> + * >>>> + * After this call fhp needs an fh_put. >>>> + * >>>> + * Returns a generic NFS status code in network byte-order. >>>> */ >>>> __be32 >>>> nfsd_unlink(struct svc_rqst *rqstp, struct svc_fh *fhp, int type, >>>> @@ -2007,10 +2015,14 @@ nfsd_unlink(struct svc_rqst *rqstp, struct svc_fh *fhp, int type, >>>> fh_drop_write(fhp); >>>> out_nfserr: >>>> if (host_err == -EBUSY) { >>>> - /* name is mounted-on. There is no perfect >>>> - * error status. >>>> + /* >>>> + * See RFC 8881 Section 18.25.4 para 4: NFSv4 REMOVE >>>> + * distinguishes between reg file and dir. >>>> */ >>>> - err = nfserr_file_open; >>>> + if (type != S_IFDIR) >>> >>> Should that be "if (type == S_ISREG)" instead? What if the inode is a >>> named pipe or device file? I'm not sure you can ever get EBUSY with >>> those, but in case you can, what's the right error in those cases? Another way to put this: If a client can acquire an OPEN state ID for those types of objects, then NFS4ERR_FILE_OPEN is the correct status code in those cases. But in practice, it depends on what existing client implementations expect. >> Check out nfsd_unlink()'s callers to see what they pass as the type >> parameter. Unfortunately we have to compare against S_IFDIR here. >> > > Not exactly. nfsd4_remove() is the only caller that needs to get > nfserr_file_open and this caller calls with type = 0, so type here > is going to be the actual type of the inode and (type == S_ISREG) > would be correct. No?
diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c index 2d8e27c225f9..3ead7fb3bf04 100644 --- a/fs/nfsd/vfs.c +++ b/fs/nfsd/vfs.c @@ -1931,9 +1931,17 @@ nfsd_rename(struct svc_rqst *rqstp, struct svc_fh *ffhp, char *fname, int flen, return err; } -/* - * Unlink a file or directory - * N.B. After this call fhp needs an fh_put +/** + * nfsd_unlink - remove a directory entry + * @rqstp: RPC transaction context + * @fhp: the file handle of the parent directory to be modified + * @type: enforced file type of the object to be removed + * @fname: the name of directory entry to be removed + * @flen: length of @fname in octets + * + * After this call fhp needs an fh_put. + * + * Returns a generic NFS status code in network byte-order. */ __be32 nfsd_unlink(struct svc_rqst *rqstp, struct svc_fh *fhp, int type, @@ -2007,10 +2015,14 @@ nfsd_unlink(struct svc_rqst *rqstp, struct svc_fh *fhp, int type, fh_drop_write(fhp); out_nfserr: if (host_err == -EBUSY) { - /* name is mounted-on. There is no perfect - * error status. + /* + * See RFC 8881 Section 18.25.4 para 4: NFSv4 REMOVE + * distinguishes between reg file and dir. */ - err = nfserr_file_open; + if (type != S_IFDIR) + err = nfserr_file_open; + else + err = nfserr_acces; } out: return err != nfs_ok ? err : nfserrno(host_err);