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)) {
+ 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)) {
+ 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;
--
2.19.0.605.g01d371f741-goog
_______________________________________________
Linux-f2fs-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
