Re: [PATCH 1/4] xfs: fix buffer state when we reject a corrupt dir free block

["Darrick J. Wong" <darrick.wong@oracle.com>]

On Wed, Mar 04, 2020 at 10:45:33AM +1100, Dave Chinner wrote:
> On Tue, Mar 03, 2020 at 08:38:53AM -0800, Darrick J. Wong wrote:
> > On Mon, Mar 02, 2020 at 05:54:07PM -0600, Eric Sandeen wrote:
> > > On 2/28/20 5:48 PM, Darrick J. Wong wrote:
> > > > From: Darrick J. Wong <darrick.wong@oracle.com>
> > > > 
> > > > Fix two problems in the dir3 free block read routine when we want to
> > > > reject a corrupt free block.  First, buffers should never have DONE set
> > > > at the same time that b_error is EFSCORRUPTED.  Second, don't leak a
> > > > pointer back to the caller.
> > > 
> > > For both of these things I'm left wondering; why does this particular
> > > location need to have XBF_DONE cleared after the verifier error?  Most
> > > other locations that mark errors don't do this.
> > 
> > Read verifier functions don't need to clear XBF_DONE because
> > xfs_buf_reverify will notice b_error being set, and clear XBF_DONE for
> > us.
> > 
> > __xfs_dir3_free_read calls _read_buf.  If the buffer read succeeds,
> > _free_read then has xfs_dir3_free_header_check do some more checking on
> > the buffer that we can't do in read verifiers.  This is *outside* the
> > regular read verifier (because we can't pass the owner into _read_buf)
> > so if we're going to use xfs_verifier_error() to set b_error then we
> > also have to clear XBF_DONE so that when we release the buffer a few
> > lines later the buffer will be in a state that the buffer code expects.
> 
> Actually, if the data in the buffer is bad after it has been
> successfully read and we want to make sure it never gets used, the
> buffer should be marked stale.
> 
> That will prevent the buffer from being placed on the LRU when it is
> released, and if a lookup finds it in cache it will clear /all/ the
> flags on it
> 
> xfs_da_read_buf() has read the buffer successfully, and set up it's
> state so that it is cached via insertion into the LRU on release. We
> want to make sure that nothing uses this buffer again without a
> complete re-initialisation, and that's effectively what
> xfs_buf_stale() does.
> 
> > This isn't theoretical, if the _header_check fails then we start
> > tripping the b_error assert the next time someone calls
> > xfs_buf_reverify.
> 
> We shouldn't be trying to re-use a corrupt buffer - it should cycle
> out of memory immediately. Clearing the XBF_DONE flag doesn't
> accomplish that; it works for buffer read verifier failures because
> that results in the buffer being released before they are configured
> to be cached on the LRU by the caller...
> 
> Indeed, xfs_buf_read_map() already stales the buffer on read and
> reverify failure....

I coded up making xfs_buf_corruption_error stale the buffer and it
didn't let out the magic smoke, so I'll add that to this series.

--D

> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> david@fromorbit.com