hulk inclusion category: bugfix bugzilla: https://gitee.com/openeuler/kernel/issues/IBALAU CVE: NA -------------------------------- This patch attempted to unmap attr leaf/node blocks when inactive attr. Under special attr structures, extent splitting may occur, which would increase attr bmap entries and allocate new blocks. Since attr inactive transaction has no block reservation, the splitting operations mentioned above could fail and eventually cause filesystem corruption. Considering attr inactive happens during inode gc process which should free more blocks, reserving blocks in transaction violates filesystem design. Therefore, revert this patch. This reverts commit 8fd6df1c963252d5681b8ea63c5765cdbde5e0a4. Fixes: 8fd6df1c9632 ("xfs: atomic drop extent entries when inactiving attr") Signed-off-by: Long Li <leo.lilong@huawei.com> --- fs/xfs/xfs_attr_inactive.c | 62 ++++++++++---------------------------- 1 file changed, 16 insertions(+), 46 deletions(-) diff --git a/fs/xfs/xfs_attr_inactive.c b/fs/xfs/xfs_attr_inactive.c index a0acbe3a7e3d..5993960d288f 100644 --- a/fs/xfs/xfs_attr_inactive.c +++ b/fs/xfs/xfs_attr_inactive.c @@ -23,7 +23,6 @@ #include "xfs_quota.h" #include "xfs_dir2.h" #include "xfs_error.h" -#include "xfs_defer.h" /* * Invalidate any incore buffers associated with this remote attribute value @@ -140,8 +139,7 @@ xfs_attr3_node_inactive( xfs_daddr_t parent_blkno, child_blkno; struct xfs_buf *child_bp; struct xfs_da3_icnode_hdr ichdr; - int error, i, done; - xfs_filblks_t count = mp->m_attr_geo->fsbcount; + int error, i; /* * Since this code is recursive (gasp!) we must protect ourselves. @@ -174,13 +172,10 @@ xfs_attr3_node_inactive( * traversal of the tree so we may deal with many blocks * before we come back to this one. */ - error = __xfs_da3_node_read(*trans, dp, child_fsb, - XFS_DABUF_MAP_HOLE_OK, &child_bp, - XFS_ATTR_FORK); + error = xfs_da3_node_read(*trans, dp, child_fsb, &child_bp, + XFS_ATTR_FORK); if (error) return error; - if (!child_bp) - goto next_entry; /* save for re-read later */ child_blkno = xfs_buf_daddr(child_bp); @@ -212,32 +207,14 @@ xfs_attr3_node_inactive( * Remove the subsidiary block from the cache and from the log. */ error = xfs_trans_get_buf(*trans, mp->m_ddev_targp, - child_blkno, XFS_FSB_TO_BB(mp, count), - 0, &child_bp); + child_blkno, + XFS_FSB_TO_BB(mp, mp->m_attr_geo->fsbcount), 0, + &child_bp); if (error) return error; - - error = xfs_bunmapi(*trans, dp, child_fsb, count, - XFS_BMAPI_ATTRFORK, 0, &done); - if (error) { - xfs_trans_brelse(*trans, child_bp); - return error; - } xfs_trans_binval(*trans, child_bp); - - error = xfs_defer_finish(trans); - if (error) - return error; child_bp = NULL; - /* - * Atomically commit the whole invalidate stuff. - */ - error = xfs_trans_roll_inode(trans, dp); - if (error) - return error; - -next_entry: /* * If we're not done, re-read the parent to get the next * child block number. @@ -255,6 +232,12 @@ xfs_attr3_node_inactive( xfs_trans_brelse(*trans, bp); bp = NULL; } + /* + * Atomically commit the whole invalidate stuff. + */ + error = xfs_trans_roll_inode(trans, dp); + if (error) + return error; } return 0; @@ -275,8 +258,7 @@ xfs_attr3_root_inactive( struct xfs_da_blkinfo *info; struct xfs_buf *bp; xfs_daddr_t blkno; - xfs_filblks_t count = mp->m_attr_geo->fsbcount; - int error, done; + int error; /* * Read block 0 to see what we have to work with. @@ -284,9 +266,8 @@ xfs_attr3_root_inactive( * the extents in reverse order the extent containing * block 0 must still be there. */ - error = __xfs_da3_node_read(*trans, dp, 0, XFS_DABUF_MAP_HOLE_OK, - &bp, XFS_ATTR_FORK); - if (error || !bp) + error = xfs_da3_node_read(*trans, dp, 0, &bp, XFS_ATTR_FORK); + if (error) return error; blkno = bp->b_bn; @@ -317,7 +298,7 @@ xfs_attr3_root_inactive( * Invalidate the incore copy of the root block. */ error = xfs_trans_get_buf(*trans, mp->m_ddev_targp, blkno, - XFS_FSB_TO_BB(mp, count), 0, &bp); + XFS_FSB_TO_BB(mp, mp->m_attr_geo->fsbcount), 0, &bp); if (error) return error; error = bp->b_error; @@ -325,17 +306,7 @@ xfs_attr3_root_inactive( xfs_trans_brelse(*trans, bp); return error; } - - error = xfs_bunmapi(*trans, dp, 0, count, XFS_BMAPI_ATTRFORK, 0, &done); - if (error) { - xfs_trans_brelse(*trans, bp); - return error; - } xfs_trans_binval(*trans, bp); /* remove from cache */ - - error = xfs_defer_finish(trans); - if (error) - return error; /* * Commit the invalidate and start the next transaction. */ @@ -398,7 +369,6 @@ xfs_attr_inactive( if (error) goto out_shutdown; - /* Remove the potential leftover remote attr blocks. */ error = xfs_itruncate_extents(&trans, dp, XFS_ATTR_FORK, 0); if (error) goto out_cancel; -- 2.39.2

