Received-SPF: pass (google.com: domain of linux-nfs-owner@vger.kernel.org designates 2620:137:e000::3:5 as permitted sender) client-ip=2620:137:e000::3:5;
Message-ID: <f81c761a4b43c8343dfb972c8b954cd25cfeeefa.camel@kernel.org>
Subject: Re: [PATCH] nfsd: lock_rename() needs both directories to live on
 the same fs
From:   Jeff Layton <jlayton@kernel.org>
To:     Al Viro <viro@zeniv.linux.org.uk>, linux-nfs@vger.kernel.org
Date:   Sun, 15 Oct 2023 16:44:17 -0400
In-Reply-To: <20231015172927.GE800259@ZenIV>
References: <20231015172927.GE800259@ZenIV>
Content-Type: text/plain; charset="ISO-8859-15"
Content-Transfer-Encoding: quoted-printable
User-Agent: Evolution 3.48.4 (3.48.4-1.fc38) 
MIME-Version: 1.0
Precedence: bulk

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...
>=20
> 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>
> ---
>=20
> [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 ;-/]
>=20
> 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;
> =20
> +	err =3D (rqstp->rq_vers =3D=3D 2) ? nfserr_acces : nfserr_xdev;
> +	if (ffhp->fh_export->ex_path.mnt !=3D tfhp->fh_export->ex_path.mnt)
> +		goto out;
> +	if (ffhp->fh_export->ex_path.dentry !=3D tfhp->fh_export->ex_path.dentr=
y)
> +		goto out;
> +
>  retry:
>  	host_err =3D 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 =3D=3D trap)
>  		goto out_dput_new;
> =20
> -	host_err =3D -EXDEV;
> -	if (ffhp->fh_export->ex_path.mnt !=3D tfhp->fh_export->ex_path.mnt)
> -		goto out_dput_new;
> -	if (ffhp->fh_export->ex_path.dentry !=3D tfhp->fh_export->ex_path.dentr=
y)
> -		goto out_dput_new;
> -
>  	if ((ndentry->d_sb->s_export_op->flags & EXPORT_OP_CLOSE_BEFORE_UNLINK)=
 &&
>  	    nfsd_has_cached_files(ndentry)) {
>  		close_cached =3D 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>