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

Chuck Lever
2025-01-24T14:04:30Z

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