hulk inclusion category: bugfix bugzilla: https://gitee.com/openeuler/kernel/issues/IBALAU CVE: NA -------------------------------- Factor out wrapper xfs_da3_node_entry_remove function, which exported for external use. Fixes: 8fd6df1c9632 ("xfs: atomic drop extent entries when inactiving attr") Signed-off-by: Long Li <leo.lilong@huawei.com> --- fs/xfs/libxfs/xfs_da_btree.c | 54 +++++++++++++++++++++++++++--------- fs/xfs/libxfs/xfs_da_btree.h | 2 ++ 2 files changed, 43 insertions(+), 13 deletions(-) diff --git a/fs/xfs/libxfs/xfs_da_btree.c b/fs/xfs/libxfs/xfs_da_btree.c index 83d0768ab26c..345458d2f632 100644 --- a/fs/xfs/libxfs/xfs_da_btree.c +++ b/fs/xfs/libxfs/xfs_da_btree.c @@ -1430,22 +1430,18 @@ xfs_da3_fixhashpath( } } -/* - * Remove an entry from an intermediate node. - */ STATIC void -xfs_da3_node_remove( - struct xfs_da_state *state, - struct xfs_da_state_blk *drop_blk) +__xfs_da3_node_remove( + struct xfs_trans *tp, + struct xfs_inode *dp, + struct xfs_da_geometry *geo, + struct xfs_da_state_blk *drop_blk) { struct xfs_da_intnode *node; struct xfs_da3_icnode_hdr nodehdr; struct xfs_da_node_entry *btree; int index; int tmp; - struct xfs_inode *dp = state->args->dp; - - trace_xfs_da_node_remove(state->args); node = drop_blk->bp->b_addr; xfs_da3_node_hdr_from_disk(dp->i_mount, &nodehdr, node); @@ -1461,17 +1457,17 @@ xfs_da3_node_remove( tmp = nodehdr.count - index - 1; tmp *= (uint)sizeof(xfs_da_node_entry_t); memmove(&btree[index], &btree[index + 1], tmp); - xfs_trans_log_buf(state->args->trans, drop_blk->bp, + xfs_trans_log_buf(tp, drop_blk->bp, XFS_DA_LOGRANGE(node, &btree[index], tmp)); index = nodehdr.count - 1; } memset(&btree[index], 0, sizeof(xfs_da_node_entry_t)); - xfs_trans_log_buf(state->args->trans, drop_blk->bp, + xfs_trans_log_buf(tp, drop_blk->bp, XFS_DA_LOGRANGE(node, &btree[index], sizeof(btree[index]))); nodehdr.count -= 1; xfs_da3_node_hdr_to_disk(dp->i_mount, node, &nodehdr); - xfs_trans_log_buf(state->args->trans, drop_blk->bp, - XFS_DA_LOGRANGE(node, &node->hdr, state->args->geo->node_hdr_size)); + xfs_trans_log_buf(tp, drop_blk->bp, + XFS_DA_LOGRANGE(node, &node->hdr, geo->node_hdr_size)); /* * Copy the last hash value from the block to propagate upwards. @@ -1479,6 +1475,38 @@ xfs_da3_node_remove( drop_blk->hashval = be32_to_cpu(btree[index - 1].hashval); } +/* + * Remove an entry from an intermediate node. + */ +STATIC void +xfs_da3_node_remove( + struct xfs_da_state *state, + struct xfs_da_state_blk *drop_blk) +{ + trace_xfs_da_node_remove(state->args); + __xfs_da3_node_remove(state->args->trans, state->args->dp, + state->args->geo, drop_blk); +} + +/* + * Wrapper function of remove an entry from node, export for external use. + */ +void +xfs_da3_node_entry_remove( + struct xfs_trans *tp, + struct xfs_inode *dp, + struct xfs_buf *bp, + int index) +{ + struct xfs_da_state_blk blk; + + memset(&blk, 0, sizeof(blk)); + blk.index = index; + blk.bp = bp; + + __xfs_da3_node_remove(tp, dp, dp->i_mount->m_attr_geo, &blk); +} + /* * Unbalance the elements between two intermediate nodes, * move all Btree elements from one node into another. diff --git a/fs/xfs/libxfs/xfs_da_btree.h b/fs/xfs/libxfs/xfs_da_btree.h index 0faf7d9ac241..98114c948673 100644 --- a/fs/xfs/libxfs/xfs_da_btree.h +++ b/fs/xfs/libxfs/xfs_da_btree.h @@ -212,6 +212,8 @@ int xfs_da_reada_buf(struct xfs_inode *dp, xfs_dablk_t bno, const struct xfs_buf_ops *ops); int xfs_da_shrink_inode(xfs_da_args_t *args, xfs_dablk_t dead_blkno, struct xfs_buf *dead_buf); +void xfs_da3_node_entry_remove(struct xfs_trans *tp, struct xfs_inode *dp, + struct xfs_buf *bp, int index); uint xfs_da_hashname(const uint8_t *name_string, int name_length); enum xfs_dacmp xfs_da_compname(struct xfs_da_args *args, -- 2.39.2

