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

[Chuck Lever <chuck.lever@oracle.com>]

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