[SRU,X/Z/A] xfs: reinit btree pointer on attr tree inactivation walk

Message ID 1509675378-15958-2-git-send-email-gavin.guo@canonical.com
State New
Headers show
Series
  • [SRU,X/Z/A] xfs: reinit btree pointer on attr tree inactivation walk
Related show

Commit Message

Gavin Guo Nov. 3, 2017, 2:16 a.m.
From: Brian Foster <bfoster@redhat.com>

BugLink: http://bugs.launchpad.net/bugs/1729256

xfs_attr3_root_inactive() walks the attr fork tree to invalidate the
associated blocks. xfs_attr3_node_inactive() recursively descends
from internal blocks to leaf blocks, caching block address values
along the way to revisit parent blocks, locate the next entry and
descend down that branch of the tree.

The code that attempts to reread the parent block is unsafe because
it assumes that the local xfs_da_node_entry pointer remains valid
after an xfs_trans_brelse() and re-read of the parent buffer. Under
heavy memory pressure, it is possible that the buffer has been
reclaimed and reallocated by the time the parent block is reread.
This means that 'btree' can point to an invalid memory address, lead
to a random/garbage value for child_fsb and cause the subsequent
read of the attr fork to go off the rails and return a NULL buffer
for an attr fork offset that is most likely not allocated.

Note that this problem can be manufactured by setting
XFS_ATTR_BTREE_REF to 0 to prevent LRU caching of attr buffers,
creating a file with a multi-level attr fork and removing it to
trigger inactivation.

To address this problem, reinit the node/btree pointers to the
parent buffer after it has been re-read. This ensures btree points
to a valid record and allows the walk to proceed.

Signed-off-by: Brian Foster <bfoster@redhat.com>
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
(cherry picked from commit f35c5e10c6ed6ba52a8dd8573924a80b6a02f03f)
Signed-off-by: Gavin Guo <gavin.guo@canonical.com>
---
 fs/xfs/xfs_attr_inactive.c | 2 ++
 1 file changed, 2 insertions(+)

Comments

Stefan Bader Nov. 13, 2017, 10:49 a.m. | #1
On 03.11.2017 03:16, Gavin Guo wrote:
> From: Brian Foster <bfoster@redhat.com>
> 
> BugLink: http://bugs.launchpad.net/bugs/1729256
> 
> xfs_attr3_root_inactive() walks the attr fork tree to invalidate the
> associated blocks. xfs_attr3_node_inactive() recursively descends
> from internal blocks to leaf blocks, caching block address values
> along the way to revisit parent blocks, locate the next entry and
> descend down that branch of the tree.
> 
> The code that attempts to reread the parent block is unsafe because
> it assumes that the local xfs_da_node_entry pointer remains valid
> after an xfs_trans_brelse() and re-read of the parent buffer. Under
> heavy memory pressure, it is possible that the buffer has been
> reclaimed and reallocated by the time the parent block is reread.
> This means that 'btree' can point to an invalid memory address, lead
> to a random/garbage value for child_fsb and cause the subsequent
> read of the attr fork to go off the rails and return a NULL buffer
> for an attr fork offset that is most likely not allocated.
> 
> Note that this problem can be manufactured by setting
> XFS_ATTR_BTREE_REF to 0 to prevent LRU caching of attr buffers,
> creating a file with a multi-level attr fork and removing it to
> trigger inactivation.
> 
> To address this problem, reinit the node/btree pointers to the
> parent buffer after it has been re-read. This ensures btree points
> to a valid record and allows the walk to proceed.
> 
> Signed-off-by: Brian Foster <bfoster@redhat.com>
> Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> (cherry picked from commit f35c5e10c6ed6ba52a8dd8573924a80b6a02f03f)
> Signed-off-by: Gavin Guo <gavin.guo@canonical.com>
Acked-by: Stefan Bader <stefan.bader@canonical.com>

