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

Commit Message

Darrick J. Wong Feb. 29, 2020, 1:48 a.m. UTC

From: Darrick J. Wong <[e-mail]>

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.

Signed-off-by: Darrick J. Wong <[e-mail]>
---
 fs/xfs/libxfs/xfs_dir2_node.c |    2 ++
 1 file changed, 2 insertions(+)

Comments

Allison Henderson March 2, 2020, 6:11 p.m. UTC | #1

On 2/28/20 6:48 PM, Darrick J. Wong wrote:
> From: Darrick J. Wong <[e-mail]>
> 
> 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.
> 
> Signed-off-by: Darrick J. Wong <[e-mail]>
Looks good:
Reviewed-by: Allison Collins <[e-mail]>

> ---
>   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;
>   	}
>   
>

Eric Sandeen March 2, 2020, 11:54 p.m. UTC | #2

On 2/28/20 5:48 PM, Darrick J. Wong wrote:
> From: Darrick J. Wong <[e-mail]>
> 
> 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.

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?

> Signed-off-by: Darrick J. Wong <[e-mail]>
> ---
>  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;
>  	}
>  
>

Darrick J. Wong March 3, 2020, 4:38 p.m. UTC | #3

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 <[e-mail]>
> > 
> > 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 <[e-mail]>
> > ---
> >  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;
> >  	}
> >  
> >

Darrick J. Wong March 3, 2020, 5:43 p.m. UTC | #4

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 <[e-mail]>
> > > 
> > > 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 <[e-mail]>
> > > ---
> > >  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;
> > >  	}
> > >  
> > >

Darrick J. Wong March 3, 2020, 8:19 p.m. UTC | #5

On Fri, Feb 28, 2020 at 05:48:41PM -0800, Darrick J. Wong wrote:
> From: Darrick J. Wong <[e-mail]>
> 
> 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.
> 
> Signed-off-by: Darrick J. Wong <[e-mail]>
> ---
>  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);

Now that I've had time to think about this further, I conclude that this
call ought to be xfs_buf_corruption_error() since we created that
function to handle exactly this sort of thing...

> +		(*bpp)->b_flags &= ~XBF_DONE;

...and then we don't need this piece.

>  		xfs_trans_brelse(tp, *bpp);
> +		*bpp = NULL;

But we still need this because xfs_trans_brelse could have nuked *bpp
and we should never pass a (potentially stale and now reused) pointer up
to a caller, even if we are about to return an error code.

--D

>  		return -EFSCORRUPTED;
>  	}
>  
>

Dave Chinner March 3, 2020, 11:45 p.m. UTC | #6

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 <[e-mail]>
> > > 
> > > 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.

Christoph Hellwig March 5, 2020, 4:49 p.m. UTC | #7

On Fri, Feb 28, 2020 at 05:48:41PM -0800, Darrick J. Wong wrote:
> From: Darrick J. Wong <[e-mail]>
> 
> 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.
> 
> Signed-off-by: Darrick J. Wong <[e-mail]>

Looks good,

Reviewed-by: Christoph Hellwig <[e-mail]>

Darrick J. Wong March 5, 2020, 5:45 p.m. UTC | #8

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 <[e-mail]>
> > > > 
> > > > 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
> [e-mail]

Patch

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;
 	}