Re: [PATCH v3] xfs: fix an ABBA deadlock in xfs_rename

[Brian Foster <bfoster@redhat.com>]

On Mon, Jan 11, 2021 at 02:50:53PM -0800, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> Subject: [PATCH] xfs: fix an ABBA deadlock in xfs_rename
> 
> When overlayfs is running on top of xfs and the user unlinks a file in
> the overlay, overlayfs will create a whiteout inode and ask xfs to
> "rename" the whiteout file atop the one being unlinked.  If the file
> being unlinked loses its one nlink, we then have to put the inode on the
> unlinked list.
> 
> This requires us to grab the AGI buffer of the whiteout inode to take it
> off the unlinked list (which is where whiteouts are created) and to grab
> the AGI buffer of the file being deleted.  If the whiteout was created
> in a higher numbered AG than the file being deleted, we'll lock the AGIs
> in the wrong order and deadlock.
> 
> Therefore, grab all the AGI locks we think we'll need ahead of time, and
> in order of increasing AG number per the locking rules.
> 
> Reported-by: wenli xie <wlxie7296@gmail.com>
> Fixes: 93597ae8dac0 ("xfs: Fix deadlock between AGI and AGF when target_ip exists in xfs_rename()")
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
> v2: Make it more obvious that we're grabbing all the AGI locks ahead of
> the AGFs, and hide functions that we don't need to export anymore.
> v3: condense the predicate code even further
> ---

Looks good, thanks for the tweaks:

Reviewed-by: Brian Foster <bfoster@redhat.com>

>  fs/xfs/libxfs/xfs_dir2.h    |    2 --
>  fs/xfs/libxfs/xfs_dir2_sf.c |    2 +-
>  fs/xfs/xfs_inode.c          |   42 +++++++++++++++++++++++++-----------------
>  3 files changed, 26 insertions(+), 20 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_dir2.h b/fs/xfs/libxfs/xfs_dir2.h
> index e55378640b05..d03e6098ded9 100644
> --- a/fs/xfs/libxfs/xfs_dir2.h
> +++ b/fs/xfs/libxfs/xfs_dir2.h
> @@ -47,8 +47,6 @@ extern int xfs_dir_lookup(struct xfs_trans *tp, struct xfs_inode *dp,
>  extern int xfs_dir_removename(struct xfs_trans *tp, struct xfs_inode *dp,
>  				struct xfs_name *name, xfs_ino_t ino,
>  				xfs_extlen_t tot);
> -extern bool xfs_dir2_sf_replace_needblock(struct xfs_inode *dp,
> -				xfs_ino_t inum);
>  extern int xfs_dir_replace(struct xfs_trans *tp, struct xfs_inode *dp,
>  				struct xfs_name *name, xfs_ino_t inum,
>  				xfs_extlen_t tot);
> diff --git a/fs/xfs/libxfs/xfs_dir2_sf.c b/fs/xfs/libxfs/xfs_dir2_sf.c
> index 2463b5d73447..8c4f76bba88b 100644
> --- a/fs/xfs/libxfs/xfs_dir2_sf.c
> +++ b/fs/xfs/libxfs/xfs_dir2_sf.c
> @@ -1018,7 +1018,7 @@ xfs_dir2_sf_removename(
>  /*
>   * Check whether the sf dir replace operation need more blocks.
>   */
> -bool
> +static bool
>  xfs_dir2_sf_replace_needblock(
>  	struct xfs_inode	*dp,
>  	xfs_ino_t		inum)
> diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
> index b7352bc4c815..e5dc41b10ebb 100644
> --- a/fs/xfs/xfs_inode.c
> +++ b/fs/xfs/xfs_inode.c
> @@ -3017,7 +3017,7 @@ xfs_rename(
>  	struct xfs_trans	*tp;
>  	struct xfs_inode	*wip = NULL;		/* whiteout inode */
>  	struct xfs_inode	*inodes[__XFS_SORT_INODES];
> -	struct xfs_buf		*agibp;
> +	int			i;
>  	int			num_inodes = __XFS_SORT_INODES;
>  	bool			new_parent = (src_dp != target_dp);
>  	bool			src_is_directory = S_ISDIR(VFS_I(src_ip)->i_mode);
> @@ -3130,6 +3130,30 @@ xfs_rename(
>  		}
>  	}
>  
> +	/*
> +	 * Lock the AGI buffers we need to handle bumping the nlink of the
> +	 * whiteout inode off the unlinked list and to handle dropping the
> +	 * nlink of the target inode.  Per locking order rules, do this in
> +	 * increasing AG order and before directory block allocation tries to
> +	 * grab AGFs because we grab AGIs before AGFs.
> +	 *
> +	 * The (vfs) caller must ensure that if src is a directory then
> +	 * target_ip is either null or an empty directory.
> +	 */
> +	for (i = 0; i < num_inodes && inodes[i] != NULL; i++) {
> +		if (inodes[i] == wip ||
> +		    (inodes[i] == target_ip &&
> +		     (VFS_I(target_ip)->i_nlink == 1 || src_is_directory))) {
> +			struct xfs_buf	*bp;
> +			xfs_agnumber_t	agno;
> +
> +			agno = XFS_INO_TO_AGNO(mp, inodes[i]->i_ino);
> +			error = xfs_read_agi(mp, tp, agno, &bp);
> +			if (error)
> +				goto out_trans_cancel;
> +		}
> +	}
> +
>  	/*
>  	 * Directory entry creation below may acquire the AGF. Remove
>  	 * the whiteout from the unlinked list first to preserve correct
> @@ -3182,22 +3206,6 @@ xfs_rename(
>  		 * In case there is already an entry with the same
>  		 * name at the destination directory, remove it first.
>  		 */
> -
> -		/*
> -		 * Check whether the replace operation will need to allocate
> -		 * blocks.  This happens when the shortform directory lacks
> -		 * space and we have to convert it to a block format directory.
> -		 * When more blocks are necessary, we must lock the AGI first
> -		 * to preserve locking order (AGI -> AGF).
> -		 */
> -		if (xfs_dir2_sf_replace_needblock(target_dp, src_ip->i_ino)) {
> -			error = xfs_read_agi(mp, tp,
> -					XFS_INO_TO_AGNO(mp, target_ip->i_ino),
> -					&agibp);
> -			if (error)
> -				goto out_trans_cancel;
> -		}
> -
>  		error = xfs_dir_replace(tp, target_dp, target_name,
>  					src_ip->i_ino, spaceres);
>  		if (error)
>