[RFC PATCH 0/4] Avoid returning NFS4ERR_FILE_OPEN when not appropriate

[RFC PATCH 0/4] Avoid returning NFS4ERR_FILE_OPEN when not appropriate

[cel]

From: Chuck Lever <chuck.lever@oracle.com>

This short series aims to prevent NFSD from returning
NFS4ERR_FILE_OPEN when an NFSv4 LINK, RENAME, or REMOVE operation
targets a directory.  The only time the protocol spec permits a
server to return FILE_OPEN is when the target of the operation is a
file that is open and cannot be closed immediately to satisfy the
request.

I would have preferred these fixes go into NFSv4-specific sections
of NFSD, but the current structure of the code prevents doing that
while maintaining operational efficiency. Plus, these small patches
should be able to apply cleanly to LTS kernels.

We can defer deeper restructuring for later. For example,
fh_verify() could be made to return an errno instead of a generic
NFS status code; then the VFS utility functions in fs/nfsd/vfs.c
could be made to do the same, making their callers responsible for
the proper NFS version-specific translation of the errno into a
status code.

This series has been only compile tested. I'm posting early for
review and comment about this approach, but please do test these if
you have the ability to trigger -EBUSY easily.

Amir notes that NFSv4 OPEN is also affected. nfsd4_open() is a
pretty deep stack of code. If there are stack traces available, we
should be able to see where to start digging for errno leaks in
that operation.

Chuck Lever (4):
  NFSD: nfsd_unlink() clobbers non-zero status returned from
    fh_fill_pre_attrs()
  NFSD: Never return NFS4ERR_FILE_OPEN when removing a directory
  NFSD: Return NFS4ERR_FILE_OPEN only when renaming over an open file
  NFSD: Return NFS4ERR_FILE_OPEN only when linking an open file

 fs/nfsd/vfs.c | 102 +++++++++++++++++++++++++++++++++++++-------------
 1 file changed, 76 insertions(+), 26 deletions(-)

-- 
2.47.0

[RFC PATCH 1/4] NFSD: nfsd_unlink() clobbers non-zero status returned from fh_fill_pre_attrs()

[cel]

From: Chuck Lever <chuck.lever@oracle.com>

If fh_fill_pre_attrs() returns a non-zero status, the error flow
takes it through out_unlock, which then overwrites the returned
status code with

	err = nfserrno(host_err);

Fixes: a332018a91c4 ("nfsd: handle failure to collect pre/post-op attrs more sanely")
Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---
 fs/nfsd/vfs.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
index 29cb7b812d71..2d8e27c225f9 100644
--- a/fs/nfsd/vfs.c
+++ b/fs/nfsd/vfs.c
@@ -2011,11 +2011,9 @@ nfsd_unlink(struct svc_rqst *rqstp, struct svc_fh *fhp, int type,
 		 * error status.
 		 */
 		err = nfserr_file_open;
-	} else {
-		err = nfserrno(host_err);
 	}
 out:
-	return err;
+	return err != nfs_ok ? err : nfserrno(host_err);
 out_unlock:
 	inode_unlock(dirp);
 	goto out_drop_write;
-- 
2.47.0

[RFC PATCH 2/4] NFSD: Never return NFS4ERR_FILE_OPEN when removing a directory

[cel]

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)
+			err = nfserr_file_open;
+		else
+			err = nfserr_acces;
 	}
 out:
 	return err != nfs_ok ? err : nfserrno(host_err);
-- 
2.47.0

[RFC PATCH 3/4] NFSD: Return NFS4ERR_FILE_OPEN only when renaming over an open file

[cel]

From: Chuck Lever <chuck.lever@oracle.com>

RFC 8881 Section 18.26.4 paragraphs 1 - 3 tell us that RENAME should
return NFS4ERR_FILE_OPEN only when the target object is a file that
is currently open. If the target is a directory, some other status
must be returned.

Generally I expect that a delegation recall will be triggered in
some of these circumstances. In other cases, the VFS might return
-EBUSY for other reasons, and NFSD has to ensure that errno does
not leak to clients as a status code that is not permitted by spec.

There are some error flows where the target dentry hasn't been
found yet. The default value for @type therefore is S_IFDIR to return
an alternate status code in those cases.

Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---
 fs/nfsd/vfs.c | 30 ++++++++++++++++++++++++++----
 1 file changed, 26 insertions(+), 4 deletions(-)

diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
index 3ead7fb3bf04..5cfb5eb54c23 100644
--- a/fs/nfsd/vfs.c
+++ b/fs/nfsd/vfs.c
@@ -1795,9 +1795,19 @@ nfsd_has_cached_files(struct dentry *dentry)
 	return ret;
 }
 
