* Re: [PATCH] nfsd: lock_rename() needs both directories to live on the same fs
2023-10-15 17:29 [PATCH] nfsd: lock_rename() needs both directories to live on the same fs Al Viro
@ 2023-10-15 17:47 ` Chuck Lever
2023-10-15 20:44 ` Jeff Layton
2023-10-16 20:32 ` Chuck Lever
2 siblings, 0 replies; 4+ messages in thread
From: Chuck Lever @ 2023-10-15 17:47 UTC (permalink / raw)
To: Al Viro; +Cc: linux-nfs
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;
--
Chuck Lever
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] nfsd: lock_rename() needs both directories to live on the same fs
2023-10-15 17:29 [PATCH] nfsd: lock_rename() needs both directories to live on the same fs Al Viro
2023-10-15 17:47 ` Chuck Lever
@ 2023-10-15 20:44 ` Jeff Layton
2023-10-16 20:32 ` Chuck Lever
2 siblings, 0 replies; 4+ messages in thread
From: Jeff Layton @ 2023-10-15 20:44 UTC (permalink / raw)
To: Al Viro, linux-nfs
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>
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] nfsd: lock_rename() needs both directories to live on the same fs
2023-10-15 17:29 [PATCH] nfsd: lock_rename() needs both directories to live on the same fs Al Viro
2023-10-15 17:47 ` Chuck Lever
2023-10-15 20:44 ` Jeff Layton
@ 2023-10-16 20:32 ` Chuck Lever
2 siblings, 0 replies; 4+ messages in thread
From: Chuck Lever @ 2023-10-16 20:32 UTC (permalink / raw)
To: Al Viro; +Cc: linux-nfs
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.
--
Chuck Lever
^ permalink raw reply [flat|nested] 4+ messages in thread