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

From: Jaegeuk Kim
Date: Mon Apr 29 2019 - 23:14:38 EST

Next message: Chao Yu: "Re: [PATCH 1/2] f2fs: fix to avoid potential negative .f_bfree"
Previous message: Al Viro: "Re: INFO: task hung in __get_super"
In reply to: Chao Yu: "Re: [PATCH v2 2/2] f2fs: introduce DATA_GENERIC_ENHANCE"
Next in thread: Chao Yu: "Re: [PATCH v2 2/2] f2fs: introduce DATA_GENERIC_ENHANCE"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

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

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.

>
> 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: Chao Yu: "Re: [PATCH 1/2] f2fs: fix to avoid potential negative .f_bfree"
Previous message: Al Viro: "Re: INFO: task hung in __get_super"
In reply to: Chao Yu: "Re: [PATCH v2 2/2] f2fs: introduce DATA_GENERIC_ENHANCE"
Next in thread: Chao Yu: "Re: [PATCH v2 2/2] f2fs: introduce DATA_GENERIC_ENHANCE"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]