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

[Qu Wenruo <quwenruo.btrfs@gmx.com>]

Hi Ritesh,

I guess no error report so far is a good thing?

Just to report what my result is, I ran my latest github branch for the
full weekend, over 50 hours, and around 20 runs of full generic/auto
without defrag groups.

And I see no crash at all.

But there is a special note, there is a new patch, introduced just
before the weekend (Fri May 7 09:31:43 2021 +0800), titled "btrfs: fix a
possible use-after-free race in metadata read path", is a new fix for a
bug I reproduced once locally.

The bug should only happen when read is slow and only happens for
metadata read path.

The details can be found in the commit message, although it's rare to
hit, I have hit such problem around 3 times in total.

Hopes you didn't hit any crash during your test.

Thanks,
Qu


On 2021/5/7 下午1:14, Qu Wenruo wrote:
>
>
> On 2021/5/7 下午12:57, Ritesh Harjani wrote:
>> On 21/05/07 07:46AM, Qu Wenruo wrote:
>>>
>>>
>>> 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.
>>
>> Yes, this was anyway on my mind to test the latest subpage branch.
>> Sure, I will do the testing.
>>
>>>
>>> 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.
>>
>> ok.
>>
>>>
>>>> @@ -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.
>>
>> Sorry, just to be clear, do you mean the latest subpage branch still
>> has some issues where we can hit the BUG_ON() or have you identifed
>> and added
>> some patches to fix it?
>
> Above race is how the old fix (with end_page_writeback() called outside
> of the spinlock) could lead to a BUG_ON().
>
> I believe the new fix, with the same title, can fix the problem.
>
>>
>> Let me clone below branch and re-test xfstests on Power.
>> https://github.com/adam900710/linux/commits/subpage
>>
>> Also if you would like me to test any extra mount option or mkfs
>> option testing
>> too, then pls do let me know. For now I will be testing with default
>> options.
>
> Thanks, let's just focus on the default mount option first.
>
> Thanks for your great help!
> Qu
>
>>
>> -ritesh
>>