xfs
[Top] [All Lists]

Re: [PATCH] xfs: shutdown filesystem if xfs_perag_get fails

To: Mark Tinguely <tinguely@xxxxxxx>
Subject: Re: [PATCH] xfs: shutdown filesystem if xfs_perag_get fails
From: Chandra Seetharaman <sekharan@xxxxxxxxxx>
Date: Tue, 23 Apr 2013 10:54:35 -0500
Cc: Dave Chinner <david@xxxxxxxxxxxxx>, Eric Sandeen <sandeen@xxxxxxxxxxx>, xfs@xxxxxxxxxxx
Delivered-to: xfs@xxxxxxxxxxx
In-reply-to: <51769111.3050103@xxxxxxx>
Organization: IBM
References: <20130419204102.736961610@xxxxxxx> <20130421174107.007313126@xxxxxxx> <5174603A.8030208@xxxxxxxxxxx> <51753EDE.6000301@xxxxxxx> <51754A13.5000808@xxxxxxxxxxx> <5175532B.3050509@xxxxxxx> <20130422233033.GK30622@dastard> <51769111.3050103@xxxxxxx>
Reply-to: sekharan@xxxxxxxxxx
On Tue, 2013-04-23 at 08:48 -0500, Mark Tinguely wrote:
> On 04/22/13 18:30, Dave Chinner wrote:
> > On Mon, Apr 22, 2013 at 10:11:39AM -0500, Mark Tinguely wrote:
> >>   #6 [ffff880135603980] _xfs_buf_find at ffffffffa01a7fef [xfs]
> >>   #7 [ffff8801356039f0] xfs_buf_get at ffffffffa01a824a [xfs]
> >>   #8 [ffff880135603a30] xfs_buf_read at ffffffffa01a83a4 [xfs]
> >>   #9 [ffff880135603a60] xlog_recover_inode_pass2 at ffffffffa0193629 [xfs]
> >
> > So it's the same problem as this bug fix addresses:
> >
> > commit 10616b806d1d7835b1d23b8d75ef638f92cb98b6
> > Author: Dave Chinner<dchinner@xxxxxxxxxx>
> > Date:   Mon Jan 21 23:53:52 2013 +1100
> >
> >      xfs: fix _xfs_buf_find oops on blocks beyond the filesystem end
> >
> >      When _xfs_buf_find is passed an out of range address, it will fail
> >      to find a relevant struct xfs_perag and oops with a null
> >      dereference. This can happen when trying to walk a filesystem with a
> >      metadata inode that has a partially corrupted extent map (i.e. the
> >      block number returned is corrupt, but is otherwise intact) and we
> >      try to read from the corrupted block address.
> >
> >      In this case, just fail the lookup. If it is readahead being issued,
> >      it will simply not be done, but if it is real read that fails we
> >      will get an error being reported.  Ideally this case should result
> >      in an EFSCORRUPTED error being reported, but we cannot return an
> >      error through xfs_buf_read() or xfs_buf_get() so this lookup failure
> >      may result in ENOMEM or EIO errors being reported instead.
> >
> >      Signed-off-by: Dave Chinner<dchinner@xxxxxxxxxx>
> >      Reviewed-by: Brian Foster<bfoster@xxxxxxxxxx>
> >      Reviewed-by: Ben Myers<bpm@xxxxxxx>
> >      Signed-off-by: Ben Myers<bpm@xxxxxxx>
> >
> >> The recovery value is bad and is a problem on its own, but XFS does
> >> not verify the validity of ag number when doing a xfs_perag_get().
> >
> > Right, that's what the above fix does, but it can't be done on older
> > kernels because grwofs relies on being able to get buffers beyond
> > the existing filesystem limits...
> >
> > Cheers,
> >
> > Dave.
> 
> Thank-you, that make sense.
> 
> I still do not like assuming xfs_perag_get() will always return a valid 
> perag pointer.

I second that.

Is there any reason we should _not_ check the return value from
xfs_perag_get() for NULL ?
> 
> --Mark.
> 
> _______________________________________________
> xfs mailing list
> xfs@xxxxxxxxxxx
> http://oss.sgi.com/mailman/listinfo/xfs
> 


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