Skip to contentSkip to conversation

xfs: don't leak root inode reference

PATCH Superseded Ben Myers 3 participants 8 messages Aug 26, 2013 – Oct 1, 2013 XFS filesystem

lore
Share & tools
lore

Ben Myers

author
#0 lore raw

PATCH 1/1 xfs: don't leak root inode reference

Looks like in 48fde701 we removed the iput of the root inode in xfs_fs_fill_super for the error case. Add it back.

Signed-off-by Ben Myers <[email protected]>
---
 fs/xfs/xfs_super.c |    6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)
fs/xfs/xfs_super.c +4 -2
Index: b/fs/xfs/xfs_super.c
===================================================================
@@ -1493,12 +1493,12 @@ xfs_fs_fill_super(
 	}
 	if (is_bad_inode(root)) {
 		error = EINVAL;
-		goto out_unmount;
+		goto out_iput;
 	}
 	sb->s_root = d_make_root(root);
 	if (!sb->s_root) {
 		error = ENOMEM;
-		goto out_unmount;
+		goto out_iput;
 	}
 
 	return 0;

@@ -1519,6 +1519,8 @@ out_destroy_workqueues:
  out:
 	return -error;
 
+ out_iput:
+	iput(root);
  out_unmount:
 	xfs_filestream_unmount(mp);
 	xfs_unmountfs(mp);

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

#1 lore raw