hulk inclusion category: bugfix bugzilla: https://gitee.com/openeuler/kernel/issues/IBALAU CVE: NA -------------------------------- When inactiving an unlinked inode and it's attrs, if xlog is shutdown either during or just after the process of recurse deleting attribute nodes/leafs in xfs_attr3_root_inactive(), the log will records some buffer cancel items, but doesn't contain the corresponding extent entries and inode updates, this is incomplete and inconsistent. Because of the inactiving process is not completed and the unlinked inode is still in the agi_unlinked table, it will continue to be inactived after replaying the log on the next mount, the attr node/leaf blocks' created record before the cancel items could not be replayed but the inode does. So we could get corrupted data when reading the canceled blocks. XFS (pmem0): Metadata corruption detected at xfs_da3_node_read_verify+0x53/0x220, xfs_da3_node block 0x78 XFS (pmem0): Unmount and run xfs_repair XFS (pmem0): First 128 bytes of corrupted metadata buffer: 00000000: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................ 00000010: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................ 00000020: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................ 00000030: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................ 00000040: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................ 00000050: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................ 00000060: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................ 00000070: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................ XFS (pmem0): metadata I/O error in "xfs_da_read_buf+0x104/0x190" at daddr 0x78 len 8 error 117 To fix this issue, put the index node/leaf block entries and the canceling of node/leaf blocks in the same transaction. For non-root blocks, it's straightforward. For root blocks, we need to put block cancel and the unmap of root block in the same transaction, so we split attr bmap truncation into two parts. During attr inactive process, even with any interruption, empty attr node blocks should never be indexed, as this is a corrupted image for xfs, while empty attr leaf blocks are allowed. To prevent empty root node blocks in the attr fork, we convert them to root leaf blocks in such scenarios. Fixes: 8fd6df1c9632 ("xfs: atomic drop extent entries when inactiving attr") Link: https://patchwork.kernel.org/project/xfs/patch/20230613030434.2944173-3-yi.z... Signed-off-by: Long Li <leo.lilong@huawei.com> --- fs/xfs/xfs_attr_inactive.c | 94 +++++++++++++++++++++----------------- fs/xfs/xfs_inode.c | 3 +- 2 files changed, 53 insertions(+), 44 deletions(-) diff --git a/fs/xfs/xfs_attr_inactive.c b/fs/xfs/xfs_attr_inactive.c index 5993960d288f..1dd3c0aa7012 100644 --- a/fs/xfs/xfs_attr_inactive.c +++ b/fs/xfs/xfs_attr_inactive.c @@ -139,7 +139,7 @@ xfs_attr3_node_inactive( xfs_daddr_t parent_blkno, child_blkno; struct xfs_buf *child_bp; struct xfs_da3_icnode_hdr ichdr; - int error, i; + int error; /* * Since this code is recursive (gasp!) we must protect ourselves. @@ -165,7 +165,7 @@ xfs_attr3_node_inactive( * over the leaves removing all of them. If this is higher up * in the tree, recurse downward. */ - for (i = 0; i < ichdr.count; i++) { + while (ichdr.count > 0) { /* * Read the subsidiary block to see what we have to work with. * Don't do this in a transaction. This is a depth-first @@ -215,29 +215,32 @@ xfs_attr3_node_inactive( xfs_trans_binval(*trans, child_bp); child_bp = NULL; + error = xfs_da3_node_read_mapped(*trans, dp, + parent_blkno, &bp, XFS_ATTR_FORK); + if (error) + return error; + /* - * If we're not done, re-read the parent to get the next - * child block number. + * Remove entry form parent node, prevents being indexed to. */ - if (i + 1 < ichdr.count) { - struct xfs_da3_icnode_hdr phdr; + xfs_da3_node_entry_remove(*trans, dp, bp, 0); + + xfs_da3_node_hdr_from_disk(dp->i_mount, &ichdr, bp->b_addr); + bp = NULL; + + if (ichdr.count > 0) { + /* + * If we're not done, get the next child block number. + */ + child_fsb = be32_to_cpu(ichdr.btree[0].before); - error = xfs_da3_node_read_mapped(*trans, dp, - parent_blkno, &bp, XFS_ATTR_FORK); + /* + * Atomically commit the whole invalidate stuff. + */ + error = xfs_trans_roll_inode(trans, dp); if (error) - return error; - xfs_da3_node_hdr_from_disk(dp->i_mount, &phdr, - bp->b_addr); - child_fsb = be32_to_cpu(phdr.btree[i + 1].before); - xfs_trans_brelse(*trans, bp); - bp = NULL; + return error; } - /* - * Atomically commit the whole invalidate stuff. - */ - error = xfs_trans_roll_inode(trans, dp); - if (error) - return error; } return 0; @@ -254,10 +257,8 @@ xfs_attr3_root_inactive( struct xfs_trans **trans, struct xfs_inode *dp) { - struct xfs_mount *mp = dp->i_mount; struct xfs_da_blkinfo *info; struct xfs_buf *bp; - xfs_daddr_t blkno; int error; /* @@ -269,7 +270,6 @@ xfs_attr3_root_inactive( error = xfs_da3_node_read(*trans, dp, 0, &bp, XFS_ATTR_FORK); if (error) return error; - blkno = bp->b_bn; /* * Invalidate the tree, even if the "tree" is only a single leaf block. @@ -280,6 +280,16 @@ xfs_attr3_root_inactive( case cpu_to_be16(XFS_DA_NODE_MAGIC): case cpu_to_be16(XFS_DA3_NODE_MAGIC): error = xfs_attr3_node_inactive(trans, dp, bp, 1); + if (error) + return error; + + /* + * Empty root node blocks are not allowed, convert it to leaf. + */ + error = xfs_attr3_leaf_init(*trans, dp, 0); + if (error) + return error; + error = xfs_trans_roll_inode(trans, dp); break; case cpu_to_be16(XFS_ATTR_LEAF_MAGIC): case cpu_to_be16(XFS_ATTR3_LEAF_MAGIC): @@ -291,26 +301,6 @@ xfs_attr3_root_inactive( xfs_trans_brelse(*trans, bp); break; } - if (error) - return error; - - /* - * Invalidate the incore copy of the root block. - */ - error = xfs_trans_get_buf(*trans, mp->m_ddev_targp, blkno, - XFS_FSB_TO_BB(mp, mp->m_attr_geo->fsbcount), 0, &bp); - if (error) - return error; - error = bp->b_error; - if (error) { - xfs_trans_brelse(*trans, bp); - return error; - } - xfs_trans_binval(*trans, bp); /* remove from cache */ - /* - * Commit the invalidate and start the next transaction. - */ - error = xfs_trans_roll_inode(trans, dp); return error; } @@ -329,6 +319,7 @@ xfs_attr_inactive( { struct xfs_trans *trans; struct xfs_mount *mp; + struct xfs_buf *bp; int lock_mode = XFS_ILOCK_SHARED; int error = 0; @@ -365,10 +356,27 @@ xfs_attr_inactive( * removal below. */ if (dp->i_af.if_nextents > 0) { + /* + * Invalidate and truncate all blocks but leave the root block. + */ error = xfs_attr3_root_inactive(&trans, dp); if (error) goto out_shutdown; + error = xfs_itruncate_extents(&trans, dp, XFS_ATTR_FORK, + XFS_FSB_TO_B(mp, mp->m_attr_geo->fsbcount)); + if (error) + goto out_cancel; + + /* + * Invalidate and truncate the root block. + */ + error = xfs_da_get_buf(trans, dp, 0, &bp, XFS_ATTR_FORK); + if (error) + goto out_cancel; + + xfs_trans_binval(trans, bp); + error = xfs_itruncate_extents(&trans, dp, XFS_ATTR_FORK, 0); if (error) goto out_cancel; diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c index 268bbc2d978b..99261829916a 100644 --- a/fs/xfs/xfs_inode.c +++ b/fs/xfs/xfs_inode.c @@ -1500,7 +1500,8 @@ xfs_itruncate_extents_flags( ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL)); ASSERT(!atomic_read(&VFS_I(ip)->i_count) || xfs_isilocked(ip, XFS_IOLOCK_EXCL)); - ASSERT(new_size <= XFS_ISIZE(ip)); + if (whichfork == XFS_DATA_FORK) + ASSERT(new_size <= XFS_ISIZE(ip)); ASSERT(tp->t_flags & XFS_TRANS_PERM_LOG_RES); ASSERT(ip->i_itemp != NULL); ASSERT(ip->i_itemp->ili_lock_flags == 0); -- 2.39.2