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