On 2019/4/30 11:14, Jaegeuk Kim wrote:
> On 04/29, Chao Yu wrote:
>> On 2019/4/28 21:31, Jaegeuk Kim wrote:
>>> On 04/24, Chao Yu wrote:
>>>> On 2019-4-24 17:36, Jaegeuk Kim wrote:
>>>>> On 04/15, Chao Yu wrote:
>>>>>> Previously, f2fs_is_valid_blkaddr(, blkaddr, DATA_GENERIC) will check
>>>>>> whether @blkaddr locates in main area or not.
>>>>>>
>>>>>> That check is weak, since the block address in range of main area can
>>>>>> point to the address which is not valid in segment info table, and we
>>>>>> can not detect such condition, we may suffer worse corruption as system
>>>>>> continues running.
>>>>>>
>>>>>> So this patch introduce DATA_GENERIC_ENHANCE to enhance the sanity check
>>>>>> which trigger SIT bitmap check rather than only range check.
>>>>>>
>>>>>> This patch did below changes as wel:
>>>>>> - set SBI_NEED_FSCK in f2fs_is_valid_blkaddr().
>>>>>> - get rid of is_valid_data_blkaddr() to avoid panic if blkaddr is
>>>>>> invalid.
>>>>>> - introduce verify_fio_blkaddr() to wrap fio {new,old}_blkaddr
>>>>>> validation check.
>>>>>> - spread blkaddr check in:
>>>>>> * f2fs_get_node_info()
>>>>>> * __read_out_blkaddrs()
>>>>>> * f2fs_submit_page_read()
>>>>>> * ra_data_block()
>>>>>> * do_recover_data()
>>>>>>
>>>>>> This patch can fix bug reported from bugzilla below:
>>>>>>
>>>>>> https://bugzilla.kernel.org/show_bug.cgi?id=203215
>>>>>> https://bugzilla.kernel.org/show_bug.cgi?id=203223
>>>>>> https://bugzilla.kernel.org/show_bug.cgi?id=203231
>>>>>> https://bugzilla.kernel.org/show_bug.cgi?id=203235
>>>>>> https://bugzilla.kernel.org/show_bug.cgi?id=203241
>>>>>
>>>>> Hi Chao,
>>>>>
>>>>> This introduces failures on xfstests/generic/446, and I'm testing the
>>>>> below
>>>>> patch on top of this. Could you check this patch, so that I could combine
>>>>> both of them?
>>>>>
>>>>> From 8c1808c1743ad75d1ad8d1dc5a53910edaf7afd7 Mon Sep 17 00:00:00 2001
>>>>> From: Jaegeuk Kim <[email protected]>
>>>>> Date: Wed, 24 Apr 2019 00:21:07 +0100
>>>>> Subject: [PATCH] f2fs: consider data race on read and truncation on
>>>>> DATA_GENERIC_ENHANCE
>>>>>
>>>>> DATA_GENERIC_ENHANCE enhanced to validate block addresses on read/write
>>>>> paths.
>>>>> But, xfstest/generic/446 compalins some generated kernel messages saying
>>>>> invalid
>>>>> bitmap was detected when reading a block. The reaons is, when we get the
>>>>> block addresses from extent_cache, there is no lock to synchronize it from
>>>>> truncating the blocks in parallel.
>>>>>
>>>>> This patch tries to return EFAULT without warning and setting
>>>>> SBI_NEED_FSCK
>>>>> in this case.
>>>>>
>>>>> Fixes: ("f2fs: introduce DATA_GENERIC_ENHANCE")
>>>>> Signed-off-by: Jaegeuk Kim <[email protected]>
>>>>> ---
>>>>> fs/f2fs/checkpoint.c | 35 ++++++++++++++++++-----------------
>>>>> fs/f2fs/data.c | 25 ++++++++++++++++++-------
>>>>> fs/f2fs/f2fs.h | 6 ++++++
>>>>> fs/f2fs/gc.c | 9 ++++++---
>>>>> 4 files changed, 48 insertions(+), 27 deletions(-)
>>>>>
>>>>> diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c
>>>>> index e37fbbf843a5..805a33088e82 100644
>>>>> --- a/fs/f2fs/checkpoint.c
>>>>> +++ b/fs/f2fs/checkpoint.c
>>>>> @@ -130,26 +130,28 @@ struct page *f2fs_get_tmp_page(struct f2fs_sb_info
>>>>> *sbi, pgoff_t index)
>>>>> return __get_meta_page(sbi, index, false);
>>>>> }
>>>>>
>>>>> -static bool __is_bitmap_valid(struct f2fs_sb_info *sbi, block_t blkaddr)
>>>>> +static bool __is_bitmap_valid(struct f2fs_sb_info *sbi, block_t blkaddr,
>>>>> + int type)
>>>>> {
>>>>> struct seg_entry *se;
>>>>> unsigned int segno, offset;
>>>>> bool exist;
>>>>>
>>>>> + if (type != DATA_GENERIC_ENHANCE && type != DATA_GENERIC_ENHANCE_READ)
>>>>> + return true;
>>>>> +
>>>>> segno = GET_SEGNO(sbi, blkaddr);
>>>>> offset = GET_BLKOFF_FROM_SEG0(sbi, blkaddr);
>>>>> se = get_seg_entry(sbi, segno);
>>>>>
>>>>> exist = f2fs_test_bit(offset, se->cur_valid_map);
>>>>> -
>>>>> - if (!exist) {
>>>>> + if (!exist && type == DATA_GENERIC_ENHANCE) {
>>>>> f2fs_msg(sbi->sb, KERN_ERR, "Inconsistent error "
>>>>> "blkaddr:%u, sit bitmap:%d", blkaddr, exist);
>>>>> set_sbi_flag(sbi, SBI_NEED_FSCK);
>>>>> WARN_ON(1);
>>>>> - return false;
>>>>> }
>>>>> - return true;
>>>>> + return exist;
>>>>> }
>>>>>
>>>>> bool f2fs_is_valid_blkaddr(struct f2fs_sb_info *sbi,
>>>>> @@ -173,23 +175,22 @@ bool f2fs_is_valid_blkaddr(struct f2fs_sb_info *sbi,
>>>>> return false;
>>>>> break;
>>>>> case META_POR:
>>>>> + if (unlikely(blkaddr >= MAX_BLKADDR(sbi) ||
>>>>> + blkaddr < MAIN_BLKADDR(sbi)))
>>>>> + return false;
>>>>> + break;
>>>>> case DATA_GENERIC:
>>>>> case DATA_GENERIC_ENHANCE:
>>>>> + case DATA_GENERIC_ENHANCE_READ:
>>>>> if (unlikely(blkaddr >= MAX_BLKADDR(sbi) ||
>>>>> - blkaddr < MAIN_BLKADDR(sbi))) {
>>>>> - if (type == DATA_GENERIC ||
>>>>> - type == DATA_GENERIC_ENHANCE) {
>>>>> - f2fs_msg(sbi->sb, KERN_WARNING,
>>>>> - "access invalid blkaddr:%u", blkaddr);
>>>>> - set_sbi_flag(sbi, SBI_NEED_FSCK);
>>>>> - WARN_ON(1);
>>>>> - }
>>>>> + blkaddr < MAIN_BLKADDR(sbi))) {
>>>>> + f2fs_msg(sbi->sb, KERN_WARNING,
>>>>> + "access invalid blkaddr:%u", blkaddr);
>>>>> + set_sbi_flag(sbi, SBI_NEED_FSCK);
>>>>> + WARN_ON(1);
>>>>> return false;
>>>>> } else {
>>>>> - if (type == DATA_GENERIC_ENHANCE) {
>>>>> - if (!__is_bitmap_valid(sbi, blkaddr))
>>>>> - return false;
>>>>> - }
>>>>> + return __is_bitmap_valid(sbi, blkaddr, type);
>>>>> }
>>>>> break;
>>>>> case META_GENERIC:
>>>>> diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
>>>>> index 34d248ac9e0f..d32a82f25f5a 100644
>>>>> --- a/fs/f2fs/data.c
>>>>> +++ b/fs/f2fs/data.c
>>>>> @@ -564,9 +564,6 @@ static struct bio *f2fs_grab_read_bio(struct inode
>>>>> *inode, block_t blkaddr,
>>>>> struct bio_post_read_ctx *ctx;
>>>>> unsigned int post_read_steps = 0;
>>>>>
>>>>> - if (!f2fs_is_valid_blkaddr(sbi, blkaddr, DATA_GENERIC_ENHANCE))
>>>>> - return ERR_PTR(-EFAULT);
>>>>> -
>>>>> bio = f2fs_bio_alloc(sbi, min_t(int, nr_pages, BIO_MAX_PAGES), false);
>>>>> if (!bio)
>>>>> return ERR_PTR(-ENOMEM);
>>>>> @@ -597,9 +594,6 @@ static int f2fs_submit_page_read(struct inode *inode,
>>>>> struct page *page,
>>>>> struct f2fs_sb_info *sbi = F2FS_I_SB(inode);
>>>>> struct bio *bio;
>>>>>
>>>>> - if (!f2fs_is_valid_blkaddr(sbi, blkaddr, DATA_GENERIC_ENHANCE))
>>>>> - return -EFAULT;
>>>>> -
>>>>> bio = f2fs_grab_read_bio(inode, blkaddr, 1, 0);
>>>>> if (IS_ERR(bio))
>>>>> return PTR_ERR(bio);
>>>>> @@ -741,6 +735,11 @@ struct page *f2fs_get_read_data_page(struct inode
>>>>> *inode, pgoff_t index,
>>>>>
>>>>> if (f2fs_lookup_extent_cache(inode, index, &ei)) {
>>>>> dn.data_blkaddr = ei.blk + index - ei.fofs;
>>>>> + if (!f2fs_is_valid_blkaddr(F2FS_I_SB(inode), dn.data_blkaddr,
>>>>> + DATA_GENERIC_ENHANCE_READ)) {
>>>>
>>>> If I'm not missing anything, we just need use DATA_GENERIC_ENHANCE_READ to
>>>> cover
>>>> below two paths:
>>>> - gc_data_segment -> f2fs_get_read_data_page
>>>> - move_data_page -> f2fs_get_lock_data_page -> f2fs_get_read_data_page
>>>>
>>>> Other paths which calls f2fs_get_read_data_page is safe to verify blkaddr
>>>> with
>>>> DATA_GENERIC_ENHANCE?
>>>
>>> The rule for here is, if block address is given by extent cache, we need to
>>> use
>>> ENHANCE_READ. If it's coming from dnode lock, I think it'd be safe.
>>
>> Okay, I tested this patch with below testcases from bugzilla, it seems there
>> is
>> no regression.
>>
>> https://bugzilla.kernel.org/show_bug.cgi?id=203215
>> https://bugzilla.kernel.org/show_bug.cgi?id=203223
>> https://bugzilla.kernel.org/show_bug.cgi?id=203231
>> https://bugzilla.kernel.org/show_bug.cgi?id=203235
>> https://bugzilla.kernel.org/show_bug.cgi?id=203241
>>
>> One comment below.
>>
>>>
>>>>
>>>>> + err = -EFAULT;
>>>>> + goto put_err;
>>>>> + }
>>>>> goto got_it;
>>>>> }
>>>>>
>>>>> @@ -754,6 +753,13 @@ struct page *f2fs_get_read_data_page(struct inode
>>>>> *inode, pgoff_t index,
>>>>> err = -ENOENT;
>>>>> goto put_err;
>>>>> }
>>>>> + if (dn.data_blkaddr != NEW_ADDR &&
>>>>> + !f2fs_is_valid_blkaddr(F2FS_I_SB(inode),
>>>>> + dn.data_blkaddr,
>>>>> + DATA_GENERIC_ENHANCE)) {
>>>>> + err = -EFAULT;
>>>>> + goto put_err;
>>>>> + }
>>>>> got_it:
>>>>> if (PageUptodate(page)) {
>>>>> unlock_page(page);
>>>>> @@ -1566,7 +1572,7 @@ static int f2fs_read_single_page(struct inode
>>>>> *inode, struct page *page,
>>>>> }
>>>>>
>>>>> if (!f2fs_is_valid_blkaddr(F2FS_I_SB(inode), block_nr,
>>>>> - DATA_GENERIC_ENHANCE)) {
>>>>> + DATA_GENERIC_ENHANCE_READ)) {
>>>>> ret = -EFAULT;
>>>>> goto out;
>>>>> }
>>>>> @@ -2528,6 +2534,11 @@ static int f2fs_write_begin(struct file *file,
>>>>> struct address_space *mapping,
>>>>> zero_user_segment(page, 0, PAGE_SIZE);
>>>>> SetPageUptodate(page);
>>>>> } else {
>>>>> + if (!f2fs_is_valid_blkaddr(sbi, blkaddr,
>>>>> + DATA_GENERIC_ENHANCE_READ)) {
>>
>> Do we need to move the check into prepare_write_begin()? then we can know
>> where
>> the block address comes from, and decide to use DATA_GENERIC_ENHANCE or
>> DATA_GENERIC_ENHANCE_READ.
>
> That makes the code quite messy, since it requires to check NEW_ADDR/NULL_ADDR
> as well as extent_cache in much deeper f2fs_get_block.
Okay, we can keep it unless there is another bug reported in bugzilla.
Thanks,
>
>>
>> Thanks,
>>
>>>>
>>>> Need DATA_GENERIC_ENHANCE because write() is exclusive with truncate() due
>>>> to
>>>> inode_lock()?
>>>>
>>>> Thanks,
>>>>
>>>>> + err = -EFAULT;
>>>>> + goto fail;
>>>>> + }
>>>>> err = f2fs_submit_page_read(inode, page, blkaddr);
>>>>> if (err)
>>>>> goto fail;
>>>>> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
>>>>> index f5ffc09705eb..533fafca68f4 100644
>>>>> --- a/fs/f2fs/f2fs.h
>>>>> +++ b/fs/f2fs/f2fs.h
>>>>> @@ -212,6 +212,12 @@ enum {
>>>>> META_POR,
>>>>> DATA_GENERIC, /* check range only */
>>>>> DATA_GENERIC_ENHANCE, /* strong check on range and segment bitmap */
>>>>> + DATA_GENERIC_ENHANCE_READ, /*
>>>>> + * strong check on range and segment
>>>>> + * bitmap but no warning due to race
>>>>> + * condition of read on truncated area
>>>>> + * by extent_cache
>>>>> + */
>>>>> META_GENERIC,
>>>>> };
>>>>>
>>>>> diff --git a/fs/f2fs/gc.c b/fs/f2fs/gc.c
>>>>> index 3a097949b5d4..963fb4571fd9 100644
>>>>> --- a/fs/f2fs/gc.c
>>>>> +++ b/fs/f2fs/gc.c
>>>>> @@ -656,6 +656,11 @@ static int ra_data_block(struct inode *inode,
>>>>> pgoff_t index)
>>>>>
>>>>> if (f2fs_lookup_extent_cache(inode, index, &ei)) {
>>>>> dn.data_blkaddr = ei.blk + index - ei.fofs;
>>>>> + if (unlikely(!f2fs_is_valid_blkaddr(sbi, dn.data_blkaddr,
>>>>> + DATA_GENERIC_ENHANCE_READ))) {
>>>>> + err = -EFAULT;
>>>>> + goto put_page;
>>>>> + }
>>>>> goto got_it;
>>>>> }
>>>>>
>>>>> @@ -669,14 +674,12 @@ static int ra_data_block(struct inode *inode,
>>>>> pgoff_t index)
>>>>> err = -ENOENT;
>>>>> goto put_page;
>>>>> }
>>>>> -
>>>>> -got_it:
>>>>> if (unlikely(!f2fs_is_valid_blkaddr(sbi, dn.data_blkaddr,
>>>>> DATA_GENERIC_ENHANCE))) {
>>>>> err = -EFAULT;
>>>>> goto put_page;
>>>>> }
>>>>> -
>>>>> +got_it:
>>>>> /* read page */
>>>>> fio.page = page;
>>>>> fio.new_blkaddr = fio.old_blkaddr = dn.data_blkaddr;
>>>>>
>>> .
>>>
> .
>
_______________________________________________
Linux-f2fs-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
