linux-fsdevel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: ebiederm@xmission.com (Eric W. Biederman)
To: "Benjamin Coddington" <bcodding@redhat.com>
Cc: "Alexander Viro" <viro@zeniv.linux.org.uk>,
	linux-fsdevel@vger.kernel.org
Subject: Re: [PATCH v2] mnt: fix __detach_mounts infinite loop
Date: Thu, 25 Oct 2018 18:31:28 -0500	[thread overview]
Message-ID: <87pnvxk3hr.fsf@xmission.com> (raw)
In-Reply-To: <477E0417-EF75-4A2D-AB3E-0C5E6B8E1D67@redhat.com> (Benjamin Coddington's message of "Mon, 22 Oct 2018 10:14:53 -0400")

"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

      reply	other threads:[~2018-10-26  8:06 UTC|newest]

Thread overview: 4+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-10-03 14:10 [PATCH] " Benjamin Coddington
2018-10-03 14:18 ` [PATCH v2] " Benjamin Coddington
2018-10-22 14:14   ` Benjamin Coddington
2018-10-25 23:31     ` Eric W. Biederman [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=87pnvxk3hr.fsf@xmission.com \
    --to=ebiederm@xmission.com \
    --cc=bcodding@redhat.com \
    --cc=linux-fsdevel@vger.kernel.org \
    --cc=viro@zeniv.linux.org.uk \
    --subject='Re: [PATCH v2] mnt: fix __detach_mounts infinite loop' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).