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

[Dave Chinner <david@fromorbit.com>]

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....

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com