xfs
[Top] [All Lists]

Re: [PATCH v2] xfs: don't leak root inode reference

To: Ben Myers <bpm@xxxxxxx>
Subject: Re: [PATCH v2] xfs: don't leak root inode reference
From: Dave Chinner <david@xxxxxxxxxxxxx>
Date: Wed, 28 Aug 2013 08:08:33 +1000
Cc: Al Viro <viro@xxxxxxxxxxxxxxxxxx>, xfs@xxxxxxxxxxx
Delivered-to: xfs@xxxxxxxxxxx
In-reply-to: <20130827212558.GY5262@xxxxxxx>
References: <20130826204730.GZ7153@xxxxxxx> <20130826212423.GX6023@dastard> <20130827212558.GY5262@xxxxxxx>
User-agent: Mutt/1.5.21 (2010-09-15)
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@xxxxxxx>
> > > 
> > > ---
> > >  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@xxxxxxx>

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@xxxxxxxxxxxxx

<Prev in Thread] Current Thread [Next in Thread>