Re: [Patch v2 41/42] btrfs: fix the use-after-free bug in writeback subpage helper

[Qu Wenruo <wqu@suse.com>]

On 2021/4/28 上午7:03, Qu Wenruo wrote:
> [BUG]
> There is a possible use-after-free bug when running generic/095.
> 
>   BUG: Unable to handle kernel data access on write at 0x6b6b6b6b6b6b725b
>   Faulting instruction address: 0xc000000000283654
>   c000000000283078 do_raw_spin_unlock+0x88/0x230
>   c0000000012b1e14 _raw_spin_unlock_irqrestore+0x44/0x90
>   c000000000a918dc btrfs_subpage_clear_writeback+0xac/0xe0
>   c0000000009e0458 end_bio_extent_writepage+0x158/0x270
>   c000000000b6fd14 bio_endio+0x254/0x270
>   c0000000009fc0f0 btrfs_end_bio+0x1a0/0x200
>   c000000000b6fd14 bio_endio+0x254/0x270
>   c000000000b781fc blk_update_request+0x46c/0x670
>   c000000000b8b394 blk_mq_end_request+0x34/0x1d0
>   c000000000d82d1c lo_complete_rq+0x11c/0x140
>   c000000000b880a4 blk_complete_reqs+0x84/0xb0
>   c0000000012b2ca4 __do_softirq+0x334/0x680
>   c0000000001dd878 irq_exit+0x148/0x1d0
>   c000000000016f4c do_IRQ+0x20c/0x240
>   c000000000009240 hardware_interrupt_common_virt+0x1b0/0x1c0
> 
> [CAUSE]
> There is very small race window like the following in generic/095.
> 
> 	Thread 1		|		Thread 2
> --------------------------------+------------------------------------
>    end_bio_extent_writepage()	| btrfs_releasepage()
>    |- spin_lock_irqsave()	| |
>    |- end_page_writeback()	| |
>    |				| |- if (PageWriteback() ||...)
>    |				| |- clear_page_extent_mapped()
>    |				|    |- kfree(subpage);
>    |- spin_unlock_irqrestore().
> 
> The race can also happen between writeback and btrfs_invalidatepage(),
> although that would be much harder as btrfs_invalidatepage() has much
> more work to do before the clear_page_extent_mapped() call.
> 
> [FIX]
> For btrfs_subpage_clear_writeback(), we don't really need to put
> end_page_writepage() call into the spinlock critical section.
> 
> By just checking the bitmap in the critical section and call
> end_page_writeback() outside of the critical section, we can avoid such
> use-after-free bug.
> 
> Reported-by: Ritesh Harjani <riteshh@linux.ibm.com>
> Tested-by: Ritesh Harjani <riteshh@linux.ibm.com>
> Signed-off-by: Qu Wenruo <wqu@suse.com>
> ---
>   fs/btrfs/subpage.c | 5 ++++-
>   1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/btrfs/subpage.c b/fs/btrfs/subpage.c
> index 696485ab68a2..c5abf9745c10 100644
> --- a/fs/btrfs/subpage.c
> +++ b/fs/btrfs/subpage.c

Hi Ritesh,

Unfortunately I have to bother you again for testing the latest subpage 
branch.

This particular fix seems to be incomplete, as I have hit several 
BUG_ON()s related to end_page_writeback() called on page without 
writeback flag.

> @@ -420,13 +420,16 @@ void btrfs_subpage_clear_writeback(const struct btrfs_fs_info *fs_info,
>   {
>   	struct btrfs_subpage *subpage = (struct btrfs_subpage *)page->private;
>   	u16 tmp = btrfs_subpage_calc_bitmap(fs_info, page, start, len);
> +	bool finished = false;
>   	unsigned long flags;
>   
>   	spin_lock_irqsave(&subpage->lock, flags);
>   	subpage->writeback_bitmap &= ~tmp;
>   	if (subpage->writeback_bitmap == 0)
> -		end_page_writeback(page);
> +		finished = true;
>   	spin_unlock_irqrestore(&subpage->lock, flags);
> +	if (finished)
> +		end_page_writeback(page);

The race can happen like this:

               T1                  |              T2
----------------------------------+----------------------------------
__extent_writepage()              |
|<< The 1st sector of the page >> |
|- writepage_delalloc()           |
|  Now the subpage range has      |
|  Writeback flag                 |
|- __extent_writepage_io()        |
|  |- submit_extent_page()        | << endio of the 1st sector >>
|                                 | end_bio_extent_writepage()
|<< The 2nd sector of the page >> | |- spin_lock_irqsave()
|- writepage_delalloc()           | |- finished = true
|  |- spin_lock()                 | |- spin_unlock_irqstore()
|  |- set_page_writeback();       | |
|  |- spin_unlock()               | |- end_page_writeback()
|                                 | << Now page has no writeback >>
|- __extent_writepagE_io()        |
    |- submit_extent_page()        | << endio of the 2nd sector >>
                                   | end_bio_extent_writepage()
                                   | |- finished = true;
                                   | |- end_page_writeback()
                                    !!! BUG_ON() triggered !!!

The reproducibility is pretty low, so far I have only hit 3 times such 
BUG_ON().
No special test case number for it, all 3 BUG_ON() happens for different 
test cases.

Thus newer fix will still keep the end_page_writeback() inside the 
spinlock, but btrfs_releasepage() and btrfs_invalidatepage() will "wait" 
for the spinlock to be released before detaching the subpage structure.

Currently the fix runs fine, but extra test will always help.

Thanks,
Qu
>   }
>   
>   void btrfs_subpage_set_ordered(const struct btrfs_fs_info *fs_info,
>