[PATCH] xfs: don't crash on unexpected holes in dir/attr btrees

[PATCH] xfs: don't crash on unexpected holes in dir/attr btrees

[Darrick J. Wong]

Hi all,

So I /think/ the xfs_attr_inactive crashes that both of you have been
seeing are a result of XFS assuming that there aren't ever any mapping
holes in the extended attribute fork and crashing when it tries to
grab a buffer for the hole and fails to notice that holes don't have
buffers.  This lightly tested patch gets rid of /that/ problem.

So, if you're willing, can you try this out and see if the crashes go
away?  Granted, this might only enable us to lurch on whatever's next...

--D

---
In quite a few places we call xfs_da_read_buf with a mappedbno that we
don't control, then assume that the function passes back either an error
code or a buffer pointer.  Unfortunately, if mappedbno == -2 and bno
maps to a hole, we get a return code of zero and a NULL buffer, which
means that we crash if we actually try to use that buffer pointer.  This
happens immediately when we set the buffer type for transaction context.

Therefore, check that we have no error code and a non-NULL bp before
trying to use bp.  This patch is a follow-up to an incomplete fix in
96a3aefb8ffde231 ("xfs: don't crash if reading a directory results in an
unexpected hole").

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 fs/xfs/libxfs/xfs_attr_leaf.c  |    2 +-
 fs/xfs/libxfs/xfs_da_btree.c   |    2 +-
 fs/xfs/libxfs/xfs_dir2_block.c |    2 +-
 fs/xfs/libxfs/xfs_dir2_leaf.c  |    4 ++--
 4 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_attr_leaf.c b/fs/xfs/libxfs/xfs_attr_leaf.c
index 2852521..c6c15e5 100644
--- a/fs/xfs/libxfs/xfs_attr_leaf.c
+++ b/fs/xfs/libxfs/xfs_attr_leaf.c
@@ -351,7 +351,7 @@ xfs_attr3_leaf_read(
 
 	err = xfs_da_read_buf(tp, dp, bno, mappedbno, bpp,
 				XFS_ATTR_FORK, &xfs_attr3_leaf_buf_ops);
-	if (!err && tp)
+	if (!err && tp && *bpp)
 		xfs_trans_buf_set_type(tp, *bpp, XFS_BLFT_ATTR_LEAF_BUF);
 	return err;
 }
diff --git a/fs/xfs/libxfs/xfs_da_btree.c b/fs/xfs/libxfs/xfs_da_btree.c
index 356f21d..6d43358 100644
--- a/fs/xfs/libxfs/xfs_da_btree.c
+++ b/fs/xfs/libxfs/xfs_da_btree.c
@@ -263,7 +263,7 @@ xfs_da3_node_read(
 
 	err = xfs_da_read_buf(tp, dp, bno, mappedbno, bpp,
 					which_fork, &xfs_da3_node_buf_ops);
-	if (!err && tp) {
+	if (!err && tp && *bpp) {
 		struct xfs_da_blkinfo	*info = (*bpp)->b_addr;
 		int			type;
 
diff --git a/fs/xfs/libxfs/xfs_dir2_block.c b/fs/xfs/libxfs/xfs_dir2_block.c
index aa17cb7..43c902f 100644
--- a/fs/xfs/libxfs/xfs_dir2_block.c
+++ b/fs/xfs/libxfs/xfs_dir2_block.c
@@ -139,7 +139,7 @@ xfs_dir3_block_read(
 
 	err = xfs_da_read_buf(tp, dp, mp->m_dir_geo->datablk, -1, bpp,
 				XFS_DATA_FORK, &xfs_dir3_block_buf_ops);
-	if (!err && tp)
+	if (!err && tp && *bpp)
 		xfs_trans_buf_set_type(tp, *bpp, XFS_BLFT_DIR_BLOCK_BUF);
 	return err;
 }
diff --git a/fs/xfs/libxfs/xfs_dir2_leaf.c b/fs/xfs/libxfs/xfs_dir2_leaf.c
index 7002024..27297a6 100644
--- a/fs/xfs/libxfs/xfs_dir2_leaf.c
+++ b/fs/xfs/libxfs/xfs_dir2_leaf.c
@@ -268,7 +268,7 @@ xfs_dir3_leaf_read(
 
 	err = xfs_da_read_buf(tp, dp, fbno, mappedbno, bpp,
 				XFS_DATA_FORK, &xfs_dir3_leaf1_buf_ops);
-	if (!err && tp)
+	if (!err && tp && *bpp)
 		xfs_trans_buf_set_type(tp, *bpp, XFS_BLFT_DIR_LEAF1_BUF);
 	return err;
 }
@@ -285,7 +285,7 @@ xfs_dir3_leafn_read(
 
 	err = xfs_da_read_buf(tp, dp, fbno, mappedbno, bpp,
 				XFS_DATA_FORK, &xfs_dir3_leafn_buf_ops);
-	if (!err && tp)
+	if (!err && tp && *bpp)
 		xfs_trans_buf_set_type(tp, *bpp, XFS_BLFT_DIR_LEAFN_BUF);
 	return err;
 }

Re: [PATCH] xfs: don't crash on unexpected holes in dir/attr btrees

[Eric Sandeen]

On 6/16/17 12:53 PM, Darrick J. Wong wrote:
> Hi all,
> 
> So I /think/ the xfs_attr_inactive crashes that both of you have been
> seeing are a result of XFS assuming that there aren't ever any mapping
> holes in the extended attribute fork and crashing when it tries to
> grab a buffer for the hole and fails to notice that holes don't have
> buffers.  This lightly tested patch gets rid of /that/ problem.
> 
> So, if you're willing, can you try this out and see if the crashes go
> away?  Granted, this might only enable us to lurch on whatever's next...

If it's unexpected, does that mean something else is wrong?  If so,
should there be a warning or a corrupted state, or anything like that?

-Eric

> --D
> 
> ---
> In quite a few places we call xfs_da_read_buf with a mappedbno that we
> don't control, then assume that the function passes back either an error
> code or a buffer pointer.  Unfortunately, if mappedbno == -2 and bno
> maps to a hole, we get a return code of zero and a NULL buffer, which
> means that we crash if we actually try to use that buffer pointer.  This
> happens immediately when we set the buffer type for transaction context.
> 
> Therefore, check that we have no error code and a non-NULL bp before
> trying to use bp.  This patch is a follow-up to an incomplete fix in
> 96a3aefb8ffde231 ("xfs: don't crash if reading a directory results in an
> unexpected hole").
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
>  fs/xfs/libxfs/xfs_attr_leaf.c  |    2 +-
>  fs/xfs/libxfs/xfs_da_btree.c   |    2 +-
>  fs/xfs/libxfs/xfs_dir2_block.c |    2 +-
>  fs/xfs/libxfs/xfs_dir2_leaf.c  |    4 ++--
>  4 files changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_attr_leaf.c b/fs/xfs/libxfs/xfs_attr_leaf.c
> index 2852521..c6c15e5 100644
> --- a/fs/xfs/libxfs/xfs_attr_leaf.c
> +++ b/fs/xfs/libxfs/xfs_attr_leaf.c
> @@ -351,7 +351,7 @@ xfs_attr3_leaf_read(
>  
>  	err = xfs_da_read_buf(tp, dp, bno, mappedbno, bpp,
>  				XFS_ATTR_FORK, &xfs_attr3_leaf_buf_ops);
> -	if (!err && tp)
> +	if (!err && tp && *bpp)
>  		xfs_trans_buf_set_type(tp, *bpp, XFS_BLFT_ATTR_LEAF_BUF);
>  	return err;
>  }
> diff --git a/fs/xfs/libxfs/xfs_da_btree.c b/fs/xfs/libxfs/xfs_da_btree.c
> index 356f21d..6d43358 100644
> --- a/fs/xfs/libxfs/xfs_da_btree.c
> +++ b/fs/xfs/libxfs/xfs_da_btree.c
> @@ -263,7 +263,7 @@ xfs_da3_node_read(
>  
>  	err = xfs_da_read_buf(tp, dp, bno, mappedbno, bpp,
>  					which_fork, &xfs_da3_node_buf_ops);
> -	if (!err && tp) {
> +	if (!err && tp && *bpp) {
>  		struct xfs_da_blkinfo	*info = (*bpp)->b_addr;
>  		int			type;
>  
> diff --git a/fs/xfs/libxfs/xfs_dir2_block.c b/fs/xfs/libxfs/xfs_dir2_block.c
> index aa17cb7..43c902f 100644
> --- a/fs/xfs/libxfs/xfs_dir2_block.c
> +++ b/fs/xfs/libxfs/xfs_dir2_block.c
> @@ -139,7 +139,7 @@ xfs_dir3_block_read(
>  
>  	err = xfs_da_read_buf(tp, dp, mp->m_dir_geo->datablk, -1, bpp,
>  				XFS_DATA_FORK, &xfs_dir3_block_buf_ops);
> -	if (!err && tp)
> +	if (!err && tp && *bpp)
>  		xfs_trans_buf_set_type(tp, *bpp, XFS_BLFT_DIR_BLOCK_BUF);
>  	return err;
>  }
> diff --git a/fs/xfs/libxfs/xfs_dir2_leaf.c b/fs/xfs/libxfs/xfs_dir2_leaf.c
> index 7002024..27297a6 100644
> --- a/fs/xfs/libxfs/xfs_dir2_leaf.c
> +++ b/fs/xfs/libxfs/xfs_dir2_leaf.c
> @@ -268,7 +268,7 @@ xfs_dir3_leaf_read(
>  
>  	err = xfs_da_read_buf(tp, dp, fbno, mappedbno, bpp,
>  				XFS_DATA_FORK, &xfs_dir3_leaf1_buf_ops);
> -	if (!err && tp)
> +	if (!err && tp && *bpp)
>  		xfs_trans_buf_set_type(tp, *bpp, XFS_BLFT_DIR_LEAF1_BUF);
>  	return err;
>  }
> @@ -285,7 +285,7 @@ xfs_dir3_leafn_read(
>  
>  	err = xfs_da_read_buf(tp, dp, fbno, mappedbno, bpp,
>  				XFS_DATA_FORK, &xfs_dir3_leafn_buf_ops);
> -	if (!err && tp)
> +	if (!err && tp && *bpp)
>  		xfs_trans_buf_set_type(tp, *bpp, XFS_BLFT_DIR_LEAFN_BUF);
>  	return err;
>  }
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH] xfs: don't crash on unexpected holes in dir/attr btrees

[Christoph Hellwig]

On Fri, Jun 16, 2017 at 01:12:41PM -0500, Eric Sandeen wrote:
> On 6/16/17 12:53 PM, Darrick J. Wong wrote:
> > Hi all,
> > 
> > So I /think/ the xfs_attr_inactive crashes that both of you have been
> > seeing are a result of XFS assuming that there aren't ever any mapping
> > holes in the extended attribute fork and crashing when it tries to
> > grab a buffer for the hole and fails to notice that holes don't have
> > buffers.  This lightly tested patch gets rid of /that/ problem.
> > 
> > So, if you're willing, can you try this out and see if the crashes go
> > away?  Granted, this might only enable us to lurch on whatever's next...
> 
> If it's unexpected, does that mean something else is wrong?  If so,
> should there be a warning or a corrupted state, or anything like that?

xfs_attr_rmtval_remove certainly removes blocks from the attr mapping,
but those should be expected..

Re: [PATCH] xfs: don't crash on unexpected holes in dir/attr btrees

[Darrick J. Wong]

On Tue, Jun 20, 2017 at 02:06:07AM -0700, Christoph Hellwig wrote:
> On Fri, Jun 16, 2017 at 01:12:41PM -0500, Eric Sandeen wrote:
> > On 6/16/17 12:53 PM, Darrick J. Wong wrote:
> > > Hi all,
> > > 
> > > So I /think/ the xfs_attr_inactive crashes that both of you have been
> > > seeing are a result of XFS assuming that there aren't ever any mapping
> > > holes in the extended attribute fork and crashing when it tries to
> > > grab a buffer for the hole and fails to notice that holes don't have
> > > buffers.  This lightly tested patch gets rid of /that/ problem.
> > > 
> > > So, if you're willing, can you try this out and see if the crashes go
> > > away?  Granted, this might only enable us to lurch on whatever's next...
> > 
> > If it's unexpected, does that mean something else is wrong?  If so,
> > should there be a warning or a corrupted state, or anything like that?
> 
> xfs_attr_rmtval_remove certainly removes blocks from the attr mapping,
> but those should be expected..

Yes, I think pretty much any attribute removal can result in blocks
being removed from the attribute mapping, but here we have the case that
the da btree points to some offset... but the offset isn't mapped and so
the attr_inactive call blows up.

That said, this patch also fixes the remaining cases where someone
deliberately feeds us a corrupt filesystem and we blow up trying to
xfs_trans_buf_set_type on a null buffer, so can I please get a review of
this patch so I can push it upstream? :)

--D

> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] xfs: don't crash on unexpected holes in dir/attr btrees

[David Shaw]

On Jun 16, 2017, at 1:53 PM, Darrick J. Wong <darrick.wong@oracle.com> wrote:
> 
> Hi all,
> 
> So I /think/ the xfs_attr_inactive crashes that both of you have been
> seeing are a result of XFS assuming that there aren't ever any mapping
> holes in the extended attribute fork and crashing when it tries to
> grab a buffer for the hole and fails to notice that holes don't have
> buffers.  This lightly tested patch gets rid of /that/ problem.
> 
> So, if you're willing, can you try this out and see if the crashes go
> away?  Granted, this might only enable us to lurch on whatever's next...

I can give this a try.

Now that you have an idea of what caused the issue, is there some way to (easily or not) reproduce it, ideally in userspace?  Part of the problem we're having is that we can't reproduce it on a test machine so are forced to wait for it to happen in the field under load, where it inconveniences users.

David

Re: [PATCH] xfs: don't crash on unexpected holes in dir/attr btrees

[Darrick J. Wong]

On Tue, Jun 20, 2017 at 09:01:11AM -0400, David Shaw wrote:
> On Jun 16, 2017, at 1:53 PM, Darrick J. Wong <darrick.wong@oracle.com> wrote:
> > 
> > Hi all,
> > 
> > So I /think/ the xfs_attr_inactive crashes that both of you have been
> > seeing are a result of XFS assuming that there aren't ever any mapping
> > holes in the extended attribute fork and crashing when it tries to
> > grab a buffer for the hole and fails to notice that holes don't have
> > buffers.  This lightly tested patch gets rid of /that/ problem.
> > 
> > So, if you're willing, can you try this out and see if the crashes go
> > away?  Granted, this might only enable us to lurch on whatever's next...
> 
> I can give this a try.
> 
> Now that you have an idea of what caused the issue, is there some way
> to (easily or not) reproduce it, ideally in userspace?  Part of the
> problem we're having is that we can't reproduce it on a test machine
> so are forced to wait for it to happen in the field under load, where
> it inconveniences users.

I know what caused the crash, but I don't know what caused the hole in
the xattr fork that caused the crash.  It would appear that the xattr
hash tree contains a pointer to an xattr fork offset that is no longer
mapped, but I don't know how we end up with a dangling pointer --
whether this is corruption going on in the attribute fork, or merely a
bug in the routine that tears down the attribute fork and accidentally
tries to delete something twice.

Unfortunately, the metadumps you've both sent me don't seem to contain
any inodes with sparse attribute forks, so evidently something cleaned
up the filesystem before the metadump was taken.

It might help to know a little more about what's going on with the
xattrs that are being attached to those files.  A fair number of the
inodes seem to have extended attributes with 2k values attached.  Does
your workload involve adding and deleting a large number of xattrs?  A
number of large xattrs?  A large number of large xattrs?

That said, if we're tearing down the inode anyway, maybe it doesn't
matter if there's a hole in the map, seeing as the attr_inactive
function already knows how to handle holes and the blocks are all
getting freed anyway.

--D

> 
> David
> 

Re: [PATCH] xfs: don't crash on unexpected holes in dir/attr btrees

[David Shaw]

On Jun 20, 2017, at 12:20 PM, Darrick J. Wong <darrick.wong@oracle.com> wrote:

>> Now that you have an idea of what caused the issue, is there some way
>> to (easily or not) reproduce it, ideally in userspace?  Part of the
>> problem we're having is that we can't reproduce it on a test machine
>> so are forced to wait for it to happen in the field under load, where
>> it inconveniences users.
> 
> I know what caused the crash, but I don't know what caused the hole in
> the xattr fork that caused the crash.  It would appear that the xattr
> hash tree contains a pointer to an xattr fork offset that is no longer
> mapped, but I don't know how we end up with a dangling pointer --
> whether this is corruption going on in the attribute fork, or merely a
> bug in the routine that tears down the attribute fork and accidentally
> tries to delete something twice.
> 
> Unfortunately, the metadumps you've both sent me don't seem to contain
> any inodes with sparse attribute forks, so evidently something cleaned
> up the filesystem before the metadump was taken.

The filesystems were automatically mounted after the box rebooted.  Could the mount (via replaying the journal) have cleaned things up?

I have a reproduction case for this now - it takes several hours, but can reliably cause this panic.  I will send you a new metadump that was not mounted after the failure.

> It might help to know a little more about what's going on with the
> xattrs that are being attached to those files.  A fair number of the
> inodes seem to have extended attributes with 2k values attached.  Does
> your workload involve adding and deleting a large number of xattrs?  A
> number of large xattrs?  A large number of large xattrs?

All of the above.  This is Samba with the alternate data streams module (vfs_streams_xattr), which stores the ADS in xattrs.  It makes for some large xattrs.

David