Re: [PATCH v2 2/2] f2fs: introduce DATA_GENERIC_ENHANCE

From: Chao Yu
Date: Mon Apr 29 2019 - 05:03:46 EST

Next message: Stephen Rothwell: "linux-next: Tree for Apr 29"
Previous message: Jose Abreu: "RE: [PATCH 1/2] net-next: stmmac: add support for hash table size 128/256 in dwmac4"
In reply to: Jaegeuk Kim: "Re: [PATCH v2 2/2] f2fs: introduce DATA_GENERIC_ENHANCE"
Next in thread: Jaegeuk Kim: "Re: [PATCH v2 2/2] f2fs: introduce DATA_GENERIC_ENHANCE"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

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

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

Next message: Stephen Rothwell: "linux-next: Tree for Apr 29"
Previous message: Jose Abreu: "RE: [PATCH 1/2] net-next: stmmac: add support for hash table size 128/256 in dwmac4"
In reply to: Jaegeuk Kim: "Re: [PATCH v2 2/2] f2fs: introduce DATA_GENERIC_ENHANCE"
Next in thread: Jaegeuk Kim: "Re: [PATCH v2 2/2] f2fs: introduce DATA_GENERIC_ENHANCE"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]