On Sun, Oct 04, 2020 at 12:11:27PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@xxxxxxxxxx>
>
> In xfs_bui_item_recover, there exists a use-after-free bug with regards
> to the inode that is involved in the bmap replay operation. If the
> mapping operation does not complete, we call xfs_bmap_unmap_extent to
> create a deferred op to finish the unmapping work, and we retain a
> pointer to the incore inode.
>
> Unfortunately, the very next thing we do is commit the transaction and
> drop the inode. If reclaim tears down the inode before we try to finish
> the defer ops, we dereference garbage and blow up. Therefore, create a
> way to join inodes to the defer ops freezer so that we can maintain the
> xfs_inode reference until we're done with the inode.
>
> Note: This imposes the requirement that there be enough memory to keep
> every incore inode in memory throughout recovery.
>
> Signed-off-by: Darrick J. Wong <darrick.wong@xxxxxxxxxx>
> ---
> v3.3: ihold the captured inode and let callers iunlock/irele their own
> reference
> v3.2: rebase on updated defer capture patches
> ---
> fs/xfs/libxfs/xfs_defer.c | 43 ++++++++++++++++++++++++++++++++++++++-----
> fs/xfs/libxfs/xfs_defer.h | 11 +++++++++--
> fs/xfs/xfs_bmap_item.c | 7 +++++--
> fs/xfs/xfs_extfree_item.c | 2 +-
> fs/xfs/xfs_inode.c | 8 ++++++++
> fs/xfs/xfs_inode.h | 2 ++
> fs/xfs/xfs_log_recover.c | 7 ++++++-
> fs/xfs/xfs_refcount_item.c | 2 +-
> fs/xfs/xfs_rmap_item.c | 2 +-
> 9 files changed, 71 insertions(+), 13 deletions(-)
>
...
> diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
> index 2bfbcf28b1bd..24b1e2244905 100644
> --- a/fs/xfs/xfs_inode.c
> +++ b/fs/xfs/xfs_inode.c
> @@ -3813,3 +3813,11 @@ xfs_iunlock2_io_mmap(
> if (!same_inode)
> inode_unlock(VFS_I(ip1));
> }
> +
> +/* Grab an extra reference to the VFS inode. */
> +void
> +xfs_ihold(
> + struct xfs_inode *ip)
> +{
> + ihold(VFS_I(ip));
> +}
It looks to me that the only reason xfs_irele() exists is for a
tracepoint. We don't have that here, so what's the purpose of the
helper?
Otherwise the patch looks good to me:
Reviewed-by: Brian Foster <bfoster@xxxxxxxxxx>
> diff --git a/fs/xfs/xfs_inode.h b/fs/xfs/xfs_inode.h
> index 751a3d1d7d84..e9b0186b594c 100644
> --- a/fs/xfs/xfs_inode.h
> +++ b/fs/xfs/xfs_inode.h
> @@ -476,4 +476,6 @@ void xfs_end_io(struct work_struct *work);
> int xfs_ilock2_io_mmap(struct xfs_inode *ip1, struct xfs_inode *ip2);
> void xfs_iunlock2_io_mmap(struct xfs_inode *ip1, struct xfs_inode *ip2);
>
> +void xfs_ihold(struct xfs_inode *ip);
> +
> #endif /* __XFS_INODE_H__ */
> diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
> index 001e1585ddc6..a8289adc1b29 100644
> --- a/fs/xfs/xfs_log_recover.c
> +++ b/fs/xfs/xfs_log_recover.c
> @@ -2439,6 +2439,7 @@ xlog_finish_defer_ops(
> {
> struct xfs_defer_capture *dfc, *next;
> struct xfs_trans *tp;
> + struct xfs_inode *ip;
> int error = 0;
>
> list_for_each_entry_safe(dfc, next, capture_list, dfc_list) {
> @@ -2464,9 +2465,13 @@ xlog_finish_defer_ops(
> * from recovering a single intent item.
> */
> list_del_init(&dfc->dfc_list);
> - xfs_defer_ops_continue(dfc, tp);
> + xfs_defer_ops_continue(dfc, tp, &ip);
>
> error = xfs_trans_commit(tp);
> + if (ip) {
> + xfs_iunlock(ip, XFS_ILOCK_EXCL);
> + xfs_irele(ip);
> + }
> if (error)
> return error;
> }
> diff --git a/fs/xfs/xfs_refcount_item.c b/fs/xfs/xfs_refcount_item.c
> index 0478374add64..ad895b48f365 100644
> --- a/fs/xfs/xfs_refcount_item.c
> +++ b/fs/xfs/xfs_refcount_item.c
> @@ -544,7 +544,7 @@ xfs_cui_item_recover(
> }
>
> xfs_refcount_finish_one_cleanup(tp, rcur, error);
> - return xfs_defer_ops_capture_and_commit(tp, capture_list);
> + return xfs_defer_ops_capture_and_commit(tp, NULL, capture_list);
>
> abort_error:
> xfs_refcount_finish_one_cleanup(tp, rcur, error);
> diff --git a/fs/xfs/xfs_rmap_item.c b/fs/xfs/xfs_rmap_item.c
> index 0d8fa707f079..1163f32c3e62 100644
> --- a/fs/xfs/xfs_rmap_item.c
> +++ b/fs/xfs/xfs_rmap_item.c
> @@ -567,7 +567,7 @@ xfs_rui_item_recover(
> }
>
> xfs_rmap_finish_one_cleanup(tp, rcur, error);
> - return xfs_defer_ops_capture_and_commit(tp, capture_list);
> + return xfs_defer_ops_capture_and_commit(tp, NULL, capture_list);
>
> abort_error:
> xfs_rmap_finish_one_cleanup(tp, rcur, error);
>
