[ 21/89] xfs: Fix possible use-after-free with AIO

[ 21/89] xfs: Fix possible use-after-free with AIO

[Greg Kroah-Hartman]

3.7-stable review patch.  If anyone has any objections, please let me know.

------------------

From: Jan Kara <jack@suse.cz>

commit 4b05d09c18d9aa62d2e7fb4b057f54e5a38963f5 upstream.

Running AIO is pinning inode in memory using file reference. Once AIO
is completed using aio_complete(), file reference is put and inode can
be freed from memory. So we have to be sure that calling aio_complete()
is the last thing we do with the inode.

Signed-off-by: Jan Kara <jack@suse.cz>
CC: xfs@oss.sgi.com
CC: Ben Myers <bpm@sgi.com>
Reviewed-by: Ben Myers <bpm@sgi.com>
Signed-off-by: Ben Myers <bpm@sgi.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

---
 fs/xfs/xfs_aops.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

--- a/fs/xfs/xfs_aops.c
+++ b/fs/xfs/xfs_aops.c
@@ -86,11 +86,11 @@ xfs_destroy_ioend(
 	}
 
 	if (ioend->io_iocb) {
+		inode_dio_done(ioend->io_inode);
 		if (ioend->io_isasync) {
 			aio_complete(ioend->io_iocb, ioend->io_error ?
 					ioend->io_error : ioend->io_result, 0);
 		}
-		inode_dio_done(ioend->io_inode);
 	}
 
 	mempool_free(ioend, xfs_ioend_pool);


_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [ 68/89] xfs: fix _xfs_buf_find oops on blocks beyond the filesystem end

[Dave Chinner]

