| Message ID | 20250123195242.1378601-5-cel@kernel.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Chuck Lever |
| Headers | show |
| Series | Avoid returning NFS4ERR_FILE_OPEN when not appropriate | expand |
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);
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.
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?
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.
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);