[PATCH 0/4] xfs: fix errors in directory verifiers

[PATCH 0/4] xfs: fix errors in directory verifiers

[Darrick J. Wong]

Hi all,

During a code audit of the effectiveness of the new online directory
repair code, it was discovered that XFS does not check the owner fields
of the dir3 free, data, and block format metadata blocks.  It was
further discovered that when the dir3 free block verifier rejects a
block, it does not leave the incore buffer in the correct state.
Correct these problems.

If you're going to start using this mess, you probably ought to just
pull from my git trees, which are linked below.

This is an extraordinary way to destroy everything.  Enjoy!
Comments and questions are, as always, welcome.

--D

kernel git tree:
https://git.kernel.org/cgit/linux/kernel/git/djwong/xfs-linux.git/log/?h=verifier-fixes

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

[Darrick J. Wong]

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.

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

[PATCH 2/4] xfs: check owner of dir3 free blocks

[Darrick J. Wong]

From: Darrick J. Wong <darrick.wong@oracle.com>

Check the owner field of dir3 free block headers.

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 f622ede7119e..79b917e62ac3 100644
--- a/fs/xfs/libxfs/xfs_dir2_node.c
+++ b/fs/xfs/libxfs/xfs_dir2_node.c
@@ -194,6 +194,8 @@ xfs_dir3_free_header_check(
 			return __this_address;
 		if (be32_to_cpu(hdr3->nvalid) < be32_to_cpu(hdr3->nused))
 			return __this_address;
+		if (be64_to_cpu(hdr3->hdr.owner) != dp->i_ino)
+			return __this_address;
 	} else {
 		struct xfs_dir2_free_hdr *hdr = bp->b_addr;
 

[PATCH 3/4] xfs: check owner of dir3 data blocks

[Darrick J. Wong]

From: Darrick J. Wong <darrick.wong@oracle.com>

Check the owner field of dir3 data block headers.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 fs/xfs/libxfs/xfs_dir2_data.c |   32 +++++++++++++++++++++++++++++++-
 1 file changed, 31 insertions(+), 1 deletion(-)


diff --git a/fs/xfs/libxfs/xfs_dir2_data.c b/fs/xfs/libxfs/xfs_dir2_data.c
index b9eba8213180..e5910bc9ab83 100644
--- a/fs/xfs/libxfs/xfs_dir2_data.c
+++ b/fs/xfs/libxfs/xfs_dir2_data.c
@@ -394,6 +394,22 @@ static const struct xfs_buf_ops xfs_dir3_data_reada_buf_ops = {
 	.verify_write = xfs_dir3_data_write_verify,
 };
 
+static xfs_failaddr_t
+xfs_dir3_data_header_check(
+	struct xfs_inode	*dp,
+	struct xfs_buf		*bp)
+{
+	struct xfs_mount	*mp = dp->i_mount;
+
+	if (xfs_sb_version_hascrc(&mp->m_sb)) {
+		struct xfs_dir3_data_hdr *hdr3 = bp->b_addr;
+
+		if (be64_to_cpu(hdr3->hdr.owner) != dp->i_ino)
+			return __this_address;
+	}
+
+	return NULL;
+}
 
 int
 xfs_dir3_data_read(
@@ -403,11 +419,25 @@ xfs_dir3_data_read(
 	unsigned int		flags,
 	struct xfs_buf		**bpp)
 {
+	xfs_failaddr_t		fa;
 	int			err;
 
 	err = xfs_da_read_buf(tp, dp, bno, flags, bpp, XFS_DATA_FORK,
 			&xfs_dir3_data_buf_ops);
-	if (!err && tp && *bpp)
+	if (err || !*bpp)
+		return err;
+
+	/* Check things that we can't do in the verifier. */
+	fa = xfs_dir3_data_header_check(dp, *bpp);
+	if (fa) {
+		xfs_verifier_error(*bpp, -EFSCORRUPTED, fa);
+		(*bpp)->b_flags &= ~XBF_DONE;
+		xfs_trans_brelse(tp, *bpp);
+		*bpp = NULL;
+		return -EFSCORRUPTED;
+	}
+
+	if (tp)
 		xfs_trans_buf_set_type(tp, *bpp, XFS_BLFT_DIR_DATA_BUF);
 	return err;
 }

[PATCH 4/4] xfs: check owner of dir3 blocks

[Darrick J. Wong]

From: Darrick J. Wong <darrick.wong@oracle.com>

Check the owner field of dir3 block headers.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 fs/xfs/libxfs/xfs_dir2_block.c |   34 +++++++++++++++++++++++++++++++++-
 1 file changed, 33 insertions(+), 1 deletion(-)


diff --git a/fs/xfs/libxfs/xfs_dir2_block.c b/fs/xfs/libxfs/xfs_dir2_block.c
index d6ced59b9567..408a9d7c8c24 100644
--- a/fs/xfs/libxfs/xfs_dir2_block.c
+++ b/fs/xfs/libxfs/xfs_dir2_block.c
@@ -20,6 +20,7 @@
 #include "xfs_error.h"
 #include "xfs_trace.h"
 #include "xfs_log.h"
+#include "xfs_health.h"
 
 /*
  * Local function prototypes.
@@ -114,6 +115,23 @@ const struct xfs_buf_ops xfs_dir3_block_buf_ops = {
 	.verify_struct = xfs_dir3_block_verify,
 };
 
+static xfs_failaddr_t
+xfs_dir3_block_header_check(
+	struct xfs_inode	*dp,
+	struct xfs_buf		*bp)
+{
+	struct xfs_mount	*mp = dp->i_mount;
+
+	if (xfs_sb_version_hascrc(&mp->m_sb)) {
+		struct xfs_dir3_blk_hdr *hdr3 = bp->b_addr;
+
+		if (be64_to_cpu(hdr3->owner) != dp->i_ino)
+			return __this_address;
+	}
+
+	return NULL;
+}
+
 int
 xfs_dir3_block_read(
 	struct xfs_trans	*tp,
@@ -121,11 +139,25 @@ xfs_dir3_block_read(
 	struct xfs_buf		**bpp)
 {
 	struct xfs_mount	*mp = dp->i_mount;
+	xfs_failaddr_t		fa;
 	int			err;
 
 	err = xfs_da_read_buf(tp, dp, mp->m_dir_geo->datablk, 0, bpp,
 				XFS_DATA_FORK, &xfs_dir3_block_buf_ops);
-	if (!err && tp && *bpp)
+	if (err || !*bpp)
+		return err;
+
+	/* Check things that we can't do in the verifier. */
+	fa = xfs_dir3_block_header_check(dp, *bpp);
+	if (fa) {
+		xfs_verifier_error(*bpp, -EFSCORRUPTED, fa);
+		(*bpp)->b_flags &= ~XBF_DONE;
+		xfs_trans_brelse(tp, *bpp);
+		*bpp = NULL;
+		return -EFSCORRUPTED;
+	}
+
+	if (tp)
 		xfs_trans_buf_set_type(tp, *bpp, XFS_BLFT_DIR_BLOCK_BUF);
 	return err;
 }

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

[Allison Collins]

On 2/28/20 6: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.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
Looks good:
Reviewed-by: Allison Collins <allison.henderson@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;
>   	}
>   
> 

