From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from out02.mta.xmission.com ([166.70.13.232]:58764 "EHLO out02.mta.xmission.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1727487AbeJZIGr (ORCPT ); Fri, 26 Oct 2018 04:06:47 -0400 From: ebiederm@xmission.com (Eric W. Biederman) To: "Benjamin Coddington" Cc: "Alexander Viro" , linux-fsdevel@vger.kernel.org References: <379eb36e37e77e3c71b2f6579664c525bda03f61.1538575707.git.bcodding@redhat.com> <9d2d5b14bac03eb3cd9d6daa476a294e83a69ab6.1538576163.git.bcodding@redhat.com> <477E0417-EF75-4A2D-AB3E-0C5E6B8E1D67@redhat.com> Date: Thu, 25 Oct 2018 18:31:28 -0500 In-Reply-To: <477E0417-EF75-4A2D-AB3E-0C5E6B8E1D67@redhat.com> (Benjamin Coddington's message of "Mon, 22 Oct 2018 10:14:53 -0400") Message-ID: <87pnvxk3hr.fsf@xmission.com> MIME-Version: 1.0 Content-Type: text/plain Subject: Re: [PATCH v2] mnt: fix __detach_mounts infinite loop Sender: linux-fsdevel-owner@vger.kernel.org List-ID: "Benjamin Coddington" 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" > 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 >> --- >> 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