xfs: fix intent use-after-free on abort
PATCH 1/1 xfs: fix intent use-after-free on abort
From: Dave Chinner <[email protected]>
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.
--- 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(-)
fs/xfs/xfs_bmap_item.c +20 -19
@@ -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);
fs/xfs/xfs_extfree_item.c +19 -19
@@ -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);
fs/xfs/xfs_refcount_item.c +20 -19
@@ -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);
fs/xfs/xfs_rmap_item.c +19 -19
@@ -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);
Signature · 1 line
2.16.1
Quoted 68 lines · Dave Chinner
From: Dave Chinner <[email protected]>
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/0x1690The problem is that the RUI has two active references - one in the current transaction, and another held by the
defer_opsstructure 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_opsreference is released separately via ->abort_intent callback. Fix all the intent code to do this correctly.Signed-off-by Dave Chinner <[email protected]>--- 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(-)fs/xfs/xfs_bmap_item.c +19
-0@@ -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,
fs/xfs/xfs_bmap_item.c xfs_bui_item_unlock( · line 163
Quoted 3 lines
fs/xfs/xfs_bmap_item.c +0
-0@@ -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
Quoted 227 lines
fs/xfs/xfs_bmap_item.c +1
-19-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);fs/xfs/xfs_extfree_item.c +19
-19@@ -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);fs/xfs/xfs_refcount_item.c +20
-19@@ -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);fs/xfs/xfs_rmap_item.c +19
-19@@ -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);Signature · 6 lines
2.16.1 -- To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Quoted 31 lines · Darrick J. Wong
Quoted 30 lines · Dave Chinner
From: Dave Chinner <[email protected]>
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/0x1690The problem is that the RUI has two active references - one in the current transaction, and another held by the
defer_opsstructure 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_opsreference is released separately via ->abort_intent callback. Fix all the intent code to do this correctly.Signed-off-by Dave Chinner <[email protected]>
.....
Quoted 7 lines
fs/xfs/xfs_bmap_item.c xfs_bui_item_unlock( · line 163
Quoted 4 lines
fs/xfs/xfs_bmap_item.c +0
-0@@ -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 thetest_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.
Quoted 3 lines
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.
Signature · 2 lines
Dave Chinner [email protected]
Quoted 46 lines · Dave Chinner
Quoted 31 lines · Darrick J. Wong
Quoted 30 lines · Dave Chinner
From: Dave Chinner <[email protected]>
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/0x1690The problem is that the RUI has two active references - one in the current transaction, and another held by the
defer_opsstructure 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_opsreference is released separately via ->abort_intent callback. Fix all the intent code to do this correctly.Signed-off-by Dave Chinner <[email protected]>.....
Quoted 7 lines
fs/xfs/xfs_bmap_item.c xfs_bui_item_unlock( · line 163
Quoted 4 lines
fs/xfs/xfs_bmap_item.c +0
-0@@ -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 thetest_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).
Quoted 6 lines
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
Quoted 17 lines
Quoted 3 lines
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.
Signature · 6 lines
Dave Chinner [email protected] -- To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Quoted 46 lines · Dave Chinner
Quoted 31 lines · Darrick J. Wong
Quoted 30 lines · Dave Chinner
From: Dave Chinner <[email protected]>
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/0x1690The problem is that the RUI has two active references - one in the current transaction, and another held by the
defer_opsstructure 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_opsreference is released separately via ->abort_intent callback. Fix all the intent code to do this correctly.Signed-off-by Dave Chinner <[email protected]>.....
Quoted 7 lines
fs/xfs/xfs_bmap_item.c xfs_bui_item_unlock( · line 163
Quoted 4 lines
fs/xfs/xfs_bmap_item.c +0
-0@@ -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 thetest_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).
Quoted 5 lines
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
Quoted 47 lines · Luis R. Rodriguez
Quoted 46 lines · Dave Chinner
Quoted 31 lines · Darrick J. Wong
Quoted 30 lines · Dave Chinner
From: Dave Chinner <[email protected]>
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/0x1690The problem is that the RUI has two active references - one in the current transaction, and another held by the
defer_opsstructure 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_opsreference is released separately via ->abort_intent callback. Fix all the intent code to do this correctly.Signed-off-by Dave Chinner <[email protected]>.....
Quoted 7 lines
fs/xfs/xfs_bmap_item.c xfs_bui_item_unlock( · line 163
Quoted 4 lines
fs/xfs/xfs_bmap_item.c +0
-0@@ -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 thetest_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).
Quoted 7 lines
Quoted 5 lines
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.
Signature · 2 lines
Dave Chinner [email protected]
Quoted 281 lines · Dave Chinner
From: Dave Chinner <[email protected]>
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/0x1690The problem is that the RUI has two active references - one in the current transaction, and another held by the
defer_opsstructure 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_opsreference is released separately via ->abort_intent callback. Fix all the intent code to do this correctly.Signed-off-by Dave Chinner <[email protected]>--- 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(-)fs/xfs/xfs_bmap_item.c +20
-19@@ -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);fs/xfs/xfs_extfree_item.c +19
-19@@ -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);fs/xfs/xfs_refcount_item.c +20
-19@@ -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);fs/xfs/xfs_rmap_item.c +19
-12@@ -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) -{
fs/xfs/xfs_rmap_item.c xfs_rui_copy_format( · line 256
Quoted 10 lines
fs/xfs/xfs_rmap_item.c +0
-7-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!