On Fri, Jan 24, 2025 at 3:04 PM Chuck Lever <chuck.lever@xxxxxxxxxx> wrote:
>
> On 1/24/25 6:22 AM, Amir Goldstein wrote:
> > On Thu, Jan 23, 2025 at 9:54 PM Jeff Layton <jlayton@xxxxxxxxxx> wrote:
> >>
> >> On Thu, 2025-01-23 at 14:52 -0500, cel@xxxxxxxxxx wrote:
> >>> From: Chuck Lever <chuck.lever@xxxxxxxxxx>
> >>>
> >>> 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@xxxxxxxxxx>
> >>> ---
> >>> 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.
