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

[Kleber Souza]

On 11/03/17 03:16, Gavin Guo wrote:
> From: Brian Foster <bfoster at 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 at redhat.com>
> Reviewed-by: Darrick J. Wong <darrick.wong at oracle.com>
> Signed-off-by: Darrick J. Wong <darrick.wong at oracle.com>
> (cherry picked from commit f35c5e10c6ed6ba52a8dd8573924a80b6a02f03f)
> Signed-off-by: Gavin Guo <gavin.guo at 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 at 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);
>  		}
>