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?
I would drop this patch if I were you, but as you wish.
Thanks,
Amir.
