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

From: "Darrick J. Wong" <darrick.wong@oracle.com>
To: Dave Chinner <david@fromorbit.com>
Cc: linux-xfs@vger.kernel.org
Subject: Re: [PATCH 09/13] xfs: validate the unlinked list pointer on update
Date: Tue, 18 Aug 2020 17:23:32 -0700	[thread overview]
Message-ID: <20200819002332.GW6096@magnolia> (raw)
In-Reply-To: <20200812092556.2567285-10-david@fromorbit.com>

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
> 