Message-ID: <20130214000730.GI26694@dastard>
Date:	Thu, 14 Feb 2013 11:07:30 +1100
From:	Dave Chinner <david@...morbit.com>
To:	Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Cc:	Paolo Bonzini <pbonzini@...hat.com>, linux-kernel@...r.kernel.org,
	stable@...r.kernel.org, Dave Chinner <dchinner@...hat.com>,
	Brian Foster <bfoster@...hat.com>, Ben Myers <bpm@....com>,
	CAI Qian <caiqian@...hat.com>, xfs@....sgi.com
Subject: Re: [ 68/89] xfs: fix _xfs_buf_find oops on blocks beyond the
 filesystem end

[cc xfs@....sgi.com]

On Wed, Feb 13, 2013 at 08:18:45AM -0800, Greg Kroah-Hartman wrote:
> On Wed, Feb 13, 2013 at 04:30:32PM +0100, Paolo Bonzini wrote:
> > Il 01/02/2013 14:08, Greg Kroah-Hartman ha scritto:
> > > 3.7-stable review patch.  If anyone has any objections, please let me know.
> > > 
> > > ------------------
> > > 
> > > From: Dave Chinner <dchinner@...hat.com>
> > > 
> > > commit eb178619f930fa2ba2348de332a1ff1c66a31424 upstream.
> > > 
> > > 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.
> > 
> > It looks like this breaks xfs_growfs.  See
> > http://bugzilla.redhat.com/show_bug.cgi?id=909602.

Entirely possible, as the filesystem size is not updated until after
all the new metadata is written to disk. in 3.8, there's this commit:

commit fd23683c3b1ab905cba61ea2981c156f4bf52845
Author: Dave Chinner <dchinner@...hat.com>
Date:   Mon Nov 12 22:53:59 2012 +1100

    xfs: growfs: use uncached buffers for new headers
    
    When writing the new AG headers to disk, we can't attach write
    verifiers because they have a dependency on the struct xfs-perag
    being attached to the buffer to be fully initialised and growfs
    can't fully initialise them until later in the process.
    
    The simplest way to avoid this problem is to use uncached buffers
    for writing the new headers. These buffers don't have the xfs-perag
    attached to them, so it's simple to detect in the write verifier and
    be able to skip the checks that need the xfs-perag.
    
    This enables us to attach the appropriate buffer ops to the buffer
    and henc calculate CRCs on the way to disk. IT also means that the
    buffer is torn down immediately, and so the first access to the AG
    headers will re-read the header from disk and perform full
    verification of the buffer. This way we also can catch corruptions
    due to problems that went undetected in growfs.
    
    Signed-off-by: Dave Chinner <dchinner@...hat.com>
    Reviewed-by Rich Johnston <rjohnston@....com>
    Signed-off-by: Ben Myers <bpm@....com>

As part of the metadata verifier feature. It means that growfs no
longer uses cached buffers, and hence does not pass through
_xfs_buf_find() and hence will not trigger the beyond-EOFS that the
above commit adds.

> Ick, not good.
> 
> Dave, any thoughts here?  Should I drop this from the 3.7-stable queue?

Yeah, drop it.

But what I'm now wondering is how this patch got proposed for
3.7-stable. I don't recall seeing anything about this being
proposed.

<trolls email archives>

Oh, it happened while I was at LCA and didn't have any access to Red
Hat email and there was a private thread about it. By the time I
read it the stable kernel was already released and so it immediately
dropped from my attention.

XFS Maintainers: Major process fail. Patches that are being proposed
for backports need to be posted to the XFS list, reviewed and tested
before saying they are OK to go.  We have several growfs tests in
xfstests would have failed if this was actually tested.

Stable folk: This is the reason why I, quite frankly, don't want to
support stable kernels *at all*. The overhead of backporting and
testing a patch to a single kernel target to ensure there are no
unintended regressions is significant, and there are so many stable
kernels no it's just a waste of developer time to try to support
them. And in this case, the process simply wasn't executed and an
unintended regression that is >this close< to causing filesystem
corruption slipped through to the stable series.....

Red Hat/Fedora folk: please report upstream XFS
bugs/regressions to xfs@....sgi.com or #xfs on freednode so we know
about them immediately and can triage problems quickly via email.
Sure, point us to the BZ you've raised, but tell us about the
problem ASAP. That way when users ask us about a problem, we are not
completely clueless...

Cheers,

Dave.
-- 
Dave Chinner
david@...morbit.com
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists