Re: [PATCH] NFS: don't try to cross a mountpount when there isn't one there.

["J. Bruce Fields" <bfields@fieldses.org>]

Sorry for the delay, I need to find a little time to digest that one.

Makes sense to me--but it's a little subtle, and it looks like this
bug's been lurking for a few years, so I think I'll let it wait for
4.12 if that's OK.

--b.

On Wed, Mar 15, 2017 at 12:40:44PM +1100, NeilBrown wrote:
> 
> consider the sequence of commands:
>  mkdir -p /import/nfs /import/bind /import/etc
>  mount --bind / /import/bind
>  mount --make-private /import/bind
>  mount --bind /import/etc /import/bind/etc
> 
>  exportfs -o rw,no_root_squash,crossmnt,async,no_subtree_check localhost:/
>  mount -o vers=4 localhost:/ /import/nfs
>  ls -l /import/nfs/etc
> 
> You would not expect this to report a stale file handle.
> Yet it does.
> 
> The manipulations under /import/bind cause the dentry for
> /etc to get the DCACHE_MOUNTED flag set, even though nothing
> is mounted on /etc.  This causes nfsd to call
> nfsd_cross_mnt() even though there is no mountpoint.  So an
> upcall to mountd for "/etc" is performed.
> 
> The 'crossmnt' flag on the export of / causes mountd to
> report that /etc is exported as it is a descendant of /.  It
> assumes the kernel wouldn't ask about something that wasn't
> a mountpoint.  The filehandle returned identifies the
> filesystem and the inode number of /etc.
> 
> When this filehandle is presented to rpc.mountd, via
> "nfsd.fh", the inode cannot be found associated with any
> name in /etc/exports, or with any mountpoint listed by
> getmntent().  So rpc.mountd says the filehandle doesn't
> exist. Hence ESTALE.
> 
> This is fixed by teaching nfsd not to trust DCACHE_MOUNTD
> too much.  It is just a hint, not a guarantee.
> Change nfsd_mountpoint() to return '1' for a certain mountpoint,
> '2' for a possible mountpoint, and 0 otherwise.
> 
> Then change nfsd_crossmnt() to check if follow_down()
> actually found a mountpount and, if not, to avoid performing
> a lookup if the location is not known to certainly require
> an export-point.
> 
> Signed-off-by: NeilBrown <neilb@suse.com>
> ---
>  fs/nfsd/vfs.c | 24 ++++++++++++++++++++----
>  1 file changed, 20 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
> index 19d50f600e8d..04cafaa94bf7 100644
> --- a/fs/nfsd/vfs.c
> +++ b/fs/nfsd/vfs.c
> @@ -94,6 +94,12 @@ nfsd_cross_mnt(struct svc_rqst *rqstp, struct dentry **dpp,
>  	err = follow_down(&path);
>  	if (err < 0)
>  		goto out;
> +	if (path.mnt == exp->ex_path.mnt && path.dentry == dentry &&
> +	    nfsd_mountpoint(dentry, exp) == 2) {
> +		/* This is only a mountpoint in some other namespace */
> +		path_put(&path);
> +		goto out;
> +	}
>  
>  	exp2 = rqst_exp_get_by_name(rqstp, &path);
>  	if (IS_ERR(exp2)) {
> @@ -167,16 +173,26 @@ static int nfsd_lookup_parent(struct svc_rqst *rqstp, struct dentry *dparent, st
>  /*
>   * For nfsd purposes, we treat V4ROOT exports as though there was an
>   * export at *every* directory.
> + * We return:
> + * '1' if this dentry *must* be an export point,
> + * '2' if it might be, if there is really a mount here, and
> + * '0' if there is no chance of an export point here.
>   */
>  int nfsd_mountpoint(struct dentry *dentry, struct svc_export *exp)
>  {
> -	if (d_mountpoint(dentry))
> +	if (!d_inode(dentry))
> +		return 0;
> +	if (exp->ex_flags & NFSEXP_V4ROOT)
>  		return 1;
>  	if (nfsd4_is_junction(dentry))
>  		return 1;
> -	if (!(exp->ex_flags & NFSEXP_V4ROOT))
> -		return 0;
> -	return d_inode(dentry) != NULL;
> +	if (d_mountpoint(dentry))
> +		/*
> +		 * Might only be a mountpoint in a different namespace,
> +		 * but we need to check.
> +		 */
> +		return 2;
> +	return 0;
>  }
>  
>  __be32
> -- 
> 2.12.0
>