Re: [PATCH] xfs: fix intent use-after-free on abort

["Darrick J. Wong" <darrick.wong@oracle.com>]

On Tue, Apr 03, 2018 at 11:44:58AM +1000, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> When an intent is aborted during it's initial commit through
> xfs_defer_trans_abort(), there is a use after free. The current
> report is for a RUI  through this path in generic/388:
> 
>  Freed by task 6274:
>   __kasan_slab_free+0x136/0x180
>   kmem_cache_free+0xe7/0x4b0
>   xfs_trans_free_items+0x198/0x2e0
>   __xfs_trans_commit+0x27f/0xcc0
>   xfs_trans_roll+0x17b/0x2a0
>   xfs_defer_trans_roll+0x6ad/0xe60
>   xfs_defer_finish+0x2a6/0x2140
>   xfs_alloc_file_space+0x53a/0xf90
>   xfs_file_fallocate+0x5c6/0xac0
>   vfs_fallocate+0x2f5/0x930
>   ioctl_preallocate+0x1dc/0x320
>   do_vfs_ioctl+0xfe4/0x1690
> 
> The problem is that the RUI has two active references - one in the
> current transaction, and another held by the defer_ops structure
> that is passed to the RUD (intent done) so that both the intent and
> the intent done structures are freed on commit of the intent done.
> 
> Hence during abort, we need to release the intent item, because the
> defer_ops reference is released separately via ->abort_intent
> callback. Fix all the intent code to do this correctly.
> 
> Signed-Off-By: Dave Chinner <dchinner@redhat.com>
> ---
>  fs/xfs/xfs_bmap_item.c     | 39 ++++++++++++++++++++-------------------
>  fs/xfs/xfs_extfree_item.c  | 38 +++++++++++++++++++-------------------
>  fs/xfs/xfs_refcount_item.c | 39 ++++++++++++++++++++-------------------
>  fs/xfs/xfs_rmap_item.c     | 38 +++++++++++++++++++-------------------
>  4 files changed, 78 insertions(+), 76 deletions(-)
> 
> diff --git a/fs/xfs/xfs_bmap_item.c b/fs/xfs/xfs_bmap_item.c
> index d769a82f3641..618bb71535c8 100644
> --- a/fs/xfs/xfs_bmap_item.c
> +++ b/fs/xfs/xfs_bmap_item.c
> @@ -53,6 +53,25 @@ xfs_bui_item_free(
>  	kmem_zone_free(xfs_bui_zone, buip);
>  }
>  
> +/*
> + * Freeing the BUI requires that we remove it from the AIL if it has already
> + * been placed there. However, the BUI may not yet have been placed in the AIL
> + * when called by xfs_bui_release() from BUD processing due to the ordering of
> + * committed vs unpin operations in bulk insert operations. Hence the reference
> + * count to ensure only the last caller frees the BUI.
> + */
> +void
> +xfs_bui_release(
> +	struct xfs_bui_log_item	*buip)
> +{
> +	ASSERT(atomic_read(&buip->bui_refcount) > 0);
> +	if (atomic_dec_and_test(&buip->bui_refcount)) {
> +		xfs_trans_ail_remove(&buip->bui_item, SHUTDOWN_LOG_IO_ERROR);
> +		xfs_bui_item_free(buip);
> +	}
> +}
> +
> +
>  STATIC void
>  xfs_bui_item_size(
>  	struct xfs_log_item	*lip,
> @@ -142,7 +161,7 @@ xfs_bui_item_unlock(
>  	struct xfs_log_item	*lip)
>  {
>  	if (test_bit(XFS_LI_ABORTED, &lip->li_flags))

Hm, in my tree this is still "if (lip->li_flags & XFS_LI_ABORTED)" :)
/me wonders where the test_bit() came from?

But this is the same change that I came up with to solve the problem, so
I guess I'll go feed it to the test machines and see how they fare
overnight, and mash on it harder with generic/388 tomorrow.

--D

> -		xfs_bui_item_free(BUI_ITEM(lip));
> +		xfs_bui_release(BUI_ITEM(lip));
>  }
>  
>  /*
> @@ -206,24 +225,6 @@ xfs_bui_init(
>  	return buip;
>  }
>  
> -/*
> - * Freeing the BUI requires that we remove it from the AIL if it has already
> - * been placed there. However, the BUI may not yet have been placed in the AIL
> - * when called by xfs_bui_release() from BUD processing due to the ordering of
> - * committed vs unpin operations in bulk insert operations. Hence the reference
> - * count to ensure only the last caller frees the BUI.
> - */
> -void
> -xfs_bui_release(
> -	struct xfs_bui_log_item	*buip)
> -{
> -	ASSERT(atomic_read(&buip->bui_refcount) > 0);
> -	if (atomic_dec_and_test(&buip->bui_refcount)) {
> -		xfs_trans_ail_remove(&buip->bui_item, SHUTDOWN_LOG_IO_ERROR);
> -		xfs_bui_item_free(buip);
> -	}
> -}
> -
>  static inline struct xfs_bud_log_item *BUD_ITEM(struct xfs_log_item *lip)
>  {
>  	return container_of(lip, struct xfs_bud_log_item, bud_item);
> diff --git a/fs/xfs/xfs_extfree_item.c b/fs/xfs/xfs_extfree_item.c
> index d60a9809f0c6..70b7d48af6d6 100644
> --- a/fs/xfs/xfs_extfree_item.c
> +++ b/fs/xfs/xfs_extfree_item.c
> @@ -50,6 +50,24 @@ xfs_efi_item_free(
>  		kmem_zone_free(xfs_efi_zone, efip);
>  }
>  
> +/*
> + * Freeing the efi requires that we remove it from the AIL if it has already
> + * been placed there. However, the EFI may not yet have been placed in the AIL
> + * when called by xfs_efi_release() from EFD processing due to the ordering of
> + * committed vs unpin operations in bulk insert operations. Hence the reference
> + * count to ensure only the last caller frees the EFI.
> + */
> +void
> +xfs_efi_release(
> +	struct xfs_efi_log_item	*efip)
> +{
> +	ASSERT(atomic_read(&efip->efi_refcount) > 0);
> +	if (atomic_dec_and_test(&efip->efi_refcount)) {
> +		xfs_trans_ail_remove(&efip->efi_item, SHUTDOWN_LOG_IO_ERROR);
> +		xfs_efi_item_free(efip);
> +	}
> +}
> +
>  /*
>   * This returns the number of iovecs needed to log the given efi item.
>   * We only need 1 iovec for an efi item.  It just logs the efi_log_format
> @@ -151,7 +169,7 @@ xfs_efi_item_unlock(
>  	struct xfs_log_item	*lip)
>  {
>  	if (test_bit(XFS_LI_ABORTED, &lip->li_flags))
> -		xfs_efi_item_free(EFI_ITEM(lip));
> +		xfs_efi_release(EFI_ITEM(lip));
>  }
>  
>  /*
> @@ -279,24 +297,6 @@ xfs_efi_copy_format(xfs_log_iovec_t *buf, xfs_efi_log_format_t *dst_efi_fmt)
>  	return -EFSCORRUPTED;
>  }
>  
> -/*
> - * Freeing the efi requires that we remove it from the AIL if it has already
> - * been placed there. However, the EFI may not yet have been placed in the AIL
> - * when called by xfs_efi_release() from EFD processing due to the ordering of
> - * committed vs unpin operations in bulk insert operations. Hence the reference
> - * count to ensure only the last caller frees the EFI.
> - */
> -void
> -xfs_efi_release(
> -	struct xfs_efi_log_item	*efip)
> -{
> -	ASSERT(atomic_read(&efip->efi_refcount) > 0);
> -	if (atomic_dec_and_test(&efip->efi_refcount)) {
> -		xfs_trans_ail_remove(&efip->efi_item, SHUTDOWN_LOG_IO_ERROR);
> -		xfs_efi_item_free(efip);
> -	}
> -}
> -
>  static inline struct xfs_efd_log_item *EFD_ITEM(struct xfs_log_item *lip)
>  {
>  	return container_of(lip, struct xfs_efd_log_item, efd_item);
> diff --git a/fs/xfs/xfs_refcount_item.c b/fs/xfs/xfs_refcount_item.c
> index a017024bf249..e5866b714d5f 100644
> --- a/fs/xfs/xfs_refcount_item.c
> +++ b/fs/xfs/xfs_refcount_item.c
> @@ -52,6 +52,25 @@ xfs_cui_item_free(
>  		kmem_zone_free(xfs_cui_zone, cuip);
>  }
>  
> +/*
> + * Freeing the CUI requires that we remove it from the AIL if it has already
> + * been placed there. However, the CUI may not yet have been placed in the AIL
> + * when called by xfs_cui_release() from CUD processing due to the ordering of
> + * committed vs unpin operations in bulk insert operations. Hence the reference
> + * count to ensure only the last caller frees the CUI.
> + */
> +void
> +xfs_cui_release(
> +	struct xfs_cui_log_item	*cuip)
> +{
> +	ASSERT(atomic_read(&cuip->cui_refcount) > 0);
> +	if (atomic_dec_and_test(&cuip->cui_refcount)) {
> +		xfs_trans_ail_remove(&cuip->cui_item, SHUTDOWN_LOG_IO_ERROR);
> +		xfs_cui_item_free(cuip);
> +	}
> +}
> +
> +
>  STATIC void
>  xfs_cui_item_size(
>  	struct xfs_log_item	*lip,
> @@ -141,7 +160,7 @@ xfs_cui_item_unlock(
>  	struct xfs_log_item	*lip)
>  {
>  	if (test_bit(XFS_LI_ABORTED, &lip->li_flags))
> -		xfs_cui_item_free(CUI_ITEM(lip));
> +		xfs_cui_release(CUI_ITEM(lip));
>  }
>  
>  /*
> @@ -211,24 +230,6 @@ xfs_cui_init(
>  	return cuip;
>  }
>  
> -/*
> - * Freeing the CUI requires that we remove it from the AIL if it has already
> - * been placed there. However, the CUI may not yet have been placed in the AIL
> - * when called by xfs_cui_release() from CUD processing due to the ordering of
> - * committed vs unpin operations in bulk insert operations. Hence the reference
> - * count to ensure only the last caller frees the CUI.
> - */
> -void
> -xfs_cui_release(
> -	struct xfs_cui_log_item	*cuip)
> -{
> -	ASSERT(atomic_read(&cuip->cui_refcount) > 0);
> -	if (atomic_dec_and_test(&cuip->cui_refcount)) {
> -		xfs_trans_ail_remove(&cuip->cui_item, SHUTDOWN_LOG_IO_ERROR);
> -		xfs_cui_item_free(cuip);
> -	}
> -}
> -
>  static inline struct xfs_cud_log_item *CUD_ITEM(struct xfs_log_item *lip)
>  {
>  	return container_of(lip, struct xfs_cud_log_item, cud_item);
> diff --git a/fs/xfs/xfs_rmap_item.c b/fs/xfs/xfs_rmap_item.c
> index cbf4ecd81616..e5b5b3e7ef82 100644
> --- a/fs/xfs/xfs_rmap_item.c
> +++ b/fs/xfs/xfs_rmap_item.c
> @@ -52,6 +52,24 @@ xfs_rui_item_free(
>  		kmem_zone_free(xfs_rui_zone, ruip);
>  }
>  
> +/*
> + * Freeing the RUI requires that we remove it from the AIL if it has already
> + * been placed there. However, the RUI may not yet have been placed in the AIL
> + * when called by xfs_rui_release() from RUD processing due to the ordering of
> + * committed vs unpin operations in bulk insert operations. Hence the reference
> + * count to ensure only the last caller frees the RUI.
> + */
> +void
> +xfs_rui_release(
> +	struct xfs_rui_log_item	*ruip)
> +{
> +	ASSERT(atomic_read(&ruip->rui_refcount) > 0);
> +	if (atomic_dec_and_test(&ruip->rui_refcount)) {
> +		xfs_trans_ail_remove(&ruip->rui_item, SHUTDOWN_LOG_IO_ERROR);
> +		xfs_rui_item_free(ruip);
> +	}
> +}
> +
>  STATIC void
>  xfs_rui_item_size(
>  	struct xfs_log_item	*lip,
> @@ -141,7 +159,7 @@ xfs_rui_item_unlock(
>  	struct xfs_log_item	*lip)
>  {
>  	if (test_bit(XFS_LI_ABORTED, &lip->li_flags))
> -		xfs_rui_item_free(RUI_ITEM(lip));
> +		xfs_rui_release(RUI_ITEM(lip));
>  }
>  
>  /*
> @@ -233,24 +251,6 @@ xfs_rui_copy_format(
>  	return 0;
>  }
>  
> -/*
> - * Freeing the RUI requires that we remove it from the AIL if it has already
> - * been placed there. However, the RUI may not yet have been placed in the AIL
> - * when called by xfs_rui_release() from RUD processing due to the ordering of
> - * committed vs unpin operations in bulk insert operations. Hence the reference
> - * count to ensure only the last caller frees the RUI.
> - */
> -void
> -xfs_rui_release(
> -	struct xfs_rui_log_item	*ruip)
> -{
> -	ASSERT(atomic_read(&ruip->rui_refcount) > 0);
> -	if (atomic_dec_and_test(&ruip->rui_refcount)) {
> -		xfs_trans_ail_remove(&ruip->rui_item, SHUTDOWN_LOG_IO_ERROR);
> -		xfs_rui_item_free(ruip);
> -	}
> -}
> -
>  static inline struct xfs_rud_log_item *RUD_ITEM(struct xfs_log_item *lip)
>  {
>  	return container_of(lip, struct xfs_rud_log_item, rud_item);
> -- 
> 2.16.1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html