> ---
>  fs/xfs/xfs_attr_inactive.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/fs/xfs/xfs_attr_inactive.c b/fs/xfs/xfs_attr_inactive.c
> index 2bb959ada45b..d8359f41f4a7 100644
> --- a/fs/xfs/xfs_attr_inactive.c
> +++ b/fs/xfs/xfs_attr_inactive.c
> @@ -302,6 +302,8 @@ xfs_attr3_node_inactive(
>  						 &bp, XFS_ATTR_FORK);
>  			if (error)
>  				return error;
> +			node = bp->b_addr;
> +			btree = dp->d_ops->node_tree_p(node);
>  			child_fsb = be32_to_cpu(btree[i + 1].before);
>  			xfs_trans_brelse(*trans, bp);
>  		}
>
Kleber Souza Nov. 15, 2017, 1:47 p.m. | #2
On 11/03/17 03:16, Gavin Guo wrote:
> From: Brian Foster <bfoster@redhat.com>
> 
> BugLink: http://bugs.launchpad.net/bugs/1729256
> 
> xfs_attr3_root_inactive() walks the attr fork tree to invalidate the
> associated blocks. xfs_attr3_node_inactive() recursively descends
> from internal blocks to leaf blocks, caching block address values
> along the way to revisit parent blocks, locate the next entry and
> descend down that branch of the tree.
> 
> The code that attempts to reread the parent block is unsafe because
> it assumes that the local xfs_da_node_entry pointer remains valid
> after an xfs_trans_brelse() and re-read of the parent buffer. Under
> heavy memory pressure, it is possible that the buffer has been
> reclaimed and reallocated by the time the parent block is reread.
> This means that 'btree' can point to an invalid memory address, lead
> to a random/garbage value for child_fsb and cause the subsequent
> read of the attr fork to go off the rails and return a NULL buffer
> for an attr fork offset that is most likely not allocated.
> 
> Note that this problem can be manufactured by setting
> XFS_ATTR_BTREE_REF to 0 to prevent LRU caching of attr buffers,
> creating a file with a multi-level attr fork and removing it to
> trigger inactivation.
> 
> To address this problem, reinit the node/btree pointers to the
> parent buffer after it has been re-read. This ensures btree points
> to a valid record and allows the walk to proceed.
> 
> Signed-off-by: Brian Foster <bfoster@redhat.com>
> Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> (cherry picked from commit f35c5e10c6ed6ba52a8dd8573924a80b6a02f03f)
> Signed-off-by: Gavin Guo <gavin.guo@canonical.com>

Patch is already queued for Artful as part of update to 4.13.10 stable
release. So for Xenial and Zesty:

Acked-by: Kleber Sacilotto de Souza <kleber.souza@canonical.com>

> ---
>  fs/xfs/xfs_attr_inactive.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/fs/xfs/xfs_attr_inactive.c b/fs/xfs/xfs_attr_inactive.c
> index 2bb959ada45b..d8359f41f4a7 100644
> --- a/fs/xfs/xfs_attr_inactive.c
> +++ b/fs/xfs/xfs_attr_inactive.c
> @@ -302,6 +302,8 @@ xfs_attr3_node_inactive(
>  						 &bp, XFS_ATTR_FORK);
>  			if (error)
>  				return error;
> +			node = bp->b_addr;
> +			btree = dp->d_ops->node_tree_p(node);
>  			child_fsb = be32_to_cpu(btree[i + 1].before);
>  			xfs_trans_brelse(*trans, bp);
>  		}
>

Patch

diff --git a/fs/xfs/xfs_attr_inactive.c b/fs/xfs/xfs_attr_inactive.c
index 2bb959ada45b..d8359f41f4a7 100644
--- a/fs/xfs/xfs_attr_inactive.c
+++ b/fs/xfs/xfs_attr_inactive.c
@@ -302,6 +302,8 @@  xfs_attr3_node_inactive(
 						 &bp, XFS_ATTR_FORK);
 			if (error)
 				return error;
+			node = bp->b_addr;
+			btree = dp->d_ops->node_tree_p(node);
 			child_fsb = be32_to_cpu(btree[i + 1].before);
 			xfs_trans_brelse(*trans, bp);
 		}