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

["Darrick J. Wong" <darrick.wong@oracle.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.
> 
> 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.

As an addendum to that, in the long run I will just fix it so that
_read_buf callers pass all the necessary context info through to the
verifiers and all of this will go away, but before that gets to RFC
status I need to iterate all the use cases that I can think of.

I /think/ all we need is an AG number, a XFS_HEALTH code, and some
combination of a (struct xfs_inode *) or an inode number to cover all
the cases of owner verification and automatic reporting of corruptions
to the health reporting subsystem.

--D

> > xfs_inode_buf_verify does, but for readahead purposes:
> > 
> >  * If the readahead buffer is invalid, we need to mark it with an error and
> >  * clear the DONE status of the buffer so that a followup read will re-read it
> >  * from disk.
> > 
> > Also, what problem does setting the pointer to NULL solve?
> 
> This avoids returning to the caller a pointer to an xfs_buf that we
> might have just released in xfs_trans_brelse.  The caller ought to bail
> out on the EFSCORRUPTED return value, but let's be defensive anyway. :)
> 
> --D
> 
> > > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > > ---
> > >  fs/xfs/libxfs/xfs_dir2_node.c |    2 ++
> > >  1 file changed, 2 insertions(+)
> > > 
> > > 
> > > diff --git a/fs/xfs/libxfs/xfs_dir2_node.c b/fs/xfs/libxfs/xfs_dir2_node.c
> > > index a0cc5e240306..f622ede7119e 100644
> > > --- a/fs/xfs/libxfs/xfs_dir2_node.c
> > > +++ b/fs/xfs/libxfs/xfs_dir2_node.c
> > > @@ -227,7 +227,9 @@ __xfs_dir3_free_read(
> > >  	fa = xfs_dir3_free_header_check(dp, fbno, *bpp);
> > >  	if (fa) {
> > >  		xfs_verifier_error(*bpp, -EFSCORRUPTED, fa);
> > > +		(*bpp)->b_flags &= ~XBF_DONE;
> > >  		xfs_trans_brelse(tp, *bpp);
> > > +		*bpp = NULL;
> > >  		return -EFSCORRUPTED;
> > >  	}
> > >  
> > >