fs/xfs/xfs_super.c xfs_fs_fill_super( · line 1496

Quoted 6 lines · Ben Myers

Looks like in 48fde701 we removed the iput of the root inode in xfs_fs_fill_super for the error case. Add it back.

fs/xfs/xfs_super.c +1 -1
 	if (is_bad_inode(root)) {
 		error = EINVAL;
-		goto out_unmount;
+		goto out_iput;

This one makes sense

fs/xfs/xfs_super.c xfs_fs_fill_super( · line 1501

Quoted 6 lines
fs/xfs/xfs_super.c +1 -1
 	}
 	sb->s_root = d_make_root(root);
 	if (!sb->s_root) {
 		error = ENOMEM;
-		goto out_unmount;
+		goto out_iput;

... this one, OTOH, is simply wrong - take a look at what d_make_root() does on failure.

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

#2 lore raw
In reply to Ben Myers
Quoted 16 lines · Ben Myers

Looks like in 48fde701 we removed the iput of the root inode in xfs_fs_fill_super for the error case. Add it back.

Signed-off-by Ben Myers <[email protected]>
---
 fs/xfs/xfs_super.c |    6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

Index: b/fs/xfs/xfs_super.c
===================================================================

fs/xfs/xfs_super.c +0 -0
@@ -1493,12 +1493,12 @@ xfs_fs_fill_super(
 	}
 	if (is_bad_inode(root)) {

fs/xfs/xfs_super.c xfs_fs_fill_super( · line 1502

Quoted 10 lines
fs/xfs/xfs_super.c +2 -2
 		error = EINVAL;
-		goto out_unmount;
+		goto out_iput;
 	}
 	sb->s_root = d_make_root(root);
 	if (!sb->s_root) {
 		error = ENOMEM;
-		goto out_unmount;
+		goto out_iput;
 	}

That's wrong. d_make_root() drops the reference to the inode on failure itself, and so the change in 48fde701 is correct and valid.

The leak on bad inodes (which, AFAICT, can never happen on XFS) has been around a lot longer than Al's change - this commit introduced it:

2bcf6e9 xfs: start periodic workers later

with this hunk:

        if (is_bad_inode(root)) {
                error = EINVAL;
-               goto fail_vnrele;
+               goto out_syncd_stop;
        }

Cheers,

Dave.

Signature · 7 lines
Dave Chinner
[email protected]

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
author
#3 lore raw

PATCH v2 1/1 xfs: don't leak root inode reference

Quoted 41 lines · Dave Chinner
Quoted 16 lines · Ben Myers

Looks like in 48fde701 we removed the iput of the root inode in xfs_fs_fill_super for the error case. Add it back.

Signed-off-by Ben Myers <[email protected]>
---
 fs/xfs/xfs_super.c |    6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

Index: b/fs/xfs/xfs_super.c
===================================================================

fs/xfs/xfs_super.c +0 -0
@@ -1493,12 +1493,12 @@ xfs_fs_fill_super(
 	}
 	if (is_bad_inode(root)) {

fs/xfs/xfs_super.c xfs_fs_fill_super( · line 1502

Quoted 10 lines
fs/xfs/xfs_super.c +2 -2
 		error = EINVAL;
-		goto out_unmount;
+		goto out_iput;
 	}
 	sb->s_root = d_make_root(root);
 	if (!sb->s_root) {
 		error = ENOMEM;
-		goto out_unmount;
+		goto out_iput;
 	}

That's wrong. d_make_root() drops the reference to the inode on failure itself, and so the change in 48fde701 is correct and valid.

The leak on bad inodes (which, AFAICT, can never happen on XFS) has been around a lot longer than Al's change - this commit introduced it:

2bcf6e9 xfs: start periodic workers later

with this hunk:

        if (is_bad_inode(root)) {
                error = EINVAL;

Review

Quoted 3 lines
(quoted diff) +1 -1
-               goto fail_vnrele;
+               goto out_syncd_stop;
        }

Thanks Gents. Here's another try:

xfs: don't leak root inode reference

Looks like in 2bcf6e9 we removed the iput of the root inode in xfs_fs_fill_super for the is_bad_inode error case. Add it back.

Signed-off-by Ben Myers <[email protected]>

---

 fs/xfs/xfs_super.c |    4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)
fs/xfs/xfs_super.c +3 -1
Index: b/fs/xfs/xfs_super.c
===================================================================
@@ -1493,7 +1493,7 @@ xfs_fs_fill_super(
 	}
 	if (is_bad_inode(root)) {
 		error = EINVAL;
-		goto out_unmount;
+		goto out_iput;
 	}
 	sb->s_root = d_make_root(root);
 	if (!sb->s_root) {

@@ -1519,6 +1519,8 @@ out_destroy_workqueues:
  out:
 	return -error;
 
+ out_iput:
+	iput(root);
  out_unmount:
 	xfs_filestream_unmount(mp);
 	xfs_unmountfs(mp);

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

#4 lore raw
Quoted 46 lines · Ben Myers
Quoted 41 lines · Dave Chinner
Quoted 16 lines · Ben Myers

Looks like in 48fde701 we removed the iput of the root inode in xfs_fs_fill_super for the error case. Add it back.

Signed-off-by Ben Myers <[email protected]>
---
 fs/xfs/xfs_super.c |    6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

Index: b/fs/xfs/xfs_super.c
===================================================================

fs/xfs/xfs_super.c +0 -0
@@ -1493,12 +1493,12 @@ xfs_fs_fill_super(
 	}
 	if (is_bad_inode(root)) {

fs/xfs/xfs_super.c xfs_fs_fill_super( · line 1502

Quoted 10 lines
fs/xfs/xfs_super.c +2 -2
 		error = EINVAL;
-		goto out_unmount;
+		goto out_iput;
 	}
 	sb->s_root = d_make_root(root);
 	if (!sb->s_root) {
 		error = ENOMEM;
-		goto out_unmount;
+		goto out_iput;
 	}

That's wrong. d_make_root() drops the reference to the inode on failure itself, and so the change in 48fde701 is correct and valid.

The leak on bad inodes (which, AFAICT, can never happen on XFS) has been around a lot longer than Al's change - this commit introduced it:

2bcf6e9 xfs: start periodic workers later

with this hunk:

        if (is_bad_inode(root)) {
                error = EINVAL;

Review

Quoted 3 lines
(quoted diff) +1 -1
-               goto fail_vnrele;
+               goto out_syncd_stop;
        }
Quoted 8 lines

Thanks Gents. Here's another try:

xfs: don't leak root inode reference

Looks like in 2bcf6e9 we removed the iput of the root inode in xfs_fs_fill_super for the is_bad_inode error case. Add it back.

Signed-off-by Ben Myers <[email protected]>

I don't think this is right, either.

As I said in my previous reply, I don't think that XFS can ever see a bad inode. The fact is that we're grabbing mp->m_rootip, which is we already have a reference to and is in cache and validated thanks to an xfs_iget() call in xfs_mountfs(). If we fail validation when reading the root inode into cache then xfs_mountfs() will fail and we won't ever get to this check.

Further, XFS never marks inodes bad - even on a failed lookup or a shut down filesystem - and so AFAICT we cannot ever see the root inode (or any other XFS inode) as a bad inode.

Hence I think that the is_bad_inode(root) check should just go away.

Cheers,

Dave.

Signature · 7 lines
Dave Chinner
[email protected]

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
author
#5 lore raw

Hi Dave,

Quoted 65 lines · Dave Chinner
Quoted 46 lines · Ben Myers
Quoted 41 lines · Dave Chinner
Quoted 16 lines · Ben Myers

Looks like in 48fde701 we removed the iput of the root inode in xfs_fs_fill_super for the error case. Add it back.

Signed-off-by Ben Myers <[email protected]>
---
 fs/xfs/xfs_super.c |    6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

Index: b/fs/xfs/xfs_super.c
===================================================================

fs/xfs/xfs_super.c +0 -0
@@ -1493,12 +1493,12 @@ xfs_fs_fill_super(
 	}
 	if (is_bad_inode(root)) {

fs/xfs/xfs_super.c xfs_fs_fill_super( · line 1502

Quoted 10 lines
fs/xfs/xfs_super.c +2 -2
 		error = EINVAL;
-		goto out_unmount;
+		goto out_iput;
 	}
 	sb->s_root = d_make_root(root);
 	if (!sb->s_root) {
 		error = ENOMEM;
-		goto out_unmount;
+		goto out_iput;
 	}

That's wrong. d_make_root() drops the reference to the inode on failure itself, and so the change in 48fde701 is correct and valid.

The leak on bad inodes (which, AFAICT, can never happen on XFS) has been around a lot longer than Al's change - this commit introduced it:

2bcf6e9 xfs: start periodic workers later

with this hunk:

        if (is_bad_inode(root)) {
                error = EINVAL;

Review

Quoted 3 lines
(quoted diff) +1 -1
-               goto fail_vnrele;
+               goto out_syncd_stop;
        }
Quoted 8 lines

Thanks Gents. Here's another try:

xfs: don't leak root inode reference

Looks like in 2bcf6e9 we removed the iput of the root inode in xfs_fs_fill_super for the is_bad_inode error case. Add it back.

Signed-off-by Ben Myers <[email protected]>

I don't think this is right, either.

As I said in my previous reply, I don't think that XFS can ever see a bad inode. The fact is that we're grabbing mp->m_rootip, which is we already have a reference to and is in cache and validated thanks to an xfs_iget() call in xfs_mountfs(). If we fail validation when reading the root inode into cache then xfs_mountfs() will fail and we won't ever get to this check.

Quoted 5 lines

Further, XFS never marks inodes bad - even on a failed lookup or a shut down filesystem - and so AFAICT we cannot ever see the root inode (or any other XFS inode) as a bad inode.

Hence I think that the is_bad_inode(root) check should just go away.

Ok. Lets try this. ;)

xfs: remove usage of is_bad_inode

XFS never calls mark_inode_bad or iget_failed, so it will never see a bad inode. Remove all checks for is_bad_inode because they are unnecessary.

Signed-off-by Ben Myers <[email protected]>
---
 fs/xfs/xfs_icache.c |    7 -------
 fs/xfs/xfs_inode.c  |    2 +-
 fs/xfs/xfs_super.c  |    9 ---------
 3 files changed, 1 insertion(+), 17 deletions(-)
fs/xfs/xfs_icache.c +0 -7
Index: b/fs/xfs/xfs_icache.c
===================================================================
@@ -501,11 +501,6 @@ xfs_inode_ag_walk_grab(
 	if (!igrab(inode))
 		return ENOENT;
 
-	if (is_bad_inode(inode)) {
-		IRELE(ip);
-		return ENOENT;
-	}
-
 	/* inode is valid */
 	return 0;
 

@@ -919,8 +914,6 @@ restart:
 		xfs_iflock(ip);
 	}
 
-	if (is_bad_inode(VFS_I(ip)))
-		goto reclaim;
 	if (XFS_FORCED_SHUTDOWN(ip->i_mount)) {
 		xfs_iunpin_wait(ip);
 		xfs_iflush_abort(ip, false);
fs/xfs/xfs_inode.c +1 -1
Index: b/fs/xfs/xfs_inode.c
===================================================================
@@ -1687,7 +1687,7 @@ xfs_inactive(
 	 * If the inode is already free, then there can be nothing
 	 * to clean up here.
 	 */
-	if (ip->i_d.di_mode == 0 || is_bad_inode(VFS_I(ip))) {
+	if (ip->i_d.di_mode == 0) {
 		ASSERT(ip->i_df.if_real_bytes == 0);
 		ASSERT(ip->i_df.if_broot_bytes == 0);
 		return VN_INACTIVE_CACHE;
fs/xfs/xfs_super.c +0 -9
Index: b/fs/xfs/xfs_super.c
===================================================================
@@ -946,10 +946,6 @@ xfs_fs_destroy_inode(
 
 	XFS_STATS_INC(vn_reclaim);
 
-	/* bad inode, get out here ASAP */
-	if (is_bad_inode(inode))
-		goto out_reclaim;
-
 	ASSERT(XFS_FORCED_SHUTDOWN(ip->i_mount) || ip->i_delayed_blks == 0);
 
 	/*

@@ -965,7 +961,6 @@ xfs_fs_destroy_inode(
 	 * this more efficiently than we can here, so simply let background
 	 * reclaim tear down all inodes.
 	 */
-out_reclaim:
 	xfs_inode_set_reclaim_tag(ip);
 }
 

@@ -1491,10 +1486,6 @@ xfs_fs_fill_super(
 		error = ENOENT;
 		goto out_unmount;
 	}
-	if (is_bad_inode(root)) {
-		error = EINVAL;
-		goto out_unmount;
-	}
 	sb->s_root = d_make_root(root);
 	if (!sb->s_root) {
 		error = ENOMEM;

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

Reviewed-by
#6 lore raw
Quoted 9 lines · Ben Myers

Ok. Lets try this. ;)

xfs: remove usage of is_bad_inode

XFS never calls mark_inode_bad or iget_failed, so it will never see a bad inode. Remove all checks for is_bad_inode because they are unnecessary.

Signed-off-by Ben Myers <[email protected]>

Looks good to me.

Reviewed-by Dave Chinner <[email protected]>
Signature · 7 lines
Dave Chinner
[email protected]

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
author
#7 lore raw
In reply to Ben Myers
Quoted 76 lines · Ben Myers

Hi Dave,

Quoted 65 lines · Dave Chinner
Quoted 46 lines · Ben Myers
Quoted 41 lines · Dave Chinner
Quoted 16 lines · Ben Myers

Looks like in 48fde701 we removed the iput of the root inode in xfs_fs_fill_super for the error case. Add it back.

Signed-off-by Ben Myers <[email protected]>
---
 fs/xfs/xfs_super.c |    6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

Index: b/fs/xfs/xfs_super.c
===================================================================

fs/xfs/xfs_super.c +0 -0
@@ -1493,12 +1493,12 @@ xfs_fs_fill_super(
 	}
 	if (is_bad_inode(root)) {

fs/xfs/xfs_super.c xfs_fs_fill_super( · line 1502

Quoted 10 lines
fs/xfs/xfs_super.c +2 -2
 		error = EINVAL;
-		goto out_unmount;
+		goto out_iput;
 	}
 	sb->s_root = d_make_root(root);
 	if (!sb->s_root) {
 		error = ENOMEM;
-		goto out_unmount;
+		goto out_iput;
 	}

That's wrong. d_make_root() drops the reference to the inode on failure itself, and so the change in 48fde701 is correct and valid.

The leak on bad inodes (which, AFAICT, can never happen on XFS) has been around a lot longer than Al's change - this commit introduced it:

2bcf6e9 xfs: start periodic workers later

with this hunk:

        if (is_bad_inode(root)) {
                error = EINVAL;

Review

Quoted 3 lines
(quoted diff) +1 -1
-               goto fail_vnrele;
+               goto out_syncd_stop;
        }
Quoted 8 lines

Thanks Gents. Here's another try:

xfs: don't leak root inode reference

Looks like in 2bcf6e9 we removed the iput of the root inode in xfs_fs_fill_super for the is_bad_inode error case. Add it back.

Signed-off-by Ben Myers <[email protected]>

I don't think this is right, either.

As I said in my previous reply, I don't think that XFS can ever see a bad inode. The fact is that we're grabbing mp->m_rootip, which is we already have a reference to and is in cache and validated thanks to an xfs_iget() call in xfs_mountfs(). If we fail validation when reading the root inode into cache then xfs_mountfs() will fail and we won't ever get to this check.

Quoted 5 lines

Further, XFS never marks inodes bad - even on a failed lookup or a shut down filesystem - and so AFAICT we cannot ever see the root inode (or any other XFS inode) as a bad inode.

Hence I think that the is_bad_inode(root) check should just go away.

Ok. Lets try this. ;)

Quoted 7 lines

xfs: remove usage of is_bad_inode

XFS never calls mark_inode_bad or iget_failed, so it will never see a bad inode. Remove all checks for is_bad_inode because they are unnecessary.

Signed-off-by Ben Myers <[email protected]>

Applied this one.

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs