| Message ID | 20231015172927.GE800259@ZenIV (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | nfsd: lock_rename() needs both directories to live on the same fs | expand |
On Sun, Oct 15, 2023 at 06:29:27PM +0100, Al Viro wrote: > ... checking that after lock_rename() is too late. Incidentally, > NFSv2 had no nfserr_xdev... > > Fixes: aa387d6ce153 "nfsd: fix EXDEV checking in rename" > Cc: stable@vger.kernel.org # v3.9+ > Reviewed-by: Jeff Layton <jlayton@kernel.org> > Signed-off-by: Al Viro <viro@zeniv.linux.org.uk> > --- > > [in git://git.kernel.org/pub/scm/linux/kernel/git/viro/vfs.git #nfsd-fix; > it's an immutable branch, please either pull from it or put that thing > into an immutable branch in your tree - there's a lock_rename-related > series in the making and I'd rather avoid mixing unrelated nfsd stuff > into it ;-/] I'll assume you will be sending this on to Linus, then: Acked-by: Chuck Lever <chuck.lever@oracle.com> Jeff or I will get back to you in a day or two on the results of regression testing, but I don't expect a problem. > diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c > index 48260cf68fde..02f5fcaad03f 100644 > --- a/fs/nfsd/vfs.c > +++ b/fs/nfsd/vfs.c > @@ -1788,6 +1788,12 @@ nfsd_rename(struct svc_rqst *rqstp, struct svc_fh *ffhp, char *fname, int flen, > if (!flen || isdotent(fname, flen) || !tlen || isdotent(tname, tlen)) > goto out; > > + err = (rqstp->rq_vers == 2) ? nfserr_acces : nfserr_xdev; > + if (ffhp->fh_export->ex_path.mnt != tfhp->fh_export->ex_path.mnt) > + goto out; > + if (ffhp->fh_export->ex_path.dentry != tfhp->fh_export->ex_path.dentry) > + goto out; > + > retry: > host_err = fh_want_write(ffhp); > if (host_err) { > @@ -1823,12 +1829,6 @@ nfsd_rename(struct svc_rqst *rqstp, struct svc_fh *ffhp, char *fname, int flen, > if (ndentry == trap) > goto out_dput_new; > > - host_err = -EXDEV; > - if (ffhp->fh_export->ex_path.mnt != tfhp->fh_export->ex_path.mnt) > - goto out_dput_new; > - if (ffhp->fh_export->ex_path.dentry != tfhp->fh_export->ex_path.dentry) > - goto out_dput_new; > - > if ((ndentry->d_sb->s_export_op->flags & EXPORT_OP_CLOSE_BEFORE_UNLINK) && > nfsd_has_cached_files(ndentry)) { > close_cached = true;
On Sun, 2023-10-15 at 18:29 +0100, Al Viro wrote: > ... checking that after lock_rename() is too late. Incidentally, > NFSv2 had no nfserr_xdev... > > Fixes: aa387d6ce153 "nfsd: fix EXDEV checking in rename" > Cc: stable@vger.kernel.org # v3.9+ > Reviewed-by: Jeff Layton <jlayton@kernel.org> > Signed-off-by: Al Viro <viro@zeniv.linux.org.uk> > --- > > [in git://git.kernel.org/pub/scm/linux/kernel/git/viro/vfs.git #nfsd-fix; > it's an immutable branch, please either pull from it or put that thing > into an immutable branch in your tree - there's a lock_rename-related > series in the making and I'd rather avoid mixing unrelated nfsd stuff > into it ;-/] > > diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c > index 48260cf68fde..02f5fcaad03f 100644 > --- a/fs/nfsd/vfs.c > +++ b/fs/nfsd/vfs.c > @@ -1788,6 +1788,12 @@ nfsd_rename(struct svc_rqst *rqstp, struct svc_fh *ffhp, char *fname, int flen, > if (!flen || isdotent(fname, flen) || !tlen || isdotent(tname, tlen)) > goto out; > > + err = (rqstp->rq_vers == 2) ? nfserr_acces : nfserr_xdev; > + if (ffhp->fh_export->ex_path.mnt != tfhp->fh_export->ex_path.mnt) > + goto out; > + if (ffhp->fh_export->ex_path.dentry != tfhp->fh_export->ex_path.dentry) > + goto out; > + > retry: > host_err = fh_want_write(ffhp); > if (host_err) { > @@ -1823,12 +1829,6 @@ nfsd_rename(struct svc_rqst *rqstp, struct svc_fh *ffhp, char *fname, int flen, > if (ndentry == trap) > goto out_dput_new; > > - host_err = -EXDEV; > - if (ffhp->fh_export->ex_path.mnt != tfhp->fh_export->ex_path.mnt) > - goto out_dput_new; > - if (ffhp->fh_export->ex_path.dentry != tfhp->fh_export->ex_path.dentry) > - goto out_dput_new; > - > if ((ndentry->d_sb->s_export_op->flags & EXPORT_OP_CLOSE_BEFORE_UNLINK) && > nfsd_has_cached_files(ndentry)) { > close_cached = true; I ran this through pynfs and fstests (via kdevops) and it seemed to do fine. You can also add: Tested-by: Jeff Layton <jlayton@kernel.org>
On Sun, Oct 15, 2023 at 06:29:27PM +0100, Al Viro wrote: > ... checking that after lock_rename() is too late. Incidentally, > NFSv2 had no nfserr_xdev... > > Fixes: aa387d6ce153 "nfsd: fix EXDEV checking in rename" > Cc: stable@vger.kernel.org # v3.9+ > Reviewed-by: Jeff Layton <jlayton@kernel.org> > Signed-off-by: Al Viro <viro@zeniv.linux.org.uk> > --- > > [in git://git.kernel.org/pub/scm/linux/kernel/git/viro/vfs.git #nfsd-fix; > it's an immutable branch, please either pull from it or put that thing > into an immutable branch in your tree - there's a lock_rename-related > series in the making and I'd rather avoid mixing unrelated nfsd stuff > into it ;-/] > > diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c > index 48260cf68fde..02f5fcaad03f 100644 > --- a/fs/nfsd/vfs.c > +++ b/fs/nfsd/vfs.c > @@ -1788,6 +1788,12 @@ nfsd_rename(struct svc_rqst *rqstp, struct svc_fh *ffhp, char *fname, int flen, > if (!flen || isdotent(fname, flen) || !tlen || isdotent(tname, tlen)) > goto out; > > + err = (rqstp->rq_vers == 2) ? nfserr_acces : nfserr_xdev; > + if (ffhp->fh_export->ex_path.mnt != tfhp->fh_export->ex_path.mnt) > + goto out; > + if (ffhp->fh_export->ex_path.dentry != tfhp->fh_export->ex_path.dentry) > + goto out; > + > retry: > host_err = fh_want_write(ffhp); > if (host_err) { > @@ -1823,12 +1829,6 @@ nfsd_rename(struct svc_rqst *rqstp, struct svc_fh *ffhp, char *fname, int flen, > if (ndentry == trap) > goto out_dput_new; > > - host_err = -EXDEV; > - if (ffhp->fh_export->ex_path.mnt != tfhp->fh_export->ex_path.mnt) > - goto out_dput_new; > - if (ffhp->fh_export->ex_path.dentry != tfhp->fh_export->ex_path.dentry) > - goto out_dput_new; > - > if ((ndentry->d_sb->s_export_op->flags & EXPORT_OP_CLOSE_BEFORE_UNLINK) && > nfsd_has_cached_files(ndentry)) { > close_cached = true; I ran the git regression suite with 12 threads on NFS/RDMA mounts using a matrix of NFSv3, NFSv4.0, NFSv4.1, and NFSv4.2 on exports of tmpfs, btrfs, xfs, and ext4. All runs completed successfully and in a timely manner. I think the patch is good to go.
diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c index 48260cf68fde..02f5fcaad03f 100644 --- a/fs/nfsd/vfs.c +++ b/fs/nfsd/vfs.c @@ -1788,6 +1788,12 @@ nfsd_rename(struct svc_rqst *rqstp, struct svc_fh *ffhp, char *fname, int flen, if (!flen || isdotent(fname, flen) || !tlen || isdotent(tname, tlen)) goto out; + err = (rqstp->rq_vers == 2) ? nfserr_acces : nfserr_xdev; + if (ffhp->fh_export->ex_path.mnt != tfhp->fh_export->ex_path.mnt) + goto out; + if (ffhp->fh_export->ex_path.dentry != tfhp->fh_export->ex_path.dentry) + goto out; + retry: host_err = fh_want_write(ffhp); if (host_err) { @@ -1823,12 +1829,6 @@ nfsd_rename(struct svc_rqst *rqstp, struct svc_fh *ffhp, char *fname, int flen, if (ndentry == trap) goto out_dput_new; - host_err = -EXDEV; - if (ffhp->fh_export->ex_path.mnt != tfhp->fh_export->ex_path.mnt) - goto out_dput_new; - if (ffhp->fh_export->ex_path.dentry != tfhp->fh_export->ex_path.dentry) - goto out_dput_new; - if ((ndentry->d_sb->s_export_op->flags & EXPORT_OP_CLOSE_BEFORE_UNLINK) && nfsd_has_cached_files(ndentry)) { close_cached = true;