Re: [PATCH 09/13] xfs: validate the unlinked list pointer on update

Darrick J. Wong
2020-08-19T00:23:40Z

On Wed, Aug 12, 2020 at 07:25:52PM +1000, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
>
> Factor this check into xfs_iunlink_update_inode() when are updating
> the code. This replaces the checks that were removed in previous
> patches as bits of functionality were removed from the update
> process.

I had wondered about that, though I saw it end up in xfs_iunlink_item.c
so I hadn't thought too much about that.

> Signed-off-by: Dave Chinner <dchinner@redhat.com>

Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

--D

> ---
> fs/xfs/xfs_inode.c | 38 ++++++++++++++------------------------
> 1 file changed, 14 insertions(+), 24 deletions(-)
>
> diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
> index 4dde1970f7cd..b098e5df07e7 100644
> --- a/fs/xfs/xfs_inode.c
> +++ b/fs/xfs/xfs_inode.c
> @@ -1998,6 +1998,7 @@ xfs_iunlink_update_inode(
> struct xfs_trans *tp,
> struct xfs_inode *ip,
> xfs_agnumber_t agno,
> + xfs_agino_t old_agino,
> xfs_agino_t next_agino)
> {
> struct xfs_mount *mp = tp->t_mountp;
> @@ -2011,6 +2012,13 @@ xfs_iunlink_update_inode(
> if (error)
> return error;
>
> + if (be32_to_cpu(dip->di_next_unlinked) != old_agino) {
> + xfs_inode_verifier_error(ip, -EFSCORRUPTED, __func__, dip,
> + sizeof(*dip), __this_address);
> + xfs_trans_brelse(tp, ibp);
> + return -EFSCORRUPTED;
> + }
> +
> /* Ok, update the new pointer. */
> xfs_iunlink_update_dinode(tp, agno, XFS_INO_TO_AGINO(mp, ip->i_ino),
> ibp, dip, &ip->i_imap, next_agino);
> @@ -2056,7 +2064,8 @@ xfs_iunlink_insert_inode(
> * There is already another inode in the bucket, so point this
> * inode to the current head of the list.
> */
> - error = xfs_iunlink_update_inode(tp, ip, agno, next_agino);
> + error = xfs_iunlink_update_inode(tp, ip, agno, NULLAGINO,
> + next_agino);
> if (error)
> return error;
> } else {
> @@ -2147,37 +2156,18 @@ xfs_iunlink_remove_inode(
> }
>
> /* Clear the on disk next unlinked pointer for this inode. */
> - error = xfs_iunlink_update_inode(tp, ip, agno, NULLAGINO);
> + error = xfs_iunlink_update_inode(tp, ip, agno, next_agino, NULLAGINO);
> if (error)
> return error;
>
>
> if (ip != list_first_entry(&agibp->b_pag->pag_ici_unlink_list,
> struct xfs_inode, i_unlink)) {
> -
> - struct xfs_inode *pip;
> - xfs_agino_t prev_agino;
> - struct xfs_buf *last_ibp;
> - struct xfs_dinode *last_dip = NULL;
> + struct xfs_inode *pip = list_prev_entry(ip, i_unlink);
>
> ASSERT(head_agino != agino);
> -
> - pip = list_prev_entry(ip, i_unlink);
> - prev_agino = XFS_INO_TO_AGINO(mp, pip->i_ino);
> -
> - error = xfs_imap_to_bp(mp, tp, &pip->i_imap, &last_dip,
> - &last_ibp, 0);
> - if (error)
> - return error;
> -
> - if (be32_to_cpu(last_dip->di_next_unlinked) != agino)
> - return -EFSCORRUPTED;
> -
> - /* Point the previous inode on the list to the next inode. */
> - xfs_iunlink_update_dinode(tp, agno, prev_agino, last_ibp,
> - last_dip, &pip->i_imap, next_agino);
> -
> - return 0;
> + return xfs_iunlink_update_inode(tp, pip, agno, agino,
> + next_agino);
> }
>
> /* Point the head of the list to the next unlinked inode. */
> --
> 2.26.2.761.g0e0b3e54be
>