-/*
- * Rename a file
- * N.B. After this call _both_ ffhp and tfhp need an fh_put
+/**
+ * nfsd_rename - rename a directory entry
+ * @rqstp: RPC transaction context
+ * @ffhp: the file handle of parent directory containing the entry to be renamed
+ * @fname: the filename of directory entry to be renamed
+ * @flen: the length of @fname in octets
+ * @tfhp: the file handle of parent directory to contain the renamed entry
+ * @tname: the filename of the new entry
+ * @tlen: the length of @tlen in octets
+ *
+ * After this call _both_ ffhp and tfhp need an fh_put.
+ *
+ * Returns a generic NFS status code in network byte-order.
  */
 __be32
 nfsd_rename(struct svc_rqst *rqstp, struct svc_fh *ffhp, char *fname, int flen,
@@ -1805,6 +1815,7 @@ nfsd_rename(struct svc_rqst *rqstp, struct svc_fh *ffhp, char *fname, int flen,
 {
 	struct dentry	*fdentry, *tdentry, *odentry, *ndentry, *trap;
 	struct inode	*fdir, *tdir;
+	int		type = S_IFDIR;
 	__be32		err;
 	int		host_err;
 	bool		close_cached = false;
@@ -1867,6 +1878,7 @@ nfsd_rename(struct svc_rqst *rqstp, struct svc_fh *ffhp, char *fname, int flen,
 	host_err = PTR_ERR(ndentry);
 	if (IS_ERR(ndentry))
 		goto out_dput_old;
+	type = d_inode(ndentry)->i_mode & S_IFMT;
 	host_err = -ENOTEMPTY;
 	if (ndentry == trap)
 		goto out_dput_new;
@@ -1904,7 +1916,17 @@ nfsd_rename(struct svc_rqst *rqstp, struct svc_fh *ffhp, char *fname, int flen,
  out_dput_old:
 	dput(odentry);
  out_nfserr:
-	err = nfserrno(host_err);
+	if (host_err == -EBUSY) {
+		/*
+		 * See RFC 8881 Section 18.26.4 para 1-3: NFSv4 RENAME
+		 * status distinguishes between reg file and dir.
+		 */
+		if (type != S_IFDIR)
+			err = nfserr_file_open;
+		else
+			err = nfserr_acces;
+	} else
+		err = nfserrno(host_err);
 
 	if (!close_cached) {
 		fh_fill_post_attrs(ffhp);
-- 
2.47.0

[RFC PATCH 4/4] NFSD: Return NFS4ERR_FILE_OPEN only when linking an open file

[cel]

From: Chuck Lever <chuck.lever@oracle.com>

RFC 8881 Section 18.9.4 paragraphs 1 - 2 tell us that RENAME should
return NFS4ERR_FILE_OPEN only when the target object is a file that
is currently open. If the target is a directory, some other status
must be returned.

Generally I expect that a delegation recall will be triggered in
some of these circumstances. In other cases, the VFS might return
-EBUSY for other reasons, and NFSD has to ensure that errno does
not leak to clients as a status code that is not permitted by spec.

Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---
 fs/nfsd/vfs.c | 44 +++++++++++++++++++++++++++++++-------------
 1 file changed, 31 insertions(+), 13 deletions(-)

diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
index 5cfb5eb54c23..566b9adf2259 100644
--- a/fs/nfsd/vfs.c
+++ b/fs/nfsd/vfs.c
@@ -1699,9 +1699,17 @@ nfsd_symlink(struct svc_rqst *rqstp, struct svc_fh *fhp,
 	return err;
 }
 
-/*
- * Create a hardlink
- * N.B. After this call _both_ ffhp and tfhp need an fh_put
+/**
+ * nfsd_link - create a link
+ * @rqstp: RPC transaction context
+ * @ffhp: the file handle of the directory where the new link is to be created
+ * @name: the filename of the new link
+ * @len: the length of @name in octets
+ * @tfhp: the file handle of an existing file object
+ *
+ * After this call _both_ ffhp and tfhp need an fh_put.
+ *
+ * Returns a generic NFS status code in network byte-order.
  */
 __be32
 nfsd_link(struct svc_rqst *rqstp, struct svc_fh *ffhp,
@@ -1709,6 +1717,7 @@ nfsd_link(struct svc_rqst *rqstp, struct svc_fh *ffhp,
 {
 	struct dentry	*ddir, *dnew, *dold;
 	struct inode	*dirp;
+	int		type;
 	__be32		err;
 	int		host_err;
 
@@ -1728,11 +1737,11 @@ nfsd_link(struct svc_rqst *rqstp, struct svc_fh *ffhp,
 	if (isdotent(name, len))
 		goto out;
 
+	err = nfs_ok;
+	type = d_inode(tfhp->fh_dentry)->i_mode & S_IFMT;
 	host_err = fh_want_write(tfhp);
-	if (host_err) {
-		err = nfserrno(host_err);
+	if (host_err)
 		goto out;
-	}
 
 	ddir = ffhp->fh_dentry;
 	dirp = d_inode(ddir);
@@ -1740,7 +1749,7 @@ nfsd_link(struct svc_rqst *rqstp, struct svc_fh *ffhp,
 
 	dnew = lookup_one_len(name, ddir, len);
 	if (IS_ERR(dnew)) {
-		err = nfserrno(PTR_ERR(dnew));
+		host_err = PTR_ERR(dnew);
 		goto out_unlock;
 	}
 
@@ -1756,17 +1765,26 @@ nfsd_link(struct svc_rqst *rqstp, struct svc_fh *ffhp,
 	fh_fill_post_attrs(ffhp);
 	inode_unlock(dirp);
 	if (!host_err) {
-		err = nfserrno(commit_metadata(ffhp));
-		if (!err)
-			err = nfserrno(commit_metadata(tfhp));
-	} else {
-		err = nfserrno(host_err);
+		host_err = commit_metadata(ffhp);
+		if (!host_err)
+			host_err = commit_metadata(tfhp);
 	}
+
 	dput(dnew);
 out_drop_write:
 	fh_drop_write(tfhp);
+	if (host_err == -EBUSY) {
+		/*
+		 * See RFC 8881 Section 18.9.4 para 1-2: NFSv4 LINK
+		 * status distinguishes between reg file and dir.
+		 */
+		if (type != S_IFDIR)
+			err = nfserr_file_open;
+		else
+			err = nfserr_acces;
+	}
 out:
-	return err;
+	return err != nfs_ok ? err : nfserrno(host_err);
 
 out_dput:
 	dput(dnew);
-- 
2.47.0

Re: [RFC PATCH 2/4] NFSD: Never return NFS4ERR_FILE_OPEN when removing a directory

[Jeff Layton]

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);

-- 
Jeff Layton <jlayton@kernel.org>

Re: [RFC PATCH 4/4] NFSD: Return NFS4ERR_FILE_OPEN only when linking an open file

[Jeff Layton]

On Thu, 2025-01-23 at 14:52 -0500, cel@kernel.org wrote:
> From: Chuck Lever <chuck.lever@oracle.com>
> 
> RFC 8881 Section 18.9.4 paragraphs 1 - 2 tell us that RENAME should
> return NFS4ERR_FILE_OPEN only when the target object is a file that
> is currently open. If the target is a directory, some other status
> must be returned.
> 
> Generally I expect that a delegation recall will be triggered in
> some of these circumstances. In other cases, the VFS might return
> -EBUSY for other reasons, and NFSD has to ensure that errno does
> not leak to clients as a status code that is not permitted by spec.
> 
> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> ---
>  fs/nfsd/vfs.c | 44 +++++++++++++++++++++++++++++++-------------
>  1 file changed, 31 insertions(+), 13 deletions(-)
> 
> diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
> index 5cfb5eb54c23..566b9adf2259 100644
> --- a/fs/nfsd/vfs.c
> +++ b/fs/nfsd/vfs.c
> @@ -1699,9 +1699,17 @@ nfsd_symlink(struct svc_rqst *rqstp, struct svc_fh *fhp,
>  	return err;
>  }
>  
> -/*
> - * Create a hardlink
> - * N.B. After this call _both_ ffhp and tfhp need an fh_put
> +/**
> + * nfsd_link - create a link
> + * @rqstp: RPC transaction context
> + * @ffhp: the file handle of the directory where the new link is to be created
> + * @name: the filename of the new link
> + * @len: the length of @name in octets
> + * @tfhp: the file handle of an existing file object
> + *
> + * After this call _both_ ffhp and tfhp need an fh_put.
> + *
> + * Returns a generic NFS status code in network byte-order.
>   */
>  __be32
>  nfsd_link(struct svc_rqst *rqstp, struct svc_fh *ffhp,
> @@ -1709,6 +1717,7 @@ nfsd_link(struct svc_rqst *rqstp, struct svc_fh *ffhp,
>  {
>  	struct dentry	*ddir, *dnew, *dold;
>  	struct inode	*dirp;
> +	int		type;
>  	__be32		err;
>  	int		host_err;
>  
> @@ -1728,11 +1737,11 @@ nfsd_link(struct svc_rqst *rqstp, struct svc_fh *ffhp,
>  	if (isdotent(name, len))
>  		goto out;
>  
> +	err = nfs_ok;
> +	type = d_inode(tfhp->fh_dentry)->i_mode & S_IFMT;
>  	host_err = fh_want_write(tfhp);
> -	if (host_err) {
> -		err = nfserrno(host_err);
> +	if (host_err)
>  		goto out;
> -	}
>  
>  	ddir = ffhp->fh_dentry;
>  	dirp = d_inode(ddir);
> @@ -1740,7 +1749,7 @@ nfsd_link(struct svc_rqst *rqstp, struct svc_fh *ffhp,
>  
>  	dnew = lookup_one_len(name, ddir, len);
>  	if (IS_ERR(dnew)) {
> -		err = nfserrno(PTR_ERR(dnew));
> +		host_err = PTR_ERR(dnew);
>  		goto out_unlock;
>  	}
>  
> @@ -1756,17 +1765,26 @@ nfsd_link(struct svc_rqst *rqstp, struct svc_fh *ffhp,
>  	fh_fill_post_attrs(ffhp);
>  	inode_unlock(dirp);
>  	if (!host_err) {
> -		err = nfserrno(commit_metadata(ffhp));
> -		if (!err)
> -			err = nfserrno(commit_metadata(tfhp));
> -	} else {
> -		err = nfserrno(host_err);
> +		host_err = commit_metadata(ffhp);
> +		if (!host_err)
> +			host_err = commit_metadata(tfhp);
>  	}
> +
>  	dput(dnew);
>  out_drop_write:
>  	fh_drop_write(tfhp);
> +	if (host_err == -EBUSY) {
> +		/*
> +		 * See RFC 8881 Section 18.9.4 para 1-2: NFSv4 LINK
> +		 * status distinguishes between reg file and dir.
> +		 */
> +		if (type != S_IFDIR)
> +			err = nfserr_file_open;
> +		else
> +			err = nfserr_acces;

I guess nothing in NFS protocol spec prohibits you from hardlinking a
directory, but hopefully any Linux filesystem will be returning -EPERM
when someone tries it! IOW, I suspect the above will probably be dead
code, but I don't think it'll hurt anything.

> +	}
>  out:
> -	return err;
> +	return err != nfs_ok ? err : nfserrno(host_err);
>  
>  out_dput:
>  	dput(dnew);

-- 
Jeff Layton <jlayton@kernel.org>

Re: [RFC PATCH 2/4] NFSD: Never return NFS4ERR_FILE_OPEN when removing a directory

[Chuck Lever]

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);
> 


-- 
Chuck Lever

Re: [RFC PATCH 2/4] NFSD: Never return NFS4ERR_FILE_OPEN when removing a directory

[Amir Goldstein]

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.

Re: [RFC PATCH 3/4] NFSD: Return NFS4ERR_FILE_OPEN only when renaming over an open file

[Amir Goldstein]

On Thu, Jan 23, 2025 at 8:52 PM <cel@kernel.org> wrote:
>
> From: Chuck Lever <chuck.lever@oracle.com>
>
> RFC 8881 Section 18.26.4 paragraphs 1 - 3 tell us that RENAME should
> return NFS4ERR_FILE_OPEN only when the target object is a file that
> is currently open. If the target is a directory, some other status
> must be returned.
>
> Generally I expect that a delegation recall will be triggered in
> some of these circumstances. In other cases, the VFS might return
> -EBUSY for other reasons, and NFSD has to ensure that errno does
> not leak to clients as a status code that is not permitted by spec.
>
> There are some error flows where the target dentry hasn't been
> found yet. The default value for @type therefore is S_IFDIR to return
> an alternate status code in those cases.
>
> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> ---
>  fs/nfsd/vfs.c | 30 ++++++++++++++++++++++++++----
>  1 file changed, 26 insertions(+), 4 deletions(-)
>
> diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
> index 3ead7fb3bf04..5cfb5eb54c23 100644
> --- a/fs/nfsd/vfs.c
> +++ b/fs/nfsd/vfs.c
> @@ -1795,9 +1795,19 @@ nfsd_has_cached_files(struct dentry *dentry)
>         return ret;
>  }
>
> -/*
> - * Rename a file
> - * N.B. After this call _both_ ffhp and tfhp need an fh_put
> +/**
> + * nfsd_rename - rename a directory entry
> + * @rqstp: RPC transaction context
> + * @ffhp: the file handle of parent directory containing the entry to be renamed
> + * @fname: the filename of directory entry to be renamed
> + * @flen: the length of @fname in octets
> + * @tfhp: the file handle of parent directory to contain the renamed entry
> + * @tname: the filename of the new entry
> + * @tlen: the length of @tlen in octets
> + *
> + * After this call _both_ ffhp and tfhp need an fh_put.
> + *
> + * Returns a generic NFS status code in network byte-order.
>   */
>  __be32
>  nfsd_rename(struct svc_rqst *rqstp, struct svc_fh *ffhp, char *fname, int flen,
> @@ -1805,6 +1815,7 @@ nfsd_rename(struct svc_rqst *rqstp, struct svc_fh *ffhp, char *fname, int flen,
>  {
>         struct dentry   *fdentry, *tdentry, *odentry, *ndentry, *trap;
>         struct inode    *fdir, *tdir;
> +       int             type = S_IFDIR;
>         __be32          err;
>         int             host_err;
>         bool            close_cached = false;
> @@ -1867,6 +1878,7 @@ nfsd_rename(struct svc_rqst *rqstp, struct svc_fh *ffhp, char *fname, int flen,
>         host_err = PTR_ERR(ndentry);
>         if (IS_ERR(ndentry))
>                 goto out_dput_old;
> +       type = d_inode(ndentry)->i_mode & S_IFMT;
>         host_err = -ENOTEMPTY;
>         if (ndentry == trap)
>                 goto out_dput_new;
> @@ -1904,7 +1916,17 @@ nfsd_rename(struct svc_rqst *rqstp, struct svc_fh *ffhp, char *fname, int flen,
>   out_dput_old:
>         dput(odentry);
>   out_nfserr:
> -       err = nfserrno(host_err);
> +       if (host_err == -EBUSY) {
> +               /*
> +                * See RFC 8881 Section 18.26.4 para 1-3: NFSv4 RENAME
> +                * status distinguishes between reg file and dir.
> +                */
> +               if (type != S_IFDIR)

Maybe (type == S_IFREG) here as well?

> +                       err = nfserr_file_open;
> +               else
> +                       err = nfserr_acces;
> +       } else
> +               err = nfserrno(host_err);
>

if {} else ; is a code smell with high potential for human
misreading of code.

Thanks,
Amir.

Re: [RFC PATCH 4/4] NFSD: Return NFS4ERR_FILE_OPEN only when linking an open file

[Amir Goldstein]

On Thu, Jan 23, 2025 at 9:54 PM Jeff Layton <jlayton@kernel.org> 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.9.4 paragraphs 1 - 2 tell us that RENAME should
> > return NFS4ERR_FILE_OPEN only when the target object is a file that
> > is currently open. If the target is a directory, some other status
> > must be returned.
> >
> > Generally I expect that a delegation recall will be triggered in
> > some of these circumstances. In other cases, the VFS might return
> > -EBUSY for other reasons, and NFSD has to ensure that errno does
> > not leak to clients as a status code that is not permitted by spec.
> >
> > Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> > ---
> >  fs/nfsd/vfs.c | 44 +++++++++++++++++++++++++++++++-------------
> >  1 file changed, 31 insertions(+), 13 deletions(-)
> >
> > diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
> > index 5cfb5eb54c23..566b9adf2259 100644
> > --- a/fs/nfsd/vfs.c
> > +++ b/fs/nfsd/vfs.c
> > @@ -1699,9 +1699,17 @@ nfsd_symlink(struct svc_rqst *rqstp, struct svc_fh *fhp,
> >       return err;
> >  }
> >
> > -/*
> > - * Create a hardlink
> > - * N.B. After this call _both_ ffhp and tfhp need an fh_put
> > +/**
> > + * nfsd_link - create a link
> > + * @rqstp: RPC transaction context
> > + * @ffhp: the file handle of the directory where the new link is to be created
> > + * @name: the filename of the new link
> > + * @len: the length of @name in octets
> > + * @tfhp: the file handle of an existing file object
> > + *
> > + * After this call _both_ ffhp and tfhp need an fh_put.
> > + *
> > + * Returns a generic NFS status code in network byte-order.
> >   */
> >  __be32
> >  nfsd_link(struct svc_rqst *rqstp, struct svc_fh *ffhp,
> > @@ -1709,6 +1717,7 @@ nfsd_link(struct svc_rqst *rqstp, struct svc_fh *ffhp,
> >  {
> >       struct dentry   *ddir, *dnew, *dold;
> >       struct inode    *dirp;
> > +     int             type;
> >       __be32          err;
> >       int             host_err;
> >
> > @@ -1728,11 +1737,11 @@ nfsd_link(struct svc_rqst *rqstp, struct svc_fh *ffhp,
> >       if (isdotent(name, len))
> >               goto out;
> >
> > +     err = nfs_ok;
> > +     type = d_inode(tfhp->fh_dentry)->i_mode & S_IFMT;
> >       host_err = fh_want_write(tfhp);
> > -     if (host_err) {
> > -             err = nfserrno(host_err);
> > +     if (host_err)
> >               goto out;
> > -     }
> >
> >       ddir = ffhp->fh_dentry;
> >       dirp = d_inode(ddir);
> > @@ -1740,7 +1749,7 @@ nfsd_link(struct svc_rqst *rqstp, struct svc_fh *ffhp,
> >
> >       dnew = lookup_one_len(name, ddir, len);
> >       if (IS_ERR(dnew)) {
> > -             err = nfserrno(PTR_ERR(dnew));
> > +             host_err = PTR_ERR(dnew);
> >               goto out_unlock;
> >       }
> >
> > @@ -1756,17 +1765,26 @@ nfsd_link(struct svc_rqst *rqstp, struct svc_fh *ffhp,
> >       fh_fill_post_attrs(ffhp);
> >       inode_unlock(dirp);
> >       if (!host_err) {
> > -             err = nfserrno(commit_metadata(ffhp));
> > -             if (!err)
> > -                     err = nfserrno(commit_metadata(tfhp));
> > -     } else {
> > -             err = nfserrno(host_err);
> > +             host_err = commit_metadata(ffhp);
> > +             if (!host_err)
> > +                     host_err = commit_metadata(tfhp);
> >       }
> > +
> >       dput(dnew);
> >  out_drop_write:
> >       fh_drop_write(tfhp);
> > +     if (host_err == -EBUSY) {
> > +             /*
> > +              * See RFC 8881 Section 18.9.4 para 1-2: NFSv4 LINK
> > +              * status distinguishes between reg file and dir.
> > +              */
> > +             if (type != S_IFDIR)
> > +                     err = nfserr_file_open;
> > +             else
> > +                     err = nfserr_acces;
>
> I guess nothing in NFS protocol spec prohibits you from hardlinking a
> directory, but hopefully any Linux filesystem will be returning -EPERM
> when someone tries it! IOW, I suspect the above will probably be dead
> code, but I don't think it'll hurt anything.
>

Not to mention that unlike rmdir() and rename(), vfs does not return EBUSY
for link(), so this code is not really testable as is, is it?

I would drop this patch if I were you, but as you wish.

Thanks,
Amir.

Re: [RFC PATCH 4/4] NFSD: Return NFS4ERR_FILE_OPEN only when linking an open file

[Chuck Lever]

On 1/24/25 6:22 AM, Amir Goldstein wrote:
> On Thu, Jan 23, 2025 at 9:54 PM Jeff Layton <jlayton@kernel.org> 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.9.4 paragraphs 1 - 2 tell us that RENAME should
>>> return NFS4ERR_FILE_OPEN only when the target object is a file that
>>> is currently open. If the target is a directory, some other status
>>> must be returned.
>>>
>>> Generally I expect that a delegation recall will be triggered in
>>> some of these circumstances. In other cases, the VFS might return
>>> -EBUSY for other reasons, and NFSD has to ensure that errno does
>>> not leak to clients as a status code that is not permitted by spec.
>>>
>>> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
>>> ---
>>>   fs/nfsd/vfs.c | 44 +++++++++++++++++++++++++++++++-------------
>>>   1 file changed, 31 insertions(+), 13 deletions(-)
>>>
>>> diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
>>> index 5cfb5eb54c23..566b9adf2259 100644
>>> --- a/fs/nfsd/vfs.c
>>> +++ b/fs/nfsd/vfs.c
>>> @@ -1699,9 +1699,17 @@ nfsd_symlink(struct svc_rqst *rqstp, struct svc_fh *fhp,
>>>        return err;
>>>   }
>>>
>>> -/*
>>> - * Create a hardlink
>>> - * N.B. After this call _both_ ffhp and tfhp need an fh_put
>>> +/**
>>> + * nfsd_link - create a link
>>> + * @rqstp: RPC transaction context
>>> + * @ffhp: the file handle of the directory where the new link is to be created
>>> + * @name: the filename of the new link
>>> + * @len: the length of @name in octets
>>> + * @tfhp: the file handle of an existing file object
>>> + *
>>> + * After this call _both_ ffhp and tfhp need an fh_put.
>>> + *
>>> + * Returns a generic NFS status code in network byte-order.
>>>    */
>>>   __be32
>>>   nfsd_link(struct svc_rqst *rqstp, struct svc_fh *ffhp,
>>> @@ -1709,6 +1717,7 @@ nfsd_link(struct svc_rqst *rqstp, struct svc_fh *ffhp,
>>>   {
>>>        struct dentry   *ddir, *dnew, *dold;
>>>        struct inode    *dirp;
>>> +     int             type;
>>>        __be32          err;
>>>        int             host_err;
>>>
>>> @@ -1728,11 +1737,11 @@ nfsd_link(struct svc_rqst *rqstp, struct svc_fh *ffhp,
>>>        if (isdotent(name, len))
>>>                goto out;
>>>
>>> +     err = nfs_ok;
>>> +     type = d_inode(tfhp->fh_dentry)->i_mode & S_IFMT;
>>>        host_err = fh_want_write(tfhp);
>>> -     if (host_err) {
>>> -             err = nfserrno(host_err);
>>> +     if (host_err)
>>>                goto out;
>>> -     }
>>>
>>>        ddir = ffhp->fh_dentry;
>>>        dirp = d_inode(ddir);
>>> @@ -1740,7 +1749,7 @@ nfsd_link(struct svc_rqst *rqstp, struct svc_fh *ffhp,
>>>
>>>        dnew = lookup_one_len(name, ddir, len);
>>>        if (IS_ERR(dnew)) {
>>> -             err = nfserrno(PTR_ERR(dnew));
>>> +             host_err = PTR_ERR(dnew);
>>>                goto out_unlock;
>>>        }
>>>
>>> @@ -1756,17 +1765,26 @@ nfsd_link(struct svc_rqst *rqstp, struct svc_fh *ffhp,
>>>        fh_fill_post_attrs(ffhp);
>>>        inode_unlock(dirp);
>>>        if (!host_err) {
>>> -             err = nfserrno(commit_metadata(ffhp));
>>> -             if (!err)
>>> -                     err = nfserrno(commit_metadata(tfhp));
>>> -     } else {
>>> -             err = nfserrno(host_err);
>>> +             host_err = commit_metadata(ffhp);
>>> +             if (!host_err)
>>> +                     host_err = commit_metadata(tfhp);
>>>        }
>>> +
>>>        dput(dnew);
>>>   out_drop_write:
>>>        fh_drop_write(tfhp);
>>> +     if (host_err == -EBUSY) {
>>> +             /*
>>> +              * See RFC 8881 Section 18.9.4 para 1-2: NFSv4 LINK
>>> +              * status distinguishes between reg file and dir.
>>> +              */
>>> +             if (type != S_IFDIR)
>>> +                     err = nfserr_file_open;
>>> +             else
>>> +                     err = nfserr_acces;
>>
>> I guess nothing in NFS protocol spec prohibits you from hardlinking a
>> directory, but hopefully any Linux filesystem will be returning -EPERM
>> when someone tries it! IOW, I suspect the above will probably be dead
>> code, but I don't think it'll hurt anything.
>>
> 
> Not to mention that unlike rmdir() and rename(), vfs does not return EBUSY
> for link(), so this code is not really testable as is, is it?

You suggested that the VFS could return -EBUSY for just about anything
for FuSE.


> I would drop this patch if I were you, but as you wish.

I can, but how do we know we'll never get -EBUSY here?


-- 
Chuck Lever

Re: [RFC PATCH 2/4] NFSD: Never return NFS4ERR_FILE_OPEN when removing a directory

[Chuck Lever]

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?


-- 
Chuck Lever

Re: [RFC PATCH 4/4] NFSD: Return NFS4ERR_FILE_OPEN only when linking an open file

[Amir Goldstein]

On Fri, Jan 24, 2025 at 3:04 PM Chuck Lever <chuck.lever@oracle.com> wrote:
>
> On 1/24/25 6:22 AM, Amir Goldstein wrote:
> > On Thu, Jan 23, 2025 at 9:54 PM Jeff Layton <jlayton@kernel.org> 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.9.4 paragraphs 1 - 2 tell us that RENAME should
> >>> return NFS4ERR_FILE_OPEN only when the target object is a file that
> >>> is currently open. If the target is a directory, some other status
> >>> must be returned.
> >>>
> >>> Generally I expect that a delegation recall will be triggered in
> >>> some of these circumstances. In other cases, the VFS might return
> >>> -EBUSY for other reasons, and NFSD has to ensure that errno does
> >>> not leak to clients as a status code that is not permitted by spec.
> >>>
> >>> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> >>> ---
> >>>   fs/nfsd/vfs.c | 44 +++++++++++++++++++++++++++++++-------------
> >>>   1 file changed, 31 insertions(+), 13 deletions(-)
> >>>
> >>> diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
> >>> index 5cfb5eb54c23..566b9adf2259 100644
> >>> --- a/fs/nfsd/vfs.c
> >>> +++ b/fs/nfsd/vfs.c
> >>> @@ -1699,9 +1699,17 @@ nfsd_symlink(struct svc_rqst *rqstp, struct svc_fh *fhp,
> >>>        return err;
> >>>   }
> >>>
> >>> -/*
> >>> - * Create a hardlink
> >>> - * N.B. After this call _both_ ffhp and tfhp need an fh_put
> >>> +/**
> >>> + * nfsd_link - create a link
> >>> + * @rqstp: RPC transaction context
> >>> + * @ffhp: the file handle of the directory where the new link is to be created
> >>> + * @name: the filename of the new link
> >>> + * @len: the length of @name in octets
> >>> + * @tfhp: the file handle of an existing file object
> >>> + *
> >>> + * After this call _both_ ffhp and tfhp need an fh_put.
> >>> + *
> >>> + * Returns a generic NFS status code in network byte-order.
> >>>    */
> >>>   __be32
> >>>   nfsd_link(struct svc_rqst *rqstp, struct svc_fh *ffhp,
> >>> @@ -1709,6 +1717,7 @@ nfsd_link(struct svc_rqst *rqstp, struct svc_fh *ffhp,
> >>>   {
> >>>        struct dentry   *ddir, *dnew, *dold;
> >>>        struct inode    *dirp;
> >>> +     int             type;
> >>>        __be32          err;
> >>>        int             host_err;
> >>>
> >>> @@ -1728,11 +1737,11 @@ nfsd_link(struct svc_rqst *rqstp, struct svc_fh *ffhp,
> >>>        if (isdotent(name, len))
> >>>                goto out;
> >>>
> >>> +     err = nfs_ok;
> >>> +     type = d_inode(tfhp->fh_dentry)->i_mode & S_IFMT;
> >>>        host_err = fh_want_write(tfhp);
> >>> -     if (host_err) {
> >>> -             err = nfserrno(host_err);
> >>> +     if (host_err)
> >>>                goto out;
> >>> -     }
> >>>
> >>>        ddir = ffhp->fh_dentry;
> >>>        dirp = d_inode(ddir);
> >>> @@ -1740,7 +1749,7 @@ nfsd_link(struct svc_rqst *rqstp, struct svc_fh *ffhp,
> >>>
> >>>        dnew = lookup_one_len(name, ddir, len);
> >>>        if (IS_ERR(dnew)) {
> >>> -             err = nfserrno(PTR_ERR(dnew));
> >>> +             host_err = PTR_ERR(dnew);
> >>>                goto out_unlock;
> >>>        }
> >>>
> >>> @@ -1756,17 +1765,26 @@ nfsd_link(struct svc_rqst *rqstp, struct svc_fh *ffhp,
> >>>        fh_fill_post_attrs(ffhp);
> >>>        inode_unlock(dirp);
> >>>        if (!host_err) {
> >>> -             err = nfserrno(commit_metadata(ffhp));
> >>> -             if (!err)
> >>> -                     err = nfserrno(commit_metadata(tfhp));
> >>> -     } else {
> >>> -             err = nfserrno(host_err);
> >>> +             host_err = commit_metadata(ffhp);
> >>> +             if (!host_err)
> >>> +                     host_err = commit_metadata(tfhp);
> >>>        }
> >>> +
> >>>        dput(dnew);
> >>>   out_drop_write:
> >>>        fh_drop_write(tfhp);
> >>> +     if (host_err == -EBUSY) {
> >>> +             /*
> >>> +              * See RFC 8881 Section 18.9.4 para 1-2: NFSv4 LINK
> >>> +              * status distinguishes between reg file and dir.
> >>> +              */
> >>> +             if (type != S_IFDIR)
> >>> +                     err = nfserr_file_open;
> >>> +             else
> >>> +                     err = nfserr_acces;
> >>
> >> I guess nothing in NFS protocol spec prohibits you from hardlinking a
> >> directory, but hopefully any Linux filesystem will be returning -EPERM
> >> when someone tries it! IOW, I suspect the above will probably be dead
> >> code, but I don't think it'll hurt anything.
> >>
> >
> > Not to mention that unlike rmdir() and rename(), vfs does not return EBUSY
> > for link(), so this code is not really testable as is, is it?
>
> You suggested that the VFS could return -EBUSY for just about anything
> for FuSE.
>

Yes, it is possible.

>
> > I would drop this patch if I were you, but as you wish.
>
> I can, but how do we know we'll never get -EBUSY here?
>

You are right.

Thanks,
Amir.