From: Qu Wenruo <quwenruo.btrfs@gmx.com>
To: Nikolay Borisov <nborisov@suse.com>, Qu Wenruo <wqu@suse.com>,
linux-btrfs@vger.kernel.org
Cc: Luciano Chavez <chavez@us.ibm.com>
Subject: Re: [PATCH] btrfs: inode: Fix NULL pointer dereference if inode doesn't need compression
Date: Mon, 3 Aug 2020 07:14:22 +0800 [thread overview]
Message-ID: <25e2bcc7-efb8-f9bc-ac00-c8d5f5bbba53@gmx.com> (raw)
In-Reply-To: <6b8fa62c-0c42-a49b-3961-b247ef8abeb2@suse.com>
On 2020/8/3 上午3:16, Nikolay Borisov wrote:
>
>
> On 28.07.20 г. 11:39 ч., 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.
>
> So why not do the elegant fix in the first place rather than adding
> cruft like this hotfix which later has to be cleaned up when the
> 'proper' fix lands?
For backport purpose.
This is reported from one vendor kernel, not upstream.
Thus backport is definitely required.
Thanks,
Qu
>
>>
>> Fixes: 4d3a800ebb12 ("btrfs: merge nr_pages input and output parameter in compress_pages")
>> Reported-by: Luciano Chavez <chavez@us.ibm.com>
>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>> ---
>> fs/btrfs/inode.c | 16 +++++++++++-----
>> 1 file changed, 11 insertions(+), 5 deletions(-)
>>
>> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
>> index 611b3412fbfd..9988d754e465 100644
>> --- a/fs/btrfs/inode.c
>> +++ b/fs/btrfs/inode.c
>> @@ -653,12 +653,18 @@ static noinline int compress_file_range(struct async_chunk *async_chunk)
>> page_error_op |
>> PAGE_END_WRITEBACK);
>>
>> - for (i = 0; i < nr_pages; i++) {
>> - WARN_ON(pages[i]->mapping);
>> - put_page(pages[i]);
>> + /*
>> + * Ensure we only free the compressed pages if we have
>> + * them allocated, as we can still reach here with
>> + * inode_need_compress() == false.
>> + */
>> + if (pages) {
>> + for (i = 0; i < nr_pages; i++) {
>> + WARN_ON(pages[i]->mapping);
>> + put_page(pages[i]);
>> + }
>> + kfree(pages);
>> }
>> - kfree(pages);
>> -
>> return 0;
>> }
>> }
>>
next prev parent reply other threads:[~2020-08-02 23:22 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-07-28 8:39 [PATCH] btrfs: inode: Fix NULL pointer dereference if inode doesn't need compression Qu Wenruo
2020-07-28 13:19 ` David Sterba
2020-07-28 13:26 ` Qu Wenruo
2020-07-28 13:41 ` David Sterba
2020-08-02 19:16 ` Nikolay Borisov
2020-08-02 23:14 ` Qu Wenruo [this message]
2020-08-25 15:03 ` David Sterba
2020-08-04 6:41 ` Qu Wenruo
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=25e2bcc7-efb8-f9bc-ac00-c8d5f5bbba53@gmx.com \
--to=quwenruo.btrfs@gmx.com \
--cc=chavez@us.ibm.com \
--cc=linux-btrfs@vger.kernel.org \
--cc=nborisov@suse.com \
--cc=wqu@suse.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).