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

[NeilBrown <neilb@suse.com>]

[-- Attachment #1: Type: text/plain, Size: 4230 bytes --]

On Fri, Mar 31 2017, J. Bruce Fields wrote:

> 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.

4.12 is fine with me.
Yes, it has been lurking.  As containers and other users of filesystem
name spaces become more common, we can expect this bug to trigger more
often.
I've only seen it under test conditions - not in a real-world use case -
so I don't think there is any rush.

Thanks,
NeilBrown


>
> --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
>> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]