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:

quoted

On 21/05/07 07:46AM, Qu Wenruo wrote:

quoted

On 2021/4/28 上午7:03, Qu Wenruo wrote:

quoted

[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 <redacted>
Tested-by: Ritesh Harjani <redacted>
Signed-off-by: Qu Wenruo <redacted>
---
   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.

quoted

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.

quoted

quoted

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

quoted

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

quoted

-ritesh