Messages in this thread | | | Subject | Re: [PATCH v2 2/2] f2fs: introduce DATA_GENERIC_ENHANCE | From | Chao Yu <> | Date | Tue, 30 Apr 2019 11:18:43 +0800 |
| |
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 <jaegeuk@kernel.org> >>>>> 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@kernel.org> >>>>> --- >>>>> 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; >>>>> >>> . >>> > . >
| |