[cc xfs@oss.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@redhat.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@redhat.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@redhat.com>
    Reviewed-by Rich Johnston <rjohnston@sgi.com>
    Signed-off-by: Ben Myers <bpm@sgi.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@oss.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@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [ 68/89] xfs: fix _xfs_buf_find oops on blocks beyond the filesystem end

[Greg Kroah-Hartman]

On Thu, Feb 14, 2013 at 11:07:30AM +1100, Dave Chinner wrote:
> [cc xfs@oss.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@redhat.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@redhat.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@redhat.com>
>     Reviewed-by Rich Johnston <rjohnston@sgi.com>
>     Signed-off-by: Ben Myers <bpm@sgi.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.....

Ok, how about I never apply any xfs stable kernel patch, unless you send
it to stable@vger.kernel.org?  I have that rule in place for some other
subsystems that don't want me applying stuff that they aren't aware of,
and have no problem doing the same thing here.

Just let me know.

I'll go revert this patch for the next 3.7-stable release.

thanks,

greg k-h

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [ 68/89] xfs: fix _xfs_buf_find oops on blocks beyond the filesystem end

[Ben Myers]

Greg,

On Thu, Feb 14, 2013 at 11:26:14AM -0800, Greg Kroah-Hartman wrote:
> On Thu, Feb 14, 2013 at 11:07:30AM +1100, Dave Chinner wrote:
> > [cc xfs@oss.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@redhat.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@redhat.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@redhat.com>
> >     Reviewed-by Rich Johnston <rjohnston@sgi.com>
> >     Signed-off-by: Ben Myers <bpm@sgi.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.....
> 
> Ok, how about I never apply any xfs stable kernel patch, unless you send
> it to stable@vger.kernel.org?

Dave has made it clear that he doesn't want to be involved in maintaining
-stable kernels.  However, my team at SGI is interested in maintaining -stable
kernels.  We're not going to use the fact that there is a risk of regression as
an excuse to starve -stable of relevant fixes, just as we do not use it as an
excuse to starve the upstream branch of feature content.

> I have that rule in place for some other subsystems that don't want me
> applying stuff that they aren't aware of, and have no problem doing the same
> thing here.
> 
> Just let me know.

Here are the usual suspects:

Ben Myers <bpm@sgi.com>
Mark Tinguely <tinguely@sgi.com>
Dave Chinner <dchinner@redhat.com>
Eric Sandeen <sandeen@redhat.com>

> I'll go revert this patch for the next 3.7-stable release.

Much appreciated.

Regards,
	Ben

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [ 68/89] xfs: fix _xfs_buf_find oops on blocks beyond the filesystem end

[Greg Kroah-Hartman]

On Thu, Feb 14, 2013 at 01:55:12PM -0600, Ben Myers wrote:
> Greg,
> 
> On Thu, Feb 14, 2013 at 11:26:14AM -0800, Greg Kroah-Hartman wrote:
> > On Thu, Feb 14, 2013 at 11:07:30AM +1100, Dave Chinner wrote:
> > > [cc xfs@oss.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@redhat.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@redhat.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@redhat.com>
> > >     Reviewed-by Rich Johnston <rjohnston@sgi.com>
> > >     Signed-off-by: Ben Myers <bpm@sgi.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.....
> > 
> > Ok, how about I never apply any xfs stable kernel patch, unless you send
> > it to stable@vger.kernel.org?
> 
> Dave has made it clear that he doesn't want to be involved in maintaining
> -stable kernels.  However, my team at SGI is interested in maintaining -stable
> kernels.  We're not going to use the fact that there is a risk of regression as
> an excuse to starve -stable of relevant fixes, just as we do not use it as an
> excuse to starve the upstream branch of feature content.
> 
> > I have that rule in place for some other subsystems that don't want me
> > applying stuff that they aren't aware of, and have no problem doing the same
> > thing here.
> > 
> > Just let me know.
> 
> Here are the usual suspects:
> 
> Ben Myers <bpm@sgi.com>
> Mark Tinguely <tinguely@sgi.com>
> Dave Chinner <dchinner@redhat.com>
> Eric Sandeen <sandeen@redhat.com>

Ok, but for this specific patch, did I do something wrong in taking it?

I guess I'll just let you send me xfs patches, is that ok with everyone
else?  Dave can just ignore them, especially given redhat's horrible
email system :)

thanks,

greg k-h

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [ 68/89] xfs: fix _xfs_buf_find oops on blocks beyond the filesystem end

[Ben Myers]

Hey Greg,

On Thu, Feb 14, 2013 at 12:05:01PM -0800, Greg Kroah-Hartman wrote:
> On Thu, Feb 14, 2013 at 01:55:12PM -0600, Ben Myers wrote:
> > Greg,
> > 
> > On Thu, Feb 14, 2013 at 11:26:14AM -0800, Greg Kroah-Hartman wrote:
> > > On Thu, Feb 14, 2013 at 11:07:30AM +1100, Dave Chinner wrote:
> > > > [cc xfs@oss.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@redhat.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@redhat.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@redhat.com>
> > > >     Reviewed-by Rich Johnston <rjohnston@sgi.com>
> > > >     Signed-off-by: Ben Myers <bpm@sgi.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.....
> > > 
> > > Ok, how about I never apply any xfs stable kernel patch, unless you send
> > > it to stable@vger.kernel.org?
> > 
> > Dave has made it clear that he doesn't want to be involved in maintaining
> > -stable kernels.  However, my team at SGI is interested in maintaining -stable
> > kernels.  We're not going to use the fact that there is a risk of regression as
> > an excuse to starve -stable of relevant fixes, just as we do not use it as an
> > excuse to starve the upstream branch of feature content.
> > 
> > > I have that rule in place for some other subsystems that don't want me
> > > applying stuff that they aren't aware of, and have no problem doing the same
> > > thing here.
> > > 
> > > Just let me know.
> > 
> > Here are the usual suspects:
> > 
> > Ben Myers <bpm@sgi.com>
> > Mark Tinguely <tinguely@sgi.com>
> > Dave Chinner <dchinner@redhat.com>
> > Eric Sandeen <sandeen@redhat.com>
> 
> Ok, but for this specific patch, did I do something wrong in taking it?

No, not in my opinion.  I was on the CC and had the opportunity to NACK it and
failed to do so.  So today I'm eating crow.

> I guess I'll just let you send me xfs patches, is that ok with everyone
> else?

For my part, I trust any of the gentlemen I listed above to do adequate testing
before proposing xfs patches for -stable.  There are more xfs geeks who fit
into that category (and I prefer not to exclude), but that's my suggestion for
now.

> Dave can just ignore them, especially given redhat's horrible
> email system :)

Lol.  I think RH will be purchasing a smart phone soon.

Thanks,
	Ben

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [ 68/89] xfs: fix _xfs_buf_find oops on blocks beyond the filesystem end

[Dave Chinner]

On Thu, Feb 14, 2013 at 12:05:01PM -0800, Greg Kroah-Hartman wrote:
> On Thu, Feb 14, 2013 at 01:55:12PM -0600, Ben Myers wrote:
> > > Ok, how about I never apply any xfs stable kernel patch, unless you send
> > > it to stable@vger.kernel.org?
> > 
> > Dave has made it clear that he doesn't want to be involved in maintaining
> > -stable kernels.

I don't think you quite understand, Ben. It is obvious from the fact
that this discussion is taking place that I'm extremely concerned
about the quality of -stable kernels and what goes into them. What I
think you are missing is that there is a difference between not
having the time to do the grunt work of backporting and testing
-stable kernels versus wanting to ensure the quality of the
maintenance work that does take place remains high.

Why do I say this? Because the responsiblity of maintaining XFS
development is not SGI's. It is a community effort, and I am as much
responsible as anyone else. Someone else doing a half-arsed job
maintaining XFS in -stable kernels reflects badly on *me* as being a
senior member of a community that allows it's members to do a
half-arsed job on something.

> > However, my team at SGI is interested in maintaining -stable
> > kernels.

Then do the job properly. Being "interested" isn't enough - you need
the *commitment* to ensure things are done properly.  If you
*personally* don't have the the time to ensure that the -stable
kernel maintenance is done properly, then don't do it at all.

> > We're not going to use the fact that there is a risk of regression as
> > an excuse to starve -stable of relevant fixes, just as we do not use it as an
> > excuse to starve the upstream branch of feature content.

I'm not complaining because there is a risk of regression in
-stable backports, I'm pointing out that our long-standing process
used to minimise that risk was not followed.

Besides, -stable backports are all about risk management.  The
primary consideration for a backport is whether the risk of
regressions is higher than the risk of leaving the bug unfixed.
We've NACKed lots of proposed patches for -stable simply because the
risk of regression outweighes the benefit to users of -stable
inclusion.

Yes, there is always a risk of unintended regressions, but you have
to do *due diligence* on the backports to -stable trees to minimise
the risk factor. Even the most basic "apply the patch, run xfstests"
check would have found this regression. So, even if we ignore the
risk factors here, a *preventable regression* occurred because due
diligence was not performed correctly on the requested patch.

BTW, Ben, I should point out that 6 months ago you said exactly the
opposite to this statement - you tried to use "risk of regressions"
as an excuse to starve the dev tree of new feature content. i.e. you
wanted to apply -stable tree criteria to the -dev tree. Now you are
saying that risk of regression is not a reason for rejection for the
-stable tree. (i.e. applying -dev tree criteria to the -stable
tree).  IMO, you are as wrong about the -stable tree now as you were
about the -dev tree 6 months ago....

> > > I have that rule in place for some other subsystems that don't want me
> > > applying stuff that they aren't aware of, and have no problem doing the same
> > > thing here.
> > > 
> > > Just let me know.

Sounds like a fine idea, Greg.

> > Here are the usual suspects:
> > 
> > Ben Myers <bpm@sgi.com>
> > Mark Tinguely <tinguely@sgi.com>
> > Dave Chinner <dchinner@redhat.com>
> > Eric Sandeen <sandeen@redhat.com>

I don't think it should be restricted to individuals.  The private
thread used to request this backport is exactly why I didn't see
the request in a timely fashion, and also the reason why we didn't
end up with notifications for review going to xfs@oss.sgi.com.

Hence I'd suggest the only thing that matters is that there is a cc
to xfs@oss.sgi.com, because that means all of the above people (and
more) are on that list and hence have the best chance to see and
review the backport request.

> Ok, but for this specific patch, did I do something wrong in taking it?

No, you didn't do anything wrong, Greg. Stuff went wrong on the XFS
side of the fence.

> I guess I'll just let you send me xfs patches, is that ok with everyone
> else? 

Sure, that would work, but only after the patches have been sent to
xfs@oss.sgi.com for review and testing and been acked. And, of
course, the stable submission woul dalso need to have a
xfs@oss.sgi.com cc on it. ;)

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [ 68/89] xfs: fix _xfs_buf_find oops on blocks beyond the filesystem end

[Ben Myers]

Hey,

On Fri, Feb 15, 2013 at 12:47:29PM +1100, Dave Chinner wrote:
> On Thu, Feb 14, 2013 at 12:05:01PM -0800, Greg Kroah-Hartman wrote:
> > On Thu, Feb 14, 2013 at 01:55:12PM -0600, Ben Myers wrote:
> > > > Ok, how about I never apply any xfs stable kernel patch, unless you send
> > > > it to stable@vger.kernel.org?
> > > 
> > > Dave has made it clear that he doesn't want to be involved in maintaining
> > > -stable kernels.
> 
> I don't think you quite understand, Ben.

...

> > > > I have that rule in place for some other subsystems that don't want me
> > > > applying stuff that they aren't aware of, and have no problem doing the same
> > > > thing here.
> > > > 
> > > > Just let me know.
> 
> Sounds like a fine idea, Greg.
> 
> > > Here are the usual suspects:
> > > 
> > > Ben Myers <bpm@sgi.com>
> > > Mark Tinguely <tinguely@sgi.com>
> > > Dave Chinner <dchinner@redhat.com>
> > > Eric Sandeen <sandeen@redhat.com>
> 
> I don't think it should be restricted to individuals.  The private
> thread used to request this backport is exactly why I didn't see
> the request in a timely fashion, and also the reason why we didn't
> end up with notifications for review going to xfs@oss.sgi.com.
> 
> Hence I'd suggest the only thing that matters is that there is a cc
> to xfs@oss.sgi.com, because that means all of the above people (and
> more) are on that list and hence have the best chance to see and
> review the backport request.
> 
> > Ok, but for this specific patch, did I do something wrong in taking it?
> 
> No, you didn't do anything wrong, Greg. Stuff went wrong on the XFS
> side of the fence.
> 
> > I guess I'll just let you send me xfs patches, is that ok with everyone
> > else? 
> 
> Sure, that would work, but only after the patches have been sent to
> xfs@oss.sgi.com for review and testing and been acked. And, of
> course, the stable submission woul dalso need to have a
> xfs@oss.sgi.com cc on it. ;)

Making sure that xfs@oss.sgi.com is Cc'd on -stable patches seems reasonable to
me.  No objection here, Dave.

-Ben

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs