On 2021/7/30 下午7:08, David Sterba wrote:

On Fri, Jul 30, 2021 at 01:58:57PM +0800, Qu Wenruo wrote:

quoted

Function set_page_extent_mapped() will setup the data page cache so that
for subpage cases those pages will have page->private to store subpage
specific info.

Normally this happens when we create a new page for the page cache.
But there is a special call site, __extent_writepage(), as we have
special cases where upper layer can mark some page dirty without going
through set_page_dirty() interface.

I haven't yet seen any real world case for this, but if that's possible
then in __extent_writepage() we will call btrfs_page_clear_error()
before setting up the page->private, which can lead to NULL pointer
dereference.

Yeah it's hard to believe, but it's been there since almost the
beginning. Back then there was a hard BUG() in the fixup worker, I've
hit it randomly on x86_64,

https://lore.kernel.org/linux-btrfs/20111031154139.GF19328@twin.jikos.cz/ (local)

you could find a lot of other reports where it crashed inside
btrfs_writepage_fixup_worker.

Well, over 10 years ago.

But finally I have seen a real world report for it.

This makes me wonder, wouldn't other sites like iomap, which also 
utilize a private structure for subpage bitmaps, also hit such crash?

quoted

Fix it by moving set_page_extent_mapped() call before
btrfs_page_clear_error().
And make sure in the error path we won't call anything subpage helper.

I'm not sure about the fix, because the whole fixup thing is not
entirely clear.

Indeed, but the idea should be straightfoward:

Don't call any subpage helper before set_page_extent_mapped().

So that such page without private get passed in, we can still handle it 
well.

quoted

Fixes: 32443de3382b ("btrfs: introduce btrfs_subpage for data inodes")
Signed-off-by: Qu Wenruo <redacted>
---
I really hope we can have a more explicit comment about in exactly which
cases we can have such page, and maybe some test cases for it.

The only reliable test case was on s390 with a particular seed for fsx,
I still have it stored somewhere. On x86_64 it's very hard to hit.

Can it be reproduced by S390 qemu tcc emulation?

And by S390, did you mean modern Power8/9/10 systems too?

quoted

In fact, I haven't really seen any case like this, and it doesn't really
make sense for me to make some MM layer code to mark a page dirty
without going through set_page_dirty() interface.

On s390 it's quick because the page state bits are stored in 2 places
and need to be synced. On x86_64 it's very unclear and low level arch
specific MM stuff but it is still a problem.

The hard to hit part is really harming our test coverage...

Thanks,
Qu