On Tue, Apr 03, 2018 at 11:44:58AM +1000, Dave Chinner wrote:
> From: Dave Chinner <dchinner@xxxxxxxxxx>
>
> 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@xxxxxxxxxx>
> ---
> 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@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