Re: [PATCH 2/4] xfs: check owner of dir3 free blocks

[Allison Collins]

On 2/28/20 6:48 PM, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Check the owner field of dir3 free block headers.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
Ok, looks fine:
Reviewed-by: Allison Collins <allison.henderson@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 f622ede7119e..79b917e62ac3 100644
> --- a/fs/xfs/libxfs/xfs_dir2_node.c
> +++ b/fs/xfs/libxfs/xfs_dir2_node.c
> @@ -194,6 +194,8 @@ xfs_dir3_free_header_check(
>   			return __this_address;
>   		if (be32_to_cpu(hdr3->nvalid) < be32_to_cpu(hdr3->nused))
>   			return __this_address;
> +		if (be64_to_cpu(hdr3->hdr.owner) != dp->i_ino)
> +			return __this_address;
>   	} else {
>   		struct xfs_dir2_free_hdr *hdr = bp->b_addr;
>   
> 

Re: [PATCH 3/4] xfs: check owner of dir3 data blocks

[Allison Collins]

On 2/28/20 6:48 PM, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Check the owner field of dir3 data block headers.
"Check the owner field of dir3 data block headers, and release the 
buffer on error." ?

It's a bit of an api change though isnt it?  Do we need to go find all 
the callers and make sure there's not going to be a double release if 
error == -EFSCORRUPTED ?

Allison

> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
>   fs/xfs/libxfs/xfs_dir2_data.c |   32 +++++++++++++++++++++++++++++++-
>   1 file changed, 31 insertions(+), 1 deletion(-)
> 
> 
> diff --git a/fs/xfs/libxfs/xfs_dir2_data.c b/fs/xfs/libxfs/xfs_dir2_data.c
> index b9eba8213180..e5910bc9ab83 100644
> --- a/fs/xfs/libxfs/xfs_dir2_data.c
> +++ b/fs/xfs/libxfs/xfs_dir2_data.c
> @@ -394,6 +394,22 @@ static const struct xfs_buf_ops xfs_dir3_data_reada_buf_ops = {
>   	.verify_write = xfs_dir3_data_write_verify,
>   };
>   
> +static xfs_failaddr_t
> +xfs_dir3_data_header_check(
> +	struct xfs_inode	*dp,
> +	struct xfs_buf		*bp)
> +{
> +	struct xfs_mount	*mp = dp->i_mount;
> +
> +	if (xfs_sb_version_hascrc(&mp->m_sb)) {
> +		struct xfs_dir3_data_hdr *hdr3 = bp->b_addr;
> +
> +		if (be64_to_cpu(hdr3->hdr.owner) != dp->i_ino)
> +			return __this_address;
> +	}
> +
> +	return NULL;
> +}
>   
>   int
>   xfs_dir3_data_read(
> @@ -403,11 +419,25 @@ xfs_dir3_data_read(
>   	unsigned int		flags,
>   	struct xfs_buf		**bpp)
>   {
> +	xfs_failaddr_t		fa;
>   	int			err;
>   
>   	err = xfs_da_read_buf(tp, dp, bno, flags, bpp, XFS_DATA_FORK,
>   			&xfs_dir3_data_buf_ops);
> -	if (!err && tp && *bpp)
> +	if (err || !*bpp)
> +		return err;
> +
> +	/* Check things that we can't do in the verifier. */
> +	fa = xfs_dir3_data_header_check(dp, *bpp);
> +	if (fa) {
> +		xfs_verifier_error(*bpp, -EFSCORRUPTED, fa);
> +		(*bpp)->b_flags &= ~XBF_DONE;
> +		xfs_trans_brelse(tp, *bpp);
> +		*bpp = NULL;
> +		return -EFSCORRUPTED;
> +	}
> +
> +	if (tp)
>   		xfs_trans_buf_set_type(tp, *bpp, XFS_BLFT_DIR_DATA_BUF);
>   	return err;
>   }
> 

Re: [PATCH 4/4] xfs: check owner of dir3 blocks

[Allison Collins]

On 2/28/20 6:49 PM, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Check the owner field of dir3 block headers.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
Same question here with the release buffer api change.

Allison
> ---
>   fs/xfs/libxfs/xfs_dir2_block.c |   34 +++++++++++++++++++++++++++++++++-
>   1 file changed, 33 insertions(+), 1 deletion(-)
> 
> 
> diff --git a/fs/xfs/libxfs/xfs_dir2_block.c b/fs/xfs/libxfs/xfs_dir2_block.c
> index d6ced59b9567..408a9d7c8c24 100644
> --- a/fs/xfs/libxfs/xfs_dir2_block.c
> +++ b/fs/xfs/libxfs/xfs_dir2_block.c
> @@ -20,6 +20,7 @@
>   #include "xfs_error.h"
>   #include "xfs_trace.h"
>   #include "xfs_log.h"
> +#include "xfs_health.h"
>   
>   /*
>    * Local function prototypes.
> @@ -114,6 +115,23 @@ const struct xfs_buf_ops xfs_dir3_block_buf_ops = {
>   	.verify_struct = xfs_dir3_block_verify,
>   };
>   
> +static xfs_failaddr_t
> +xfs_dir3_block_header_check(
> +	struct xfs_inode	*dp,
> +	struct xfs_buf		*bp)
> +{
> +	struct xfs_mount	*mp = dp->i_mount;
> +
> +	if (xfs_sb_version_hascrc(&mp->m_sb)) {
> +		struct xfs_dir3_blk_hdr *hdr3 = bp->b_addr;
> +
> +		if (be64_to_cpu(hdr3->owner) != dp->i_ino)
> +			return __this_address;
> +	}
> +
> +	return NULL;
> +}
> +
>   int
>   xfs_dir3_block_read(
>   	struct xfs_trans	*tp,
> @@ -121,11 +139,25 @@ xfs_dir3_block_read(
>   	struct xfs_buf		**bpp)
>   {
>   	struct xfs_mount	*mp = dp->i_mount;
> +	xfs_failaddr_t		fa;
>   	int			err;
>   
>   	err = xfs_da_read_buf(tp, dp, mp->m_dir_geo->datablk, 0, bpp,
>   				XFS_DATA_FORK, &xfs_dir3_block_buf_ops);
> -	if (!err && tp && *bpp)
> +	if (err || !*bpp)
> +		return err;
> +
> +	/* Check things that we can't do in the verifier. */
> +	fa = xfs_dir3_block_header_check(dp, *bpp);
> +	if (fa) {
> +		xfs_verifier_error(*bpp, -EFSCORRUPTED, fa);
> +		(*bpp)->b_flags &= ~XBF_DONE;
> +		xfs_trans_brelse(tp, *bpp);
> +		*bpp = NULL;
> +		return -EFSCORRUPTED;
> +	}
> +
> +	if (tp)
>   		xfs_trans_buf_set_type(tp, *bpp, XFS_BLFT_DIR_BLOCK_BUF);
>   	return err;
>   }
> 

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

[Eric Sandeen]

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.

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

Re: [PATCH 2/4] xfs: check owner of dir3 free blocks

[Eric Sandeen]

On 2/28/20 5:48 PM, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Check the owner field of dir3 free block headers.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>

Do we need a similar check in xfs_repair?

Should this also check

hdr.blkno == bp->b_bn? ?

> ---
>  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 f622ede7119e..79b917e62ac3 100644
> --- a/fs/xfs/libxfs/xfs_dir2_node.c
> +++ b/fs/xfs/libxfs/xfs_dir2_node.c
> @@ -194,6 +194,8 @@ xfs_dir3_free_header_check(
>  			return __this_address;
>  		if (be32_to_cpu(hdr3->nvalid) < be32_to_cpu(hdr3->nused))
>  			return __this_address;
> +		if (be64_to_cpu(hdr3->hdr.owner) != dp->i_ino)
> +			return __this_address;
>  	} else {
>  		struct xfs_dir2_free_hdr *hdr = bp->b_addr;
>  
> 

Re: [PATCH 2/4] xfs: check owner of dir3 free blocks

[Darrick J. Wong]

On Mon, Mar 02, 2020 at 06:04:07PM -0600, Eric Sandeen wrote:
> On 2/28/20 5:48 PM, Darrick J. Wong wrote:
> > From: Darrick J. Wong <darrick.wong@oracle.com>
> > 
> > Check the owner field of dir3 free block headers.
> > 
> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Do we need a similar check in xfs_repair?

Yeah, working on that...

> Should this also check
> 
> hdr.blkno == bp->b_bn? ?

That's checked by xfs_dir3_free_verify.

--D

> > ---
> >  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 f622ede7119e..79b917e62ac3 100644
> > --- a/fs/xfs/libxfs/xfs_dir2_node.c
> > +++ b/fs/xfs/libxfs/xfs_dir2_node.c
> > @@ -194,6 +194,8 @@ xfs_dir3_free_header_check(
> >  			return __this_address;
> >  		if (be32_to_cpu(hdr3->nvalid) < be32_to_cpu(hdr3->nused))
> >  			return __this_address;
> > +		if (be64_to_cpu(hdr3->hdr.owner) != dp->i_ino)
> > +			return __this_address;
> >  	} else {
> >  		struct xfs_dir2_free_hdr *hdr = bp->b_addr;
> >  
> > 

Re: [PATCH 2/4] xfs: check owner of dir3 free blocks

[Eric Sandeen]

On 3/3/20 8:08 AM, Darrick J. Wong wrote:
> On Mon, Mar 02, 2020 at 06:04:07PM -0600, Eric Sandeen wrote:
>> On 2/28/20 5:48 PM, Darrick J. Wong wrote:
>>> From: Darrick J. Wong <darrick.wong@oracle.com>
>>>
>>> Check the owner field of dir3 free block headers.
>>>
>>> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
>>
>> Do we need a similar check in xfs_repair?
> 
> Yeah, working on that...


k

>> Should this also check
>>
>> hdr.blkno == bp->b_bn? ?
> 
> That's checked by xfs_dir3_free_verify.

Oh, right.  sorry.

-Eric

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

[Darrick J. Wong]

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.

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

Re: [PATCH 3/4] xfs: check owner of dir3 data blocks

[Darrick J. Wong]

On Mon, Mar 02, 2020 at 11:12:17AM -0700, Allison Collins wrote:
> On 2/28/20 6:48 PM, Darrick J. Wong wrote:
> > From: Darrick J. Wong <darrick.wong@oracle.com>
> > 
> > Check the owner field of dir3 data block headers.
> "Check the owner field of dir3 data block headers, and release the buffer on
> error." ?
> 
> It's a bit of an api change though isnt it?  Do we need to go find all the
> callers and make sure there's not going to be a double release if error ==
> -EFSCORRUPTED ?

There shouldn't be, since we set *bpp to NULL before returning
EFSCORRUPTED.  The callers all seemed to handle nonzero return and/or
null bp properly.

(I dunno, did I miss one?  It's entirely likely... :))

--D

> Allison
> 
> > 
> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > ---
> >   fs/xfs/libxfs/xfs_dir2_data.c |   32 +++++++++++++++++++++++++++++++-
> >   1 file changed, 31 insertions(+), 1 deletion(-)
> > 
> > 
> > diff --git a/fs/xfs/libxfs/xfs_dir2_data.c b/fs/xfs/libxfs/xfs_dir2_data.c
> > index b9eba8213180..e5910bc9ab83 100644
> > --- a/fs/xfs/libxfs/xfs_dir2_data.c
> > +++ b/fs/xfs/libxfs/xfs_dir2_data.c
> > @@ -394,6 +394,22 @@ static const struct xfs_buf_ops xfs_dir3_data_reada_buf_ops = {
> >   	.verify_write = xfs_dir3_data_write_verify,
> >   };
> > +static xfs_failaddr_t
> > +xfs_dir3_data_header_check(
> > +	struct xfs_inode	*dp,
> > +	struct xfs_buf		*bp)
> > +{
> > +	struct xfs_mount	*mp = dp->i_mount;
> > +
> > +	if (xfs_sb_version_hascrc(&mp->m_sb)) {
> > +		struct xfs_dir3_data_hdr *hdr3 = bp->b_addr;
> > +
> > +		if (be64_to_cpu(hdr3->hdr.owner) != dp->i_ino)
> > +			return __this_address;
> > +	}
> > +
> > +	return NULL;
> > +}
> >   int
> >   xfs_dir3_data_read(
> > @@ -403,11 +419,25 @@ xfs_dir3_data_read(
> >   	unsigned int		flags,
> >   	struct xfs_buf		**bpp)
> >   {
> > +	xfs_failaddr_t		fa;
> >   	int			err;
> >   	err = xfs_da_read_buf(tp, dp, bno, flags, bpp, XFS_DATA_FORK,
> >   			&xfs_dir3_data_buf_ops);
> > -	if (!err && tp && *bpp)
> > +	if (err || !*bpp)
> > +		return err;
> > +
> > +	/* Check things that we can't do in the verifier. */
> > +	fa = xfs_dir3_data_header_check(dp, *bpp);
> > +	if (fa) {
> > +		xfs_verifier_error(*bpp, -EFSCORRUPTED, fa);
> > +		(*bpp)->b_flags &= ~XBF_DONE;
> > +		xfs_trans_brelse(tp, *bpp);
> > +		*bpp = NULL;
> > +		return -EFSCORRUPTED;
> > +	}
> > +
> > +	if (tp)
> >   		xfs_trans_buf_set_type(tp, *bpp, XFS_BLFT_DIR_DATA_BUF);
> >   	return err;
> >   }
> > 

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

[Darrick J. Wong]

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

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

[Darrick J. Wong]

On Fri, Feb 28, 2020 at 05:48:41PM -0800, 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.
> 
> 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);

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

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

[Dave Chinner]

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

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

[Christoph Hellwig]

On Fri, Feb 28, 2020 at 05:48:41PM -0800, 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.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH 2/4] xfs: check owner of dir3 free blocks

[Christoph Hellwig]

On Fri, Feb 28, 2020 at 05:48:48PM -0800, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Check the owner field of dir3 free block headers.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH 3/4] xfs: check owner of dir3 data blocks

[Christoph Hellwig]

On Fri, Feb 28, 2020 at 05:48:54PM -0800, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Check the owner field of dir3 data block headers.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH 4/4] xfs: check owner of dir3 blocks

[Christoph Hellwig]

On Fri, Feb 28, 2020 at 05:49:01PM -0800, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Check the owner field of dir3 block headers.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

> +		xfs_verifier_error(*bpp, -EFSCORRUPTED, fa);
> +		(*bpp)->b_flags &= ~XBF_DONE;
> +		xfs_trans_brelse(tp, *bpp);
> +		*bpp = NULL;
> +		return -EFSCORRUPTED;

Although I wonder if we should factor this repeated sniplet into a
helper..

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

[Darrick J. Wong]

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

Re: [PATCH 4/4] xfs: check owner of dir3 blocks

[Darrick J. Wong]

On Thu, Mar 05, 2020 at 08:51:28AM -0800, Christoph Hellwig wrote:
> On Fri, Feb 28, 2020 at 05:49:01PM -0800, Darrick J. Wong wrote:
> > From: Darrick J. Wong <darrick.wong@oracle.com>
> > 
> > Check the owner field of dir3 block headers.
> > 
> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Looks good,
> 
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> 
> > +		xfs_verifier_error(*bpp, -EFSCORRUPTED, fa);
> > +		(*bpp)->b_flags &= ~XBF_DONE;
> > +		xfs_trans_brelse(tp, *bpp);
> > +		*bpp = NULL;
> > +		return -EFSCORRUPTED;
> 
> Although I wonder if we should factor this repeated sniplet into a
> helper..

Yes.  Dave and I were discussing in patch 1 that
xfs_buf_corruption_error() is the correct function to call for a buffer
that we've decided is corrupt outside of a read verifier, so this all
becomes:

if (fa) {
	xfs_buf_corruption_error(*bpp...);
	xfs_trans_brelse(tp, *bpp);
	*bpp = NULL;
	return -EFSCORRUPTED;
}

(We also observed that xfs_buf_corruption_error ought to stale the
corrupt buffer to get it out of the system asap.)

--D