| Message ID | 9d2d5b14bac03eb3cd9d6daa476a294e83a69ab6.1538576163.git.bcodding@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] mnt: fix __detach_mounts infinite loop | expand |
Al, would you be willing to take a look at this one? 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
"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
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);
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(-)