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

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

[Dave Chinner]

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);
-- 
2.16.1

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

[Darrick J. Wong]

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

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

[Dave Chinner]

On Mon, Apr 02, 2018 at 08:06:47PM -0700, Darrick J. Wong wrote:
> 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>
.....
> > @@ -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?

Oh, sorry, I just dumped the patch on the end of my stack, which has
this conversion in it (because we do a bunch of racy updates on the
log item flags with any serialisation and that needs to be fixed to
get rid of log item descriptors).

Change them all to

	if (lip->li_flags & XFS_LI_ABORTED)

and it'll apply to the for-next tree.

> 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.

Been running generic/388 in a loop and nothing seems to have gone
wrong yet.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

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

[Allison Henderson]

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>

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

[Brian Foster]

On Tue, Apr 03, 2018 at 02:03:58PM +1000, Dave Chinner wrote:
> On Mon, Apr 02, 2018 at 08:06:47PM -0700, Darrick J. Wong wrote:
> > 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>
> .....
> > > @@ -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?
> 
> Oh, sorry, I just dumped the patch on the end of my stack, which has
> this conversion in it (because we do a bunch of racy updates on the
> log item flags with any serialisation and that needs to be fixed to
> get rid of log item descriptors).
> 
> Change them all to
> 
> 	if (lip->li_flags & XFS_LI_ABORTED)
> 
> and it'll apply to the for-next tree.
> 

This also needs comment updates for all of the unlock handlers that
currently explain why we free the item directly (which should now
explain that the caller is responsible for the additional reference).

Brian

> > 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.
> 
> Been running generic/388 in a loop and nothing seems to have gone
> wrong yet.
> 
> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> david@fromorbit.com
> --
> 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

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

[Luis R. Rodriguez]

On Tue, Apr 03, 2018 at 02:03:58PM +1000, Dave Chinner wrote:
> On Mon, Apr 02, 2018 at 08:06:47PM -0700, Darrick J. Wong wrote:
> > 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>
> .....
> > > @@ -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?
> 
> Oh, sorry, I just dumped the patch on the end of my stack, which has
> this conversion in it (because we do a bunch of racy updates on the
> log item flags with any serialisation and that needs to be fixed to
> get rid of log item descriptors).
> 
> Change them all to
> 
> 	if (lip->li_flags & XFS_LI_ABORTED)
> 
> and it'll apply to the for-next tree.

Just a friendly poke for resubmission, so it doesn't fall through the cracks.

  Luis

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

[Dave Chinner]

On Thu, Apr 19, 2018 at 11:22:19PM +0200, Luis R. Rodriguez wrote:
> On Tue, Apr 03, 2018 at 02:03:58PM +1000, Dave Chinner wrote:
> > On Mon, Apr 02, 2018 at 08:06:47PM -0700, Darrick J. Wong wrote:
> > > 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>
> > .....
> > > > @@ -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?
> > 
> > Oh, sorry, I just dumped the patch on the end of my stack, which has
> > this conversion in it (because we do a bunch of racy updates on the
> > log item flags with any serialisation and that needs to be fixed to
> > get rid of log item descriptors).
> > 
> > Change them all to
> > 
> > 	if (lip->li_flags & XFS_LI_ABORTED)
> > 
> > and it'll apply to the for-next tree.
> 
> Just a friendly poke for resubmission, so it doesn't fall through the cracks.

0612d1166330 xfs: fix intent use-after-free on abort

-Dave.
-- 
Dave Chinner
david@fromorbit.com