* [PATCH] xfs: don't leak root inode reference
@ 2013-08-26 20:47 Ben Myers
2013-08-26 21:03 ` Al Viro
2013-08-26 21:24 ` Dave Chinner
0 siblings, 2 replies; 8+ messages in thread
From: Ben Myers @ 2013-08-26 20:47 UTC (permalink / raw)
To: xfs; +Cc: Al Viro
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 <bpm@sgi.com>
---
fs/xfs/xfs_super.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
Index: b/fs/xfs/xfs_super.c
===================================================================
--- a/fs/xfs/xfs_super.c 2013-08-26 15:36:09.170848579 -0500
+++ b/fs/xfs/xfs_super.c 2013-08-26 15:40:19.450817933 -0500
@@ -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
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] xfs: don't leak root inode reference 2013-08-26 20:47 [PATCH] xfs: don't leak root inode reference Ben Myers @ 2013-08-26 21:03 ` Al Viro 2013-08-26 21:24 ` Dave Chinner 1 sibling, 0 replies; 8+ messages in thread From: Al Viro @ 2013-08-26 21:03 UTC (permalink / raw) To: Ben Myers; +Cc: xfs On Mon, Aug 26, 2013 at 03:47:30PM -0500, Ben Myers wrote: > Looks like in 48fde701 we removed the iput of the root inode in > xfs_fs_fill_super for the error case. Add it back. > if (is_bad_inode(root)) { > error = EINVAL; > - goto out_unmount; > + goto out_iput; This one makes sense > } > 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 xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] xfs: don't leak root inode reference 2013-08-26 20:47 [PATCH] xfs: don't leak root inode reference Ben Myers 2013-08-26 21:03 ` Al Viro @ 2013-08-26 21:24 ` Dave Chinner 2013-08-27 21:25 ` [PATCH v2] " Ben Myers 1 sibling, 1 reply; 8+ messages in thread From: Dave Chinner @ 2013-08-26 21:24 UTC (permalink / raw) To: Ben Myers; +Cc: Al Viro, xfs On Mon, Aug 26, 2013 at 03:47:30PM -0500, Ben Myers wrote: > 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 <bpm@sgi.com> > > --- > fs/xfs/xfs_super.c | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > > Index: b/fs/xfs/xfs_super.c > =================================================================== > --- a/fs/xfs/xfs_super.c 2013-08-26 15:36:09.170848579 -0500 > +++ b/fs/xfs/xfs_super.c 2013-08-26 15:40:19.450817933 -0500 > @@ -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; > } 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. -- Dave Chinner david@fromorbit.com _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH v2] xfs: don't leak root inode reference 2013-08-26 21:24 ` Dave Chinner @ 2013-08-27 21:25 ` Ben Myers 2013-08-27 22:08 ` Dave Chinner 0 siblings, 1 reply; 8+ messages in thread From: Ben Myers @ 2013-08-27 21:25 UTC (permalink / raw) To: Dave Chinner; +Cc: Al Viro, xfs On Tue, Aug 27, 2013 at 07:24:23AM +1000, Dave Chinner wrote: > On Mon, Aug 26, 2013 at 03:47:30PM -0500, Ben Myers wrote: > > 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 <bpm@sgi.com> > > > > --- > > fs/xfs/xfs_super.c | 6 ++++-- > > 1 file changed, 4 insertions(+), 2 deletions(-) > > > > Index: b/fs/xfs/xfs_super.c > > =================================================================== > > --- a/fs/xfs/xfs_super.c 2013-08-26 15:36:09.170848579 -0500 > > +++ b/fs/xfs/xfs_super.c 2013-08-26 15:40:19.450817933 -0500 > > @@ -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; > > } > > 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; > } 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 <bpm@sgi.com> --- fs/xfs/xfs_super.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) Index: b/fs/xfs/xfs_super.c =================================================================== --- a/fs/xfs/xfs_super.c 2013-08-26 15:43:55.530817462 -0500 +++ b/fs/xfs/xfs_super.c 2013-08-27 16:20:50.100857436 -0500 @@ -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 xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2] xfs: don't leak root inode reference 2013-08-27 21:25 ` [PATCH v2] " Ben Myers @ 2013-08-27 22:08 ` Dave Chinner 2013-09-10 23:11 ` Ben Myers 0 siblings, 1 reply; 8+ messages in thread From: Dave Chinner @ 2013-08-27 22:08 UTC (permalink / raw) To: Ben Myers; +Cc: Al Viro, xfs On Tue, Aug 27, 2013 at 04:25:58PM -0500, Ben Myers wrote: > On Tue, Aug 27, 2013 at 07:24:23AM +1000, Dave Chinner wrote: > > On Mon, Aug 26, 2013 at 03:47:30PM -0500, Ben Myers wrote: > > > 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 <bpm@sgi.com> > > > > > > --- > > > fs/xfs/xfs_super.c | 6 ++++-- > > > 1 file changed, 4 insertions(+), 2 deletions(-) > > > > > > Index: b/fs/xfs/xfs_super.c > > > =================================================================== > > > --- a/fs/xfs/xfs_super.c 2013-08-26 15:36:09.170848579 -0500 > > > +++ b/fs/xfs/xfs_super.c 2013-08-26 15:40:19.450817933 -0500 > > > @@ -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; > > > } > > > > 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; > > } > > 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 <bpm@sgi.com> 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. -- Dave Chinner david@fromorbit.com _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2] xfs: don't leak root inode reference 2013-08-27 22:08 ` Dave Chinner @ 2013-09-10 23:11 ` Ben Myers 2013-09-18 3:13 ` Dave Chinner 2013-10-01 22:40 ` Ben Myers 0 siblings, 2 replies; 8+ messages in thread From: Ben Myers @ 2013-09-10 23:11 UTC (permalink / raw) To: Dave Chinner; +Cc: Al Viro, xfs Hi Dave, On Wed, Aug 28, 2013 at 08:08:33AM +1000, Dave Chinner wrote: > On Tue, Aug 27, 2013 at 04:25:58PM -0500, Ben Myers wrote: > > On Tue, Aug 27, 2013 at 07:24:23AM +1000, Dave Chinner wrote: > > > On Mon, Aug 26, 2013 at 03:47:30PM -0500, Ben Myers wrote: > > > > 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 <bpm@sgi.com> > > > > > > > > --- > > > > fs/xfs/xfs_super.c | 6 ++++-- > > > > 1 file changed, 4 insertions(+), 2 deletions(-) > > > > > > > > Index: b/fs/xfs/xfs_super.c > > > > =================================================================== > > > > --- a/fs/xfs/xfs_super.c 2013-08-26 15:36:09.170848579 -0500 > > > > +++ b/fs/xfs/xfs_super.c 2013-08-26 15:40:19.450817933 -0500 > > > > @@ -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; > > > > } > > > > > > 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; > > > } > > > > 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 <bpm@sgi.com> > > 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. 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 <bpm@sgi.com> --- 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(-) Index: b/fs/xfs/xfs_icache.c =================================================================== --- a/fs/xfs/xfs_icache.c 2013-09-10 14:55:38.574338256 -0500 +++ b/fs/xfs/xfs_icache.c 2013-09-10 14:56:09.514377472 -0500 @@ -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); Index: b/fs/xfs/xfs_inode.c =================================================================== --- a/fs/xfs/xfs_inode.c 2013-09-10 14:56:38.124337715 -0500 +++ b/fs/xfs/xfs_inode.c 2013-09-10 14:56:40.777424778 -0500 @@ -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; Index: b/fs/xfs/xfs_super.c =================================================================== --- a/fs/xfs/xfs_super.c 2013-09-10 14:38:48.144338844 -0500 +++ b/fs/xfs/xfs_super.c 2013-09-10 14:57:06.793813184 -0500 @@ -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 xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2] xfs: don't leak root inode reference 2013-09-10 23:11 ` Ben Myers @ 2013-09-18 3:13 ` Dave Chinner 2013-10-01 22:40 ` Ben Myers 1 sibling, 0 replies; 8+ messages in thread From: Dave Chinner @ 2013-09-18 3:13 UTC (permalink / raw) To: Ben Myers; +Cc: Al Viro, xfs On Tue, Sep 10, 2013 at 06:11:22PM -0500, Ben Myers wrote: > 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 <bpm@sgi.com> Looks good to me. Reviewed-by: Dave Chinner <dchinner@redhat.com> -- Dave Chinner david@fromorbit.com _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2] xfs: don't leak root inode reference 2013-09-10 23:11 ` Ben Myers 2013-09-18 3:13 ` Dave Chinner @ 2013-10-01 22:40 ` Ben Myers 1 sibling, 0 replies; 8+ messages in thread From: Ben Myers @ 2013-10-01 22:40 UTC (permalink / raw) To: Ben Myers; +Cc: xfs On Tue, Sep 10, 2013 at 06:11:22PM -0500, Ben Myers wrote: > Hi Dave, > > On Wed, Aug 28, 2013 at 08:08:33AM +1000, Dave Chinner wrote: > > On Tue, Aug 27, 2013 at 04:25:58PM -0500, Ben Myers wrote: > > > On Tue, Aug 27, 2013 at 07:24:23AM +1000, Dave Chinner wrote: > > > > On Mon, Aug 26, 2013 at 03:47:30PM -0500, Ben Myers wrote: > > > > > 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 <bpm@sgi.com> > > > > > > > > > > --- > > > > > fs/xfs/xfs_super.c | 6 ++++-- > > > > > 1 file changed, 4 insertions(+), 2 deletions(-) > > > > > > > > > > Index: b/fs/xfs/xfs_super.c > > > > > =================================================================== > > > > > --- a/fs/xfs/xfs_super.c 2013-08-26 15:36:09.170848579 -0500 > > > > > +++ b/fs/xfs/xfs_super.c 2013-08-26 15:40:19.450817933 -0500 > > > > > @@ -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; > > > > > } > > > > > > > > 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; > > > > } > > > > > > 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 <bpm@sgi.com> > > > > 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. > > 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 <bpm@sgi.com> Applied this one. _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2013-10-01 22:40 UTC | newest] Thread overview: 8+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2013-08-26 20:47 [PATCH] xfs: don't leak root inode reference Ben Myers 2013-08-26 21:03 ` Al Viro 2013-08-26 21:24 ` Dave Chinner 2013-08-27 21:25 ` [PATCH v2] " Ben Myers 2013-08-27 22:08 ` Dave Chinner 2013-09-10 23:11 ` Ben Myers 2013-09-18 3:13 ` Dave Chinner 2013-10-01 22:40 ` Ben Myers
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox