xfs: fix intent use-after-free on abort

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>
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>

diff --git a/fs/xfs/xfs_bmap_item.c b/fs/xfs/xfs_bmap_item.c
index e5fb008d75e899aaa26948ae5044e55e59ca6f11..2203465e63eab05e678b5ee9f2bfd30890468e67 100644 (file)
--- 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 (lip->li_flags & XFS_LI_ABORTED)
-               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 64da90655e957c3fd01331720aa32093909ddad6..b5b1e567b9f4b17a6dad5aee722b49101e0724c8 100644 (file)
--- 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 (lip->li_flags & XFS_LI_ABORTED)
-               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 7a39f40645f7dddbd41740bce4404dbf36fd635b..15c9393dd7a7869f140c146d9d5ea884e8f7e193 100644 (file)
--- 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 (lip->li_flags & XFS_LI_ABORTED)
-               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 49d3124863a81f719efd1774b0b6ac7f651b0a26..06a07846c9b3155f466cad7a609214b96e0f7195 100644 (file)
--- 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 (lip->li_flags & XFS_LI_ABORTED)
-               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);