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@xxxxxxxxxx>
> >
> > 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.
> 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@xxxxxxxxxx>
> > ---
> > 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;
> > }
> >
> >
