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

[Allison Henderson <allison.henderson@oracle.com>]

On 04/02/2018 06:44 PM, 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))
> -		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);
This looks ok to me.  I may need to add something similar to some intent 
code I am working on.  You can add my review.  Thanks!

Reviewed-by: Allison Henderson <allison.henderson@oracle.com>