Re: [PATCH v2] mnt: fix __detach_mounts infinite loop

[ebiederm@xmission.com (Eric W. Biederman)]

"Benjamin Coddington" <bcodding@redhat.com> writes:

> Al, would you be willing to take a look at this one?


As of ff17fa561a04 ("d_invalidate(): unhash immediately") d_invalidate
will call __detach_mounts with the dentry unhashed/unlinked if there
is something mounted directly on top of it.  Ordinary unlink won't
be called for dentries that have amount on top.  But the examples in the
patch description will.

I don't see that __detach_mounts cares one way or another if the dentry
it is detaching things from is unlinked/unhashed.  __detach_mounts just
wants to get remove all of the mounts from that dentry.  So removing the
"d_unlinked(dentry)" test from lookup_mount seems fine.

The code in get_mountpoint definitely needs the "d_unlinked(dentry)"
test to ensure new mounts stop being created after d_invalidate calls
__d_drop.  It does not seem to make much difference where the
"d_unlinked(dentry)" test is in get_mountpoint because it will always be
possible for d_invalidate to come in right after get_mountpoint returns.

Because d_invalidate waits for every dentry to have d_mountpoint cleared
it is guaranteed that d_mountpoint won't return until every mount is
cleaned up.

So this looks as good as it can get to me.

Reviewed-by: "Eric W. Biederman" <ebiederm@xmission.com>

> Ben
>
> On 3 Oct 2018, at 10:18, Benjamin Coddington wrote:
>
>> v2: less whitespace
>>
>> 8<------------------------------------------------------------------------
>>
>> Since commit ff17fa561a04 ("d_invalidate(): unhash immediately")
>> immediately unhashes the dentry, we'll never return the mountpoint in
>> lookup_mountpoint(), which can lead to an unbreakable loop in
>> d_invalidate().
>>
>> I have reports of NFS clients getting into this condition after the server
>> removes an export of an existing mount created through follow_automount(),
>> but I suspect there are various other ways to produce this problem if we
>> hunt down users of d_invalidate().  For example, it is possible to get into
>> this state by using XFS' d_invalidate() call in xfs_vn_unlink():
>>
>> truncate -s 100m img{1,2}
>>
>> mkfs.xfs -q -n version=ci img1
>> mkfs.xfs -q -n version=ci img2
>>
>> mkdir -p /mnt/xfs
>> mount img1 /mnt/xfs
>>
>> mkdir /mnt/xfs/sub1
>> mount img2 /mnt/xfs/sub1
>>
>> cat > /mnt/xfs/sub1/foo &
>> umount -l /mnt/xfs/sub1
>> mount img2 /mnt/xfs/sub1
>>
>> mount --make-private /mnt/xfs
>>
>> mkdir /mnt/xfs/sub2
>> mount --move /mnt/xfs/sub1 /mnt/xfs/sub2
>> rmdir /mnt/xfs/sub1
>>
>> Fix this by moving the check for an unlinked dentry out of the
>> detach_mounts() path.
>>
>> Fixes: ff17fa561a04 ("d_invalidate(): unhash immediately")
>> Signed-off-by: Benjamin Coddington <bcodding@redhat.com>
>> ---
>>  fs/namespace.c | 6 +++---
>>  1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/fs/namespace.c b/fs/namespace.c
>> index bd2f4c68506a..ad8feb9371f2 100644
>> --- a/fs/namespace.c
>> +++ b/fs/namespace.c
>> @@ -780,9 +780,6 @@ static struct mountpoint *lookup_mountpoint(struct dentry
>> *dentry)
>>
>>  	hlist_for_each_entry(mp, chain, m_hash) {
>>  		if (mp->m_dentry == dentry) {
>> -			/* might be worth a WARN_ON() */
>> -			if (d_unlinked(dentry))
>> -				return ERR_PTR(-ENOENT);
>>  			mp->m_count++;
>>  			return mp;
>>  		}
>> @@ -796,6 +793,9 @@ static struct mountpoint *get_mountpoint(struct dentry
>> *dentry)
>>  	int ret;
>>
>>  	if (d_mountpoint(dentry)) {
>> +		/* might be worth a WARN_ON() */
>> +		if (d_unlinked(dentry))
>> +			return ERR_PTR(-ENOENT);
>>  mountpoint:
>>  		read_seqlock_excl(&mount_lock);
>>  		mp = lookup_mountpoint(dentry);
>> -- 
>> 2.14.3