Re: [PATCH] btrfs: inode: Fix NULL pointer dereference if inode doesn't need compression

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

[-- Attachment #1.1: Type: text/plain, Size: 3778 bytes --]



On 2020/7/28 下午9:19, David Sterba wrote:
> On Tue, Jul 28, 2020 at 04:39:26PM +0800, Qu Wenruo wrote:
>> [BUG]
>> There is a bug report of NULL pointer dereference caused in
>> compress_file_extent():
>>
>>   Oops: Kernel access of bad area, sig: 11 [#1]
>>   LE PAGE_SIZE=64K MMU=Hash SMP NR_CPUS=2048 NUMA pSeries
>>   Workqueue: btrfs-delalloc btrfs_delalloc_helper [btrfs]
>>   NIP [c008000006dd4d34] compress_file_range.constprop.41+0x75c/0x8a0 [btrfs]
>>   LR [c008000006dd4d1c] compress_file_range.constprop.41+0x744/0x8a0 [btrfs]
>>   Call Trace:
>>   [c000000c69093b00] [c008000006dd4d1c] compress_file_range.constprop.41+0x744/0x8a0 [btrfs] (unreliable)
>>   [c000000c69093bd0] [c008000006dd4ebc] async_cow_start+0x44/0xa0 [btrfs]
>>   [c000000c69093c10] [c008000006e14824] normal_work_helper+0xdc/0x598 [btrfs]
>>   [c000000c69093c80] [c0000000001608c0] process_one_work+0x2c0/0x5b0
>>   [c000000c69093d10] [c000000000160c38] worker_thread+0x88/0x660
>>   [c000000c69093db0] [c00000000016b55c] kthread+0x1ac/0x1c0
>>   [c000000c69093e20] [c00000000000b660] ret_from_kernel_thread+0x5c/0x7c
>>   ---[ end trace f16954aa20d822f6 ]---
>>
>> [CAUSE]
>> For the following execution route of compress_file_range(), it's
>> possible to hit NULL pointer dereference:
>>
>>  compress_file_extent()
>>  |- pages = NULL;
>>  |- start = async_chunk->start = 0;
>>  |- end = async_chunk = 4095;
>>  |- nr_pages = 1;
>>  |- inode_need_compress() == false; <<< Possible, see later explanation
>>  |  Now, we have nr_pages = 1, pages = NULL
>>  |- cont:
>>  |- 		ret = cow_file_range_inline();
>>  |- 		if (ret <= 0) {
>>  |-		for (i = 0; i < nr_pages; i++) {
>>  |-			WARN_ON(pages[i]->mapping);	<<< Crash
>>
>> To enter above call execution branch, we need the following race:
>>
>>     Thread 1 (chattr)     |            Thread 2 (writeback)
>> --------------------------+------------------------------
>>                           | btrfs_run_delalloc_range
>>                           | |- inode_need_compress = true
>>                           | |- cow_file_range_async()
>> btrfs_ioctl_set_flag()    |
>> |- binode_flags |=        |
>>    BTRFS_INODE_NOCOMPRESS |
>>                           | compress_file_range()
>>                           | |- inode_need_compress = false
>>                           | |- nr_page = 1 while pages = NULL
>>                           | |  Then hit the crash
>>
>> [FIX]
>> This patch will fix it by checking @pages before doing accessing it.
>> This patch is only designed as a hot fix and easy to backport.
>>
>> More elegant fix may make btrfs only check inode_need_compress() once to
>> avoid such race, but that would be another story.
> 
> Yeah it gets mistakenly called twice.
> 
>> Fixes: 4d3a800ebb12 ("btrfs: merge nr_pages input and output parameter in compress_pages")
> 
> How does this patch cause the bug?
> 
Sorry, I should explain more on that.

In fact it takes me quite some time to find the proper culprit.

Before that commit, we have @nr_pages_ret initialized to 0 in
compress_file_extent().

If inode_need_compress() returned false in that function, we continue to
the same inline file extent insert,.

But in free_pages_out: tag, we use @nr_pages_nr to free pages, which is
still 0, as it only get initialized to proper values after
btrfs_compress_pages() call, which we skipped due to
inode_need_compress() returned false.

Then free_pages_out: tag will not execute the WARN_ON() and put_pages()
calls, just skip to kfree(pages). And kfree() can handle NULL pointers
without any problem.

Thus a completely sane looking cleanup in fact caused the NULL pointer
dereference regression for race cases.

Thanks,
Qu


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]