* [PATCH v2 1/2] f2fs: fix wrong __is_meta_io() macro
@ 2019-04-15 7:26 ` Chao Yu
0 siblings, 0 replies; 13+ messages in thread
From: Chao Yu @ 2019-04-15 7:26 UTC (permalink / raw)
To: jaegeuk; +Cc: linux-f2fs-devel, linux-kernel, chao, Chao Yu
This patch changes codes as below:
- don't use is_read_io() as a condition to judge the meta IO.
- use .is_por to replace .is_meta to indicate IO is from recovery explicitly.
Signed-off-by: Chao Yu <yuchao0@huawei.com>
---
v2: rebase the code to last dev branch.
fs/f2fs/checkpoint.c | 4 ++--
fs/f2fs/data.c | 3 ++-
fs/f2fs/f2fs.h | 5 ++---
3 files changed, 6 insertions(+), 6 deletions(-)
diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c
index 935ebdb9cf47..f42b0015724b 100644
--- a/fs/f2fs/checkpoint.c
+++ b/fs/f2fs/checkpoint.c
@@ -66,7 +66,7 @@ static struct page *__get_meta_page(struct f2fs_sb_info *sbi, pgoff_t index,
.old_blkaddr = index,
.new_blkaddr = index,
.encrypted_page = NULL,
- .is_meta = is_meta,
+ .is_por = !is_meta,
};
int err;
@@ -189,7 +189,7 @@ int f2fs_ra_meta_pages(struct f2fs_sb_info *sbi, block_t start, int nrpages,
.op_flags = sync ? (REQ_META | REQ_PRIO) : REQ_RAHEAD,
.encrypted_page = NULL,
.in_list = false,
- .is_meta = (type != META_POR),
+ .is_por = (type == META_POR),
};
struct blk_plug plug;
diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
index 9d3c11e09a03..eabd34a79451 100644
--- a/fs/f2fs/data.c
+++ b/fs/f2fs/data.c
@@ -456,7 +456,8 @@ int f2fs_submit_page_bio(struct f2fs_io_info *fio)
fio->encrypted_page : fio->page;
if (!f2fs_is_valid_blkaddr(fio->sbi, fio->new_blkaddr,
- __is_meta_io(fio) ? META_GENERIC : DATA_GENERIC))
+ fio->is_por ? META_POR :
+ (__is_meta_io(fio) ? META_GENERIC : DATA_GENERIC)))
return -EFAULT;
trace_f2fs_submit_page_bio(page, fio);
diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
index d5478f94cd25..003a07bd56e3 100644
--- a/fs/f2fs/f2fs.h
+++ b/fs/f2fs/f2fs.h
@@ -1041,7 +1041,7 @@ struct f2fs_io_info {
bool submitted; /* indicate IO submission */
int need_lock; /* indicate we need to lock cp_rwsem */
bool in_list; /* indicate fio is in io_list */
- bool is_meta; /* indicate borrow meta inode mapping or not */
+ bool is_por; /* indicate IO is from recovery or not */
bool retry; /* need to reallocate block address */
enum iostat_type io_type; /* io type */
struct writeback_control *io_wbc; /* writeback control */
@@ -2814,8 +2814,7 @@ static inline void f2fs_update_iostat(struct f2fs_sb_info *sbi,
#define __is_large_section(sbi) ((sbi)->segs_per_sec > 1)
-#define __is_meta_io(fio) (PAGE_TYPE_OF_BIO((fio)->type) == META && \
- (!is_read_io((fio)->op) || (fio)->is_meta))
+#define __is_meta_io(fio) (PAGE_TYPE_OF_BIO((fio)->type) == META)
bool f2fs_is_valid_blkaddr(struct f2fs_sb_info *sbi,
block_t blkaddr, int type);
--
2.18.0.rc1
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH v2 1/2] f2fs: fix wrong __is_meta_io() macro
@ 2019-04-15 7:26 ` Chao Yu
0 siblings, 0 replies; 13+ messages in thread
From: Chao Yu @ 2019-04-15 7:26 UTC (permalink / raw)
To: jaegeuk; +Cc: linux-f2fs-devel, linux-kernel, chao, Chao Yu
This patch changes codes as below:
- don't use is_read_io() as a condition to judge the meta IO.
- use .is_por to replace .is_meta to indicate IO is from recovery explicitly.
Signed-off-by: Chao Yu <yuchao0@huawei.com>
---
v2: rebase the code to last dev branch.
fs/f2fs/checkpoint.c | 4 ++--
fs/f2fs/data.c | 3 ++-
fs/f2fs/f2fs.h | 5 ++---
3 files changed, 6 insertions(+), 6 deletions(-)
diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c
index 935ebdb9cf47..f42b0015724b 100644
--- a/fs/f2fs/checkpoint.c
+++ b/fs/f2fs/checkpoint.c
@@ -66,7 +66,7 @@ static struct page *__get_meta_page(struct f2fs_sb_info *sbi, pgoff_t index,
.old_blkaddr = index,
.new_blkaddr = index,
.encrypted_page = NULL,
- .is_meta = is_meta,
+ .is_por = !is_meta,
};
int err;
@@ -189,7 +189,7 @@ int f2fs_ra_meta_pages(struct f2fs_sb_info *sbi, block_t start, int nrpages,
.op_flags = sync ? (REQ_META | REQ_PRIO) : REQ_RAHEAD,
.encrypted_page = NULL,
.in_list = false,
- .is_meta = (type != META_POR),
+ .is_por = (type == META_POR),
};
struct blk_plug plug;
diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
index 9d3c11e09a03..eabd34a79451 100644
--- a/fs/f2fs/data.c
+++ b/fs/f2fs/data.c
@@ -456,7 +456,8 @@ int f2fs_submit_page_bio(struct f2fs_io_info *fio)
fio->encrypted_page : fio->page;
if (!f2fs_is_valid_blkaddr(fio->sbi, fio->new_blkaddr,
- __is_meta_io(fio) ? META_GENERIC : DATA_GENERIC))
+ fio->is_por ? META_POR :
+ (__is_meta_io(fio) ? META_GENERIC : DATA_GENERIC)))
return -EFAULT;
trace_f2fs_submit_page_bio(page, fio);
diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
index d5478f94cd25..003a07bd56e3 100644
--- a/fs/f2fs/f2fs.h
+++ b/fs/f2fs/f2fs.h
@@ -1041,7 +1041,7 @@ struct f2fs_io_info {
bool submitted; /* indicate IO submission */
int need_lock; /* indicate we need to lock cp_rwsem */
bool in_list; /* indicate fio is in io_list */
- bool is_meta; /* indicate borrow meta inode mapping or not */
+ bool is_por; /* indicate IO is from recovery or not */
bool retry; /* need to reallocate block address */
enum iostat_type io_type; /* io type */
struct writeback_control *io_wbc; /* writeback control */
@@ -2814,8 +2814,7 @@ static inline void f2fs_update_iostat(struct f2fs_sb_info *sbi,
#define __is_large_section(sbi) ((sbi)->segs_per_sec > 1)
-#define __is_meta_io(fio) (PAGE_TYPE_OF_BIO((fio)->type) == META && \
- (!is_read_io((fio)->op) || (fio)->is_meta))
+#define __is_meta_io(fio) (PAGE_TYPE_OF_BIO((fio)->type) == META)
bool f2fs_is_valid_blkaddr(struct f2fs_sb_info *sbi,
block_t blkaddr, int type);
--
2.18.0.rc1
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH v2 2/2] f2fs: introduce DATA_GENERIC_ENHANCE
2019-04-15 7:26 ` Chao Yu
@ 2019-04-15 7:26 ` Chao Yu
-1 siblings, 0 replies; 13+ messages in thread
From: Chao Yu @ 2019-04-15 7:26 UTC (permalink / raw)
To: jaegeuk; +Cc: linux-f2fs-devel, linux-kernel, chao, Chao Yu
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
Signed-off-by: Chao Yu <yuchao0@huawei.com>
---
v2:
- update issue list this patch can fix.
- check blkaddr in do_recover_data().
- rebase code to last dev branch.
fs/f2fs/checkpoint.c | 32 +++++++++++++++++++++++++++++++-
fs/f2fs/data.c | 34 +++++++++++++++++++---------------
fs/f2fs/f2fs.h | 12 ++----------
fs/f2fs/file.c | 15 ++++++++++++---
fs/f2fs/gc.c | 10 ++++++++--
fs/f2fs/inode.c | 7 ++++---
fs/f2fs/node.c | 13 ++++++++++---
fs/f2fs/recovery.c | 12 ++++++++++++
fs/f2fs/segment.c | 4 ++--
fs/f2fs/segment.h | 13 +++++++------
10 files changed, 107 insertions(+), 45 deletions(-)
diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c
index f42b0015724b..a7ad1b1e5750 100644
--- a/fs/f2fs/checkpoint.c
+++ b/fs/f2fs/checkpoint.c
@@ -130,6 +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)
+{
+ struct seg_entry *se;
+ unsigned int segno, offset;
+ bool exist;
+
+ 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) {
+ 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;
+}
+
bool f2fs_is_valid_blkaddr(struct f2fs_sb_info *sbi,
block_t blkaddr, int type)
{
@@ -152,14 +174,22 @@ bool f2fs_is_valid_blkaddr(struct f2fs_sb_info *sbi,
break;
case META_POR:
case DATA_GENERIC:
+ case DATA_GENERIC_ENHANCE:
if (unlikely(blkaddr >= MAX_BLKADDR(sbi) ||
blkaddr < MAIN_BLKADDR(sbi))) {
- if (type == DATA_GENERIC) {
+ 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);
}
return false;
+ } else {
+ if (type == DATA_GENERIC_ENHANCE) {
+ if (!__is_bitmap_valid(sbi, blkaddr))
+ return false;
+ }
}
break;
case META_GENERIC:
diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
index eabd34a79451..ee5c7962b0f3 100644
--- a/fs/f2fs/data.c
+++ b/fs/f2fs/data.c
@@ -456,8 +456,8 @@ int f2fs_submit_page_bio(struct f2fs_io_info *fio)
fio->encrypted_page : fio->page;
if (!f2fs_is_valid_blkaddr(fio->sbi, fio->new_blkaddr,
- fio->is_por ? META_POR :
- (__is_meta_io(fio) ? META_GENERIC : DATA_GENERIC)))
+ fio->is_por ? META_POR : (__is_meta_io(fio) ?
+ META_GENERIC : DATA_GENERIC_ENHANCE)))
return -EFAULT;
trace_f2fs_submit_page_bio(page, fio);
@@ -507,9 +507,7 @@ void f2fs_submit_page_write(struct f2fs_io_info *fio)
spin_unlock(&io->io_lock);
}
- if (__is_valid_data_blkaddr(fio->old_blkaddr))
- verify_block_addr(fio, fio->old_blkaddr);
- verify_block_addr(fio, fio->new_blkaddr);
+ verify_fio_blkaddr(fio);
bio_page = fio->encrypted_page ? fio->encrypted_page : fio->page;
@@ -566,7 +564,7 @@ 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))
+ 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);
@@ -596,8 +594,13 @@ static struct bio *f2fs_grab_read_bio(struct inode *inode, block_t blkaddr,
static int f2fs_submit_page_read(struct inode *inode, struct page *page,
block_t blkaddr)
{
- struct bio *bio = f2fs_grab_read_bio(inode, blkaddr, 1, 0);
+ 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);
@@ -609,8 +612,8 @@ static int f2fs_submit_page_read(struct inode *inode, struct page *page,
return -EFAULT;
}
ClearPageError(page);
- inc_page_count(F2FS_I_SB(inode), F2FS_RD_DATA);
- __submit_bio(F2FS_I_SB(inode), bio, DATA);
+ inc_page_count(sbi, F2FS_RD_DATA);
+ __submit_bio(sbi, bio, DATA);
return 0;
}
@@ -1093,12 +1096,12 @@ int f2fs_map_blocks(struct inode *inode, struct f2fs_map_blocks *map,
blkaddr = datablock_addr(dn.inode, dn.node_page, dn.ofs_in_node);
if (__is_valid_data_blkaddr(blkaddr) &&
- !f2fs_is_valid_blkaddr(sbi, blkaddr, DATA_GENERIC)) {
+ !f2fs_is_valid_blkaddr(sbi, blkaddr, DATA_GENERIC_ENHANCE)) {
err = -EFAULT;
goto sync_out;
}
- if (is_valid_data_blkaddr(sbi, blkaddr)) {
+ if (__is_valid_data_blkaddr(blkaddr)) {
/* use out-place-update for driect IO under LFS mode */
if (test_opt(sbi, LFS) && flag == F2FS_GET_BLOCK_DIO &&
map->m_may_create) {
@@ -1591,7 +1594,7 @@ static int f2fs_mpage_readpages(struct address_space *mapping,
}
if (!f2fs_is_valid_blkaddr(F2FS_I_SB(inode), block_nr,
- DATA_GENERIC))
+ DATA_GENERIC_ENHANCE))
goto set_error_page;
} else {
zero_out:
@@ -1822,7 +1825,7 @@ int f2fs_do_write_data_page(struct f2fs_io_info *fio)
fio->old_blkaddr = ei.blk + page->index - ei.fofs;
if (!f2fs_is_valid_blkaddr(fio->sbi, fio->old_blkaddr,
- DATA_GENERIC))
+ DATA_GENERIC_ENHANCE))
return -EFAULT;
ipu_force = true;
@@ -1849,7 +1852,7 @@ int f2fs_do_write_data_page(struct f2fs_io_info *fio)
got_it:
if (__is_valid_data_blkaddr(fio->old_blkaddr) &&
!f2fs_is_valid_blkaddr(fio->sbi, fio->old_blkaddr,
- DATA_GENERIC)) {
+ DATA_GENERIC_ENHANCE)) {
err = -EFAULT;
goto out_writepage;
}
@@ -1857,7 +1860,8 @@ int f2fs_do_write_data_page(struct f2fs_io_info *fio)
* If current allocation needs SSR,
* it had better in-place writes for updated data.
*/
- if (ipu_force || (is_valid_data_blkaddr(fio->sbi, fio->old_blkaddr) &&
+ if (ipu_force ||
+ (__is_valid_data_blkaddr(fio->old_blkaddr) &&
need_inplace_update(fio))) {
err = encrypt_one_page(fio);
if (err)
diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
index 003a07bd56e3..691452448196 100644
--- a/fs/f2fs/f2fs.h
+++ b/fs/f2fs/f2fs.h
@@ -210,7 +210,8 @@ enum {
META_SSA,
META_MAX,
META_POR,
- DATA_GENERIC,
+ DATA_GENERIC, /* check range only */
+ DATA_GENERIC_ENHANCE, /* strong check on range and segment bitmap */
META_GENERIC,
};
@@ -2837,15 +2838,6 @@ static inline bool __is_valid_data_blkaddr(block_t blkaddr)
return true;
}
-static inline bool is_valid_data_blkaddr(struct f2fs_sb_info *sbi,
- block_t blkaddr)
-{
- if (!__is_valid_data_blkaddr(blkaddr))
- return false;
- verify_blkaddr(sbi, blkaddr, DATA_GENERIC);
- return true;
-}
-
static inline void f2fs_set_page_private(struct page *page,
unsigned long data)
{
diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
index 65e14ec7366f..f19a2ec9f0ef 100644
--- a/fs/f2fs/file.c
+++ b/fs/f2fs/file.c
@@ -358,7 +358,7 @@ static bool __found_offset(struct f2fs_sb_info *sbi, block_t blkaddr,
switch (whence) {
case SEEK_DATA:
if ((blkaddr == NEW_ADDR && dirty == pgofs) ||
- is_valid_data_blkaddr(sbi, blkaddr))
+ __is_valid_data_blkaddr(blkaddr))
return true;
break;
case SEEK_HOLE:
@@ -424,7 +424,7 @@ static loff_t f2fs_seek_block(struct file *file, loff_t offset, int whence)
if (__is_valid_data_blkaddr(blkaddr) &&
!f2fs_is_valid_blkaddr(F2FS_I_SB(inode),
- blkaddr, DATA_GENERIC)) {
+ blkaddr, DATA_GENERIC_ENHANCE)) {
f2fs_put_dnode(&dn);
goto fail;
}
@@ -525,7 +525,8 @@ void f2fs_truncate_data_blocks_range(struct dnode_of_data *dn, int count)
f2fs_set_data_blkaddr(dn);
if (__is_valid_data_blkaddr(blkaddr) &&
- !f2fs_is_valid_blkaddr(sbi, blkaddr, DATA_GENERIC))
+ !f2fs_is_valid_blkaddr(sbi, blkaddr,
+ DATA_GENERIC_ENHANCE))
continue;
f2fs_invalidate_blocks(sbi, blkaddr);
@@ -1019,6 +1020,14 @@ static int __read_out_blkaddrs(struct inode *inode, block_t *blkaddr,
for (i = 0; i < done; i++, blkaddr++, do_replace++, dn.ofs_in_node++) {
*blkaddr = datablock_addr(dn.inode,
dn.node_page, dn.ofs_in_node);
+
+ if (__is_valid_data_blkaddr(*blkaddr) &&
+ !f2fs_is_valid_blkaddr(sbi, *blkaddr,
+ DATA_GENERIC_ENHANCE)) {
+ f2fs_put_dnode(&dn);
+ return -EFAULT;
+ }
+
if (!f2fs_is_checkpointed_data(sbi, *blkaddr)) {
if (test_opt(sbi, LFS)) {
diff --git a/fs/f2fs/gc.c b/fs/f2fs/gc.c
index a66a8752e5f6..45781a1fc680 100644
--- a/fs/f2fs/gc.c
+++ b/fs/f2fs/gc.c
@@ -665,12 +665,18 @@ static int ra_data_block(struct inode *inode, pgoff_t index)
goto put_page;
f2fs_put_dnode(&dn);
+ if (!__is_valid_data_blkaddr(dn.data_blkaddr)) {
+ err = -ENOENT;
+ goto put_page;
+ }
+
+got_it:
if (unlikely(!f2fs_is_valid_blkaddr(sbi, dn.data_blkaddr,
- DATA_GENERIC))) {
+ DATA_GENERIC_ENHANCE))) {
err = -EFAULT;
goto put_page;
}
-got_it:
+
/* read page */
fio.page = page;
fio.new_blkaddr = fio.old_blkaddr = dn.data_blkaddr;
diff --git a/fs/f2fs/inode.c b/fs/f2fs/inode.c
index e7f2e8759315..06233375c129 100644
--- a/fs/f2fs/inode.c
+++ b/fs/f2fs/inode.c
@@ -73,7 +73,7 @@ static int __written_first_block(struct f2fs_sb_info *sbi,
if (!__is_valid_data_blkaddr(addr))
return 1;
- if (!f2fs_is_valid_blkaddr(sbi, addr, DATA_GENERIC))
+ if (!f2fs_is_valid_blkaddr(sbi, addr, DATA_GENERIC_ENHANCE))
return -EFAULT;
return 0;
}
@@ -267,9 +267,10 @@ static bool sanity_check_inode(struct inode *inode, struct page *node_page)
struct extent_info *ei = &F2FS_I(inode)->extent_tree->largest;
if (ei->len &&
- (!f2fs_is_valid_blkaddr(sbi, ei->blk, DATA_GENERIC) ||
+ (!f2fs_is_valid_blkaddr(sbi, ei->blk,
+ DATA_GENERIC_ENHANCE) ||
!f2fs_is_valid_blkaddr(sbi, ei->blk + ei->len - 1,
- DATA_GENERIC))) {
+ DATA_GENERIC_ENHANCE))) {
set_sbi_flag(sbi, SBI_NEED_FSCK);
f2fs_msg(sbi->sb, KERN_WARNING,
"%s: inode (ino=%lx) extent info [%u, %u, %u] "
diff --git a/fs/f2fs/node.c b/fs/f2fs/node.c
index 3f99ab288695..b2c496c62921 100644
--- a/fs/f2fs/node.c
+++ b/fs/f2fs/node.c
@@ -454,7 +454,7 @@ static void set_node_addr(struct f2fs_sb_info *sbi, struct node_info *ni,
new_blkaddr == NULL_ADDR);
f2fs_bug_on(sbi, nat_get_blkaddr(e) == NEW_ADDR &&
new_blkaddr == NEW_ADDR);
- f2fs_bug_on(sbi, is_valid_data_blkaddr(sbi, nat_get_blkaddr(e)) &&
+ f2fs_bug_on(sbi, __is_valid_data_blkaddr(nat_get_blkaddr(e)) &&
new_blkaddr == NEW_ADDR);
/* increment version no as node is removed */
@@ -465,7 +465,7 @@ static void set_node_addr(struct f2fs_sb_info *sbi, struct node_info *ni,
/* change address */
nat_set_blkaddr(e, new_blkaddr);
- if (!is_valid_data_blkaddr(sbi, new_blkaddr))
+ if (!__is_valid_data_blkaddr(new_blkaddr))
set_nat_flag(e, IS_CHECKPOINTED, false);
__set_nat_cache_dirty(nm_i, e);
@@ -526,6 +526,7 @@ int f2fs_get_node_info(struct f2fs_sb_info *sbi, nid_t nid,
struct f2fs_nat_entry ne;
struct nat_entry *e;
pgoff_t index;
+ block_t blkaddr;
int i;
ni->nid = nid;
@@ -569,6 +570,11 @@ int f2fs_get_node_info(struct f2fs_sb_info *sbi, nid_t nid,
node_info_from_raw_nat(ni, &ne);
f2fs_put_page(page, 1);
cache:
+ blkaddr = le32_to_cpu(ne.block_addr);
+ if (__is_valid_data_blkaddr(blkaddr) &&
+ !f2fs_is_valid_blkaddr(sbi, blkaddr, DATA_GENERIC_ENHANCE))
+ return -EFAULT;
+
/* cache nat entry */
cache_nat_entry(sbi, nid, &ne);
return 0;
@@ -1541,7 +1547,8 @@ static int __write_node_page(struct page *page, bool atomic, bool *submitted,
}
if (__is_valid_data_blkaddr(ni.blk_addr) &&
- !f2fs_is_valid_blkaddr(sbi, ni.blk_addr, DATA_GENERIC)) {
+ !f2fs_is_valid_blkaddr(sbi, ni.blk_addr,
+ DATA_GENERIC_ENHANCE)) {
up_read(&sbi->node_write);
goto redirty_out;
}
diff --git a/fs/f2fs/recovery.c b/fs/f2fs/recovery.c
index d4f9d5408103..79f5812c5e1f 100644
--- a/fs/f2fs/recovery.c
+++ b/fs/f2fs/recovery.c
@@ -559,6 +559,18 @@ static int do_recover_data(struct f2fs_sb_info *sbi, struct inode *inode,
src = datablock_addr(dn.inode, dn.node_page, dn.ofs_in_node);
dest = datablock_addr(dn.inode, page, dn.ofs_in_node);
+ if (__is_valid_data_blkaddr(src) &&
+ !f2fs_is_valid_blkaddr(sbi, src, META_POR)) {
+ err = -EFAULT;
+ goto err;
+ }
+
+ if (__is_valid_data_blkaddr(dest) &&
+ !f2fs_is_valid_blkaddr(sbi, dest, META_POR)) {
+ err = -EFAULT;
+ goto err;
+ }
+
/* skip recovering if dest is the same as src */
if (src == dest)
continue;
diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
index a3380d1de600..b59d1c85863b 100644
--- a/fs/f2fs/segment.c
+++ b/fs/f2fs/segment.c
@@ -2217,7 +2217,7 @@ bool f2fs_is_checkpointed_data(struct f2fs_sb_info *sbi, block_t blkaddr)
struct seg_entry *se;
bool is_cp = false;
- if (!is_valid_data_blkaddr(sbi, blkaddr))
+ if (!__is_valid_data_blkaddr(blkaddr))
return true;
down_read(&sit_i->sentry_lock);
@@ -3333,7 +3333,7 @@ void f2fs_wait_on_block_writeback(struct inode *inode, block_t blkaddr)
if (!f2fs_post_read_required(inode))
return;
- if (!is_valid_data_blkaddr(sbi, blkaddr))
+ if (!__is_valid_data_blkaddr(blkaddr))
return;
cpage = find_lock_page(META_MAPPING(sbi), blkaddr);
diff --git a/fs/f2fs/segment.h b/fs/f2fs/segment.h
index 5c7ed0442d6e..b333ecca6ed6 100644
--- a/fs/f2fs/segment.h
+++ b/fs/f2fs/segment.h
@@ -82,7 +82,7 @@
(GET_SEGOFF_FROM_SEG0(sbi, blk_addr) & ((sbi)->blocks_per_seg - 1))
#define GET_SEGNO(sbi, blk_addr) \
- ((!is_valid_data_blkaddr(sbi, blk_addr)) ? \
+ ((!__is_valid_data_blkaddr(blk_addr)) ? \
NULL_SEGNO : GET_L2R_SEGNO(FREE_I(sbi), \
GET_SEGNO_FROM_SEG0(sbi, blk_addr)))
#define BLKS_PER_SEC(sbi) \
@@ -656,14 +656,15 @@ static inline void check_seg_range(struct f2fs_sb_info *sbi, unsigned int segno)
f2fs_bug_on(sbi, segno > TOTAL_SEGS(sbi) - 1);
}
-static inline void verify_block_addr(struct f2fs_io_info *fio, block_t blk_addr)
+static inline void verify_fio_blkaddr(struct f2fs_io_info *fio)
{
struct f2fs_sb_info *sbi = fio->sbi;
- if (__is_meta_io(fio))
- verify_blkaddr(sbi, blk_addr, META_GENERIC);
- else
- verify_blkaddr(sbi, blk_addr, DATA_GENERIC);
+ if (__is_valid_data_blkaddr(fio->old_blkaddr))
+ verify_blkaddr(sbi, fio->old_blkaddr, __is_meta_io(fio) ?
+ META_GENERIC : DATA_GENERIC);
+ verify_blkaddr(sbi, fio->new_blkaddr, __is_meta_io(fio) ?
+ META_GENERIC : DATA_GENERIC_ENHANCE);
}
/*
--
2.18.0.rc1
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH v2 2/2] f2fs: introduce DATA_GENERIC_ENHANCE
@ 2019-04-15 7:26 ` Chao Yu
0 siblings, 0 replies; 13+ messages in thread
From: Chao Yu @ 2019-04-15 7:26 UTC (permalink / raw)
To: jaegeuk; +Cc: linux-f2fs-devel, linux-kernel, chao, Chao Yu
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
Signed-off-by: Chao Yu <yuchao0@huawei.com>
---
v2:
- update issue list this patch can fix.
- check blkaddr in do_recover_data().
- rebase code to last dev branch.
fs/f2fs/checkpoint.c | 32 +++++++++++++++++++++++++++++++-
fs/f2fs/data.c | 34 +++++++++++++++++++---------------
fs/f2fs/f2fs.h | 12 ++----------
fs/f2fs/file.c | 15 ++++++++++++---
fs/f2fs/gc.c | 10 ++++++++--
fs/f2fs/inode.c | 7 ++++---
fs/f2fs/node.c | 13 ++++++++++---
fs/f2fs/recovery.c | 12 ++++++++++++
fs/f2fs/segment.c | 4 ++--
fs/f2fs/segment.h | 13 +++++++------
10 files changed, 107 insertions(+), 45 deletions(-)
diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c
index f42b0015724b..a7ad1b1e5750 100644
--- a/fs/f2fs/checkpoint.c
+++ b/fs/f2fs/checkpoint.c
@@ -130,6 +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)
+{
+ struct seg_entry *se;
+ unsigned int segno, offset;
+ bool exist;
+
+ 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) {
+ 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;
+}
+
bool f2fs_is_valid_blkaddr(struct f2fs_sb_info *sbi,
block_t blkaddr, int type)
{
@@ -152,14 +174,22 @@ bool f2fs_is_valid_blkaddr(struct f2fs_sb_info *sbi,
break;
case META_POR:
case DATA_GENERIC:
+ case DATA_GENERIC_ENHANCE:
if (unlikely(blkaddr >= MAX_BLKADDR(sbi) ||
blkaddr < MAIN_BLKADDR(sbi))) {
- if (type == DATA_GENERIC) {
+ 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);
}
return false;
+ } else {
+ if (type == DATA_GENERIC_ENHANCE) {
+ if (!__is_bitmap_valid(sbi, blkaddr))
+ return false;
+ }
}
break;
case META_GENERIC:
diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
index eabd34a79451..ee5c7962b0f3 100644
--- a/fs/f2fs/data.c
+++ b/fs/f2fs/data.c
@@ -456,8 +456,8 @@ int f2fs_submit_page_bio(struct f2fs_io_info *fio)
fio->encrypted_page : fio->page;
if (!f2fs_is_valid_blkaddr(fio->sbi, fio->new_blkaddr,
- fio->is_por ? META_POR :
- (__is_meta_io(fio) ? META_GENERIC : DATA_GENERIC)))
+ fio->is_por ? META_POR : (__is_meta_io(fio) ?
+ META_GENERIC : DATA_GENERIC_ENHANCE)))
return -EFAULT;
trace_f2fs_submit_page_bio(page, fio);
@@ -507,9 +507,7 @@ void f2fs_submit_page_write(struct f2fs_io_info *fio)
spin_unlock(&io->io_lock);
}
- if (__is_valid_data_blkaddr(fio->old_blkaddr))
- verify_block_addr(fio, fio->old_blkaddr);
- verify_block_addr(fio, fio->new_blkaddr);
+ verify_fio_blkaddr(fio);
bio_page = fio->encrypted_page ? fio->encrypted_page : fio->page;
@@ -566,7 +564,7 @@ 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))
+ 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);
@@ -596,8 +594,13 @@ static struct bio *f2fs_grab_read_bio(struct inode *inode, block_t blkaddr,
static int f2fs_submit_page_read(struct inode *inode, struct page *page,
block_t blkaddr)
{
- struct bio *bio = f2fs_grab_read_bio(inode, blkaddr, 1, 0);
+ 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);
@@ -609,8 +612,8 @@ static int f2fs_submit_page_read(struct inode *inode, struct page *page,
return -EFAULT;
}
ClearPageError(page);
- inc_page_count(F2FS_I_SB(inode), F2FS_RD_DATA);
- __submit_bio(F2FS_I_SB(inode), bio, DATA);
+ inc_page_count(sbi, F2FS_RD_DATA);
+ __submit_bio(sbi, bio, DATA);
return 0;
}
@@ -1093,12 +1096,12 @@ int f2fs_map_blocks(struct inode *inode, struct f2fs_map_blocks *map,
blkaddr = datablock_addr(dn.inode, dn.node_page, dn.ofs_in_node);
if (__is_valid_data_blkaddr(blkaddr) &&
- !f2fs_is_valid_blkaddr(sbi, blkaddr, DATA_GENERIC)) {
+ !f2fs_is_valid_blkaddr(sbi, blkaddr, DATA_GENERIC_ENHANCE)) {
err = -EFAULT;
goto sync_out;
}
- if (is_valid_data_blkaddr(sbi, blkaddr)) {
+ if (__is_valid_data_blkaddr(blkaddr)) {
/* use out-place-update for driect IO under LFS mode */
if (test_opt(sbi, LFS) && flag == F2FS_GET_BLOCK_DIO &&
map->m_may_create) {
@@ -1591,7 +1594,7 @@ static int f2fs_mpage_readpages(struct address_space *mapping,
}
if (!f2fs_is_valid_blkaddr(F2FS_I_SB(inode), block_nr,
- DATA_GENERIC))
+ DATA_GENERIC_ENHANCE))
goto set_error_page;
} else {
zero_out:
@@ -1822,7 +1825,7 @@ int f2fs_do_write_data_page(struct f2fs_io_info *fio)
fio->old_blkaddr = ei.blk + page->index - ei.fofs;
if (!f2fs_is_valid_blkaddr(fio->sbi, fio->old_blkaddr,
- DATA_GENERIC))
+ DATA_GENERIC_ENHANCE))
return -EFAULT;
ipu_force = true;
@@ -1849,7 +1852,7 @@ int f2fs_do_write_data_page(struct f2fs_io_info *fio)
got_it:
if (__is_valid_data_blkaddr(fio->old_blkaddr) &&
!f2fs_is_valid_blkaddr(fio->sbi, fio->old_blkaddr,
- DATA_GENERIC)) {
+ DATA_GENERIC_ENHANCE)) {
err = -EFAULT;
goto out_writepage;
}
@@ -1857,7 +1860,8 @@ int f2fs_do_write_data_page(struct f2fs_io_info *fio)
* If current allocation needs SSR,
* it had better in-place writes for updated data.
*/
- if (ipu_force || (is_valid_data_blkaddr(fio->sbi, fio->old_blkaddr) &&
+ if (ipu_force ||
+ (__is_valid_data_blkaddr(fio->old_blkaddr) &&
need_inplace_update(fio))) {
err = encrypt_one_page(fio);
if (err)
diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
index 003a07bd56e3..691452448196 100644
--- a/fs/f2fs/f2fs.h
+++ b/fs/f2fs/f2fs.h
@@ -210,7 +210,8 @@ enum {
META_SSA,
META_MAX,
META_POR,
- DATA_GENERIC,
+ DATA_GENERIC, /* check range only */
+ DATA_GENERIC_ENHANCE, /* strong check on range and segment bitmap */
META_GENERIC,
};
@@ -2837,15 +2838,6 @@ static inline bool __is_valid_data_blkaddr(block_t blkaddr)
return true;
}
-static inline bool is_valid_data_blkaddr(struct f2fs_sb_info *sbi,
- block_t blkaddr)
-{
- if (!__is_valid_data_blkaddr(blkaddr))
- return false;
- verify_blkaddr(sbi, blkaddr, DATA_GENERIC);
- return true;
-}
-
static inline void f2fs_set_page_private(struct page *page,
unsigned long data)
{
diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
index 65e14ec7366f..f19a2ec9f0ef 100644
--- a/fs/f2fs/file.c
+++ b/fs/f2fs/file.c
@@ -358,7 +358,7 @@ static bool __found_offset(struct f2fs_sb_info *sbi, block_t blkaddr,
switch (whence) {
case SEEK_DATA:
if ((blkaddr == NEW_ADDR && dirty == pgofs) ||
- is_valid_data_blkaddr(sbi, blkaddr))
+ __is_valid_data_blkaddr(blkaddr))
return true;
break;
case SEEK_HOLE:
@@ -424,7 +424,7 @@ static loff_t f2fs_seek_block(struct file *file, loff_t offset, int whence)
if (__is_valid_data_blkaddr(blkaddr) &&
!f2fs_is_valid_blkaddr(F2FS_I_SB(inode),
- blkaddr, DATA_GENERIC)) {
+ blkaddr, DATA_GENERIC_ENHANCE)) {
f2fs_put_dnode(&dn);
goto fail;
}
@@ -525,7 +525,8 @@ void f2fs_truncate_data_blocks_range(struct dnode_of_data *dn, int count)
f2fs_set_data_blkaddr(dn);
if (__is_valid_data_blkaddr(blkaddr) &&
- !f2fs_is_valid_blkaddr(sbi, blkaddr, DATA_GENERIC))
+ !f2fs_is_valid_blkaddr(sbi, blkaddr,
+ DATA_GENERIC_ENHANCE))
continue;
f2fs_invalidate_blocks(sbi, blkaddr);
@@ -1019,6 +1020,14 @@ static int __read_out_blkaddrs(struct inode *inode, block_t *blkaddr,
for (i = 0; i < done; i++, blkaddr++, do_replace++, dn.ofs_in_node++) {
*blkaddr = datablock_addr(dn.inode,
dn.node_page, dn.ofs_in_node);
+
+ if (__is_valid_data_blkaddr(*blkaddr) &&
+ !f2fs_is_valid_blkaddr(sbi, *blkaddr,
+ DATA_GENERIC_ENHANCE)) {
+ f2fs_put_dnode(&dn);
+ return -EFAULT;
+ }
+
if (!f2fs_is_checkpointed_data(sbi, *blkaddr)) {
if (test_opt(sbi, LFS)) {
diff --git a/fs/f2fs/gc.c b/fs/f2fs/gc.c
index a66a8752e5f6..45781a1fc680 100644
--- a/fs/f2fs/gc.c
+++ b/fs/f2fs/gc.c
@@ -665,12 +665,18 @@ static int ra_data_block(struct inode *inode, pgoff_t index)
goto put_page;
f2fs_put_dnode(&dn);
+ if (!__is_valid_data_blkaddr(dn.data_blkaddr)) {
+ err = -ENOENT;
+ goto put_page;
+ }
+
+got_it:
if (unlikely(!f2fs_is_valid_blkaddr(sbi, dn.data_blkaddr,
- DATA_GENERIC))) {
+ DATA_GENERIC_ENHANCE))) {
err = -EFAULT;
goto put_page;
}
-got_it:
+
/* read page */
fio.page = page;
fio.new_blkaddr = fio.old_blkaddr = dn.data_blkaddr;
diff --git a/fs/f2fs/inode.c b/fs/f2fs/inode.c
index e7f2e8759315..06233375c129 100644
--- a/fs/f2fs/inode.c
+++ b/fs/f2fs/inode.c
@@ -73,7 +73,7 @@ static int __written_first_block(struct f2fs_sb_info *sbi,
if (!__is_valid_data_blkaddr(addr))
return 1;
- if (!f2fs_is_valid_blkaddr(sbi, addr, DATA_GENERIC))
+ if (!f2fs_is_valid_blkaddr(sbi, addr, DATA_GENERIC_ENHANCE))
return -EFAULT;
return 0;
}
@@ -267,9 +267,10 @@ static bool sanity_check_inode(struct inode *inode, struct page *node_page)
struct extent_info *ei = &F2FS_I(inode)->extent_tree->largest;
if (ei->len &&
- (!f2fs_is_valid_blkaddr(sbi, ei->blk, DATA_GENERIC) ||
+ (!f2fs_is_valid_blkaddr(sbi, ei->blk,
+ DATA_GENERIC_ENHANCE) ||
!f2fs_is_valid_blkaddr(sbi, ei->blk + ei->len - 1,
- DATA_GENERIC))) {
+ DATA_GENERIC_ENHANCE))) {
set_sbi_flag(sbi, SBI_NEED_FSCK);
f2fs_msg(sbi->sb, KERN_WARNING,
"%s: inode (ino=%lx) extent info [%u, %u, %u] "
diff --git a/fs/f2fs/node.c b/fs/f2fs/node.c
index 3f99ab288695..b2c496c62921 100644
--- a/fs/f2fs/node.c
+++ b/fs/f2fs/node.c
@@ -454,7 +454,7 @@ static void set_node_addr(struct f2fs_sb_info *sbi, struct node_info *ni,
new_blkaddr == NULL_ADDR);
f2fs_bug_on(sbi, nat_get_blkaddr(e) == NEW_ADDR &&
new_blkaddr == NEW_ADDR);
- f2fs_bug_on(sbi, is_valid_data_blkaddr(sbi, nat_get_blkaddr(e)) &&
+ f2fs_bug_on(sbi, __is_valid_data_blkaddr(nat_get_blkaddr(e)) &&
new_blkaddr == NEW_ADDR);
/* increment version no as node is removed */
@@ -465,7 +465,7 @@ static void set_node_addr(struct f2fs_sb_info *sbi, struct node_info *ni,
/* change address */
nat_set_blkaddr(e, new_blkaddr);
- if (!is_valid_data_blkaddr(sbi, new_blkaddr))
+ if (!__is_valid_data_blkaddr(new_blkaddr))
set_nat_flag(e, IS_CHECKPOINTED, false);
__set_nat_cache_dirty(nm_i, e);
@@ -526,6 +526,7 @@ int f2fs_get_node_info(struct f2fs_sb_info *sbi, nid_t nid,
struct f2fs_nat_entry ne;
struct nat_entry *e;
pgoff_t index;
+ block_t blkaddr;
int i;
ni->nid = nid;
@@ -569,6 +570,11 @@ int f2fs_get_node_info(struct f2fs_sb_info *sbi, nid_t nid,
node_info_from_raw_nat(ni, &ne);
f2fs_put_page(page, 1);
cache:
+ blkaddr = le32_to_cpu(ne.block_addr);
+ if (__is_valid_data_blkaddr(blkaddr) &&
+ !f2fs_is_valid_blkaddr(sbi, blkaddr, DATA_GENERIC_ENHANCE))
+ return -EFAULT;
+
/* cache nat entry */
cache_nat_entry(sbi, nid, &ne);
return 0;
@@ -1541,7 +1547,8 @@ static int __write_node_page(struct page *page, bool atomic, bool *submitted,
}
if (__is_valid_data_blkaddr(ni.blk_addr) &&
- !f2fs_is_valid_blkaddr(sbi, ni.blk_addr, DATA_GENERIC)) {
+ !f2fs_is_valid_blkaddr(sbi, ni.blk_addr,
+ DATA_GENERIC_ENHANCE)) {
up_read(&sbi->node_write);
goto redirty_out;
}
diff --git a/fs/f2fs/recovery.c b/fs/f2fs/recovery.c
index d4f9d5408103..79f5812c5e1f 100644
--- a/fs/f2fs/recovery.c
+++ b/fs/f2fs/recovery.c
@@ -559,6 +559,18 @@ static int do_recover_data(struct f2fs_sb_info *sbi, struct inode *inode,
src = datablock_addr(dn.inode, dn.node_page, dn.ofs_in_node);
dest = datablock_addr(dn.inode, page, dn.ofs_in_node);
+ if (__is_valid_data_blkaddr(src) &&
+ !f2fs_is_valid_blkaddr(sbi, src, META_POR)) {
+ err = -EFAULT;
+ goto err;
+ }
+
+ if (__is_valid_data_blkaddr(dest) &&
+ !f2fs_is_valid_blkaddr(sbi, dest, META_POR)) {
+ err = -EFAULT;
+ goto err;
+ }
+
/* skip recovering if dest is the same as src */
if (src == dest)
continue;
diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
index a3380d1de600..b59d1c85863b 100644
--- a/fs/f2fs/segment.c
+++ b/fs/f2fs/segment.c
@@ -2217,7 +2217,7 @@ bool f2fs_is_checkpointed_data(struct f2fs_sb_info *sbi, block_t blkaddr)
struct seg_entry *se;
bool is_cp = false;
- if (!is_valid_data_blkaddr(sbi, blkaddr))
+ if (!__is_valid_data_blkaddr(blkaddr))
return true;
down_read(&sit_i->sentry_lock);
@@ -3333,7 +3333,7 @@ void f2fs_wait_on_block_writeback(struct inode *inode, block_t blkaddr)
if (!f2fs_post_read_required(inode))
return;
- if (!is_valid_data_blkaddr(sbi, blkaddr))
+ if (!__is_valid_data_blkaddr(blkaddr))
return;
cpage = find_lock_page(META_MAPPING(sbi), blkaddr);
diff --git a/fs/f2fs/segment.h b/fs/f2fs/segment.h
index 5c7ed0442d6e..b333ecca6ed6 100644
--- a/fs/f2fs/segment.h
+++ b/fs/f2fs/segment.h
@@ -82,7 +82,7 @@
(GET_SEGOFF_FROM_SEG0(sbi, blk_addr) & ((sbi)->blocks_per_seg - 1))
#define GET_SEGNO(sbi, blk_addr) \
- ((!is_valid_data_blkaddr(sbi, blk_addr)) ? \
+ ((!__is_valid_data_blkaddr(blk_addr)) ? \
NULL_SEGNO : GET_L2R_SEGNO(FREE_I(sbi), \
GET_SEGNO_FROM_SEG0(sbi, blk_addr)))
#define BLKS_PER_SEC(sbi) \
@@ -656,14 +656,15 @@ static inline void check_seg_range(struct f2fs_sb_info *sbi, unsigned int segno)
f2fs_bug_on(sbi, segno > TOTAL_SEGS(sbi) - 1);
}
-static inline void verify_block_addr(struct f2fs_io_info *fio, block_t blk_addr)
+static inline void verify_fio_blkaddr(struct f2fs_io_info *fio)
{
struct f2fs_sb_info *sbi = fio->sbi;
- if (__is_meta_io(fio))
- verify_blkaddr(sbi, blk_addr, META_GENERIC);
- else
- verify_blkaddr(sbi, blk_addr, DATA_GENERIC);
+ if (__is_valid_data_blkaddr(fio->old_blkaddr))
+ verify_blkaddr(sbi, fio->old_blkaddr, __is_meta_io(fio) ?
+ META_GENERIC : DATA_GENERIC);
+ verify_blkaddr(sbi, fio->new_blkaddr, __is_meta_io(fio) ?
+ META_GENERIC : DATA_GENERIC_ENHANCE);
}
/*
--
2.18.0.rc1
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH v2 2/2] f2fs: introduce DATA_GENERIC_ENHANCE
2019-04-15 7:26 ` Chao Yu
@ 2019-04-24 9:36 ` Jaegeuk Kim
-1 siblings, 0 replies; 13+ messages in thread
From: Jaegeuk Kim @ 2019-04-24 9:36 UTC (permalink / raw)
To: Chao Yu; +Cc: linux-f2fs-devel, linux-kernel, chao
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)) {
+ 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
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH v2 2/2] f2fs: introduce DATA_GENERIC_ENHANCE
@ 2019-04-24 9:36 ` Jaegeuk Kim
0 siblings, 0 replies; 13+ messages in thread
From: Jaegeuk Kim @ 2019-04-24 9:36 UTC (permalink / raw)
To: Chao Yu; +Cc: linux-f2fs-devel, linux-kernel, chao
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)) {
+ 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
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH v2 2/2] f2fs: introduce DATA_GENERIC_ENHANCE
2019-04-24 9:36 ` Jaegeuk Kim
(?)
@ 2019-04-24 15:06 ` Chao Yu
2019-04-28 13:31 ` Jaegeuk Kim
-1 siblings, 1 reply; 13+ messages in thread
From: Chao Yu @ 2019-04-24 15:06 UTC (permalink / raw)
To: Jaegeuk Kim, Chao Yu; +Cc: linux-f2fs-devel, linux-kernel
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?
> + 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)) {
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;
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 2/2] f2fs: introduce DATA_GENERIC_ENHANCE
2019-04-24 15:06 ` Chao Yu
@ 2019-04-28 13:31 ` Jaegeuk Kim
2019-04-29 9:03 ` Chao Yu
0 siblings, 1 reply; 13+ messages in thread
From: Jaegeuk Kim @ 2019-04-28 13:31 UTC (permalink / raw)
To: Chao Yu; +Cc: Chao Yu, linux-f2fs-devel, linux-kernel
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.
>
> > + 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)) {
>
> 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;
> >
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 2/2] f2fs: introduce DATA_GENERIC_ENHANCE
2019-04-28 13:31 ` Jaegeuk Kim
@ 2019-04-29 9:03 ` Chao Yu
0 siblings, 0 replies; 13+ messages in thread
From: Chao Yu @ 2019-04-29 9:03 UTC (permalink / raw)
To: Jaegeuk Kim, Chao Yu; +Cc: linux-f2fs-devel, linux-kernel
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.
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;
>>>
> .
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 2/2] f2fs: introduce DATA_GENERIC_ENHANCE
@ 2019-04-29 9:03 ` Chao Yu
0 siblings, 0 replies; 13+ messages in thread
From: Chao Yu @ 2019-04-29 9:03 UTC (permalink / raw)
To: Jaegeuk Kim, Chao Yu; +Cc: linux-f2fs-devel, linux-kernel
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.
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;
>>>
> .
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 2/2] f2fs: introduce DATA_GENERIC_ENHANCE
2019-04-29 9:03 ` Chao Yu
(?)
@ 2019-04-30 3:14 ` Jaegeuk Kim
2019-04-30 3:18 ` Chao Yu
-1 siblings, 1 reply; 13+ messages in thread
From: Jaegeuk Kim @ 2019-04-30 3:14 UTC (permalink / raw)
To: Chao Yu; +Cc: Chao Yu, linux-f2fs-devel, linux-kernel
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.
>
> 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;
> >>>
> > .
> >
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 2/2] f2fs: introduce DATA_GENERIC_ENHANCE
2019-04-30 3:14 ` Jaegeuk Kim
@ 2019-04-30 3:18 ` Chao Yu
0 siblings, 0 replies; 13+ messages in thread
From: Chao Yu @ 2019-04-30 3:18 UTC (permalink / raw)
To: Jaegeuk Kim; +Cc: Chao Yu, linux-f2fs-devel, linux-kernel
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;
>>>>>
>>> .
>>>
> .
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 2/2] f2fs: introduce DATA_GENERIC_ENHANCE
@ 2019-04-30 3:18 ` Chao Yu
0 siblings, 0 replies; 13+ messages in thread
From: Chao Yu @ 2019-04-30 3:18 UTC (permalink / raw)
To: Jaegeuk Kim; +Cc: Chao Yu, linux-f2fs-devel, linux-kernel
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;
>>>>>
>>> .
>>>
> .
>
^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2019-04-30 3:19 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-04-15 7:26 [PATCH v2 1/2] f2fs: fix wrong __is_meta_io() macro Chao Yu
2019-04-15 7:26 ` Chao Yu
2019-04-15 7:26 ` [PATCH v2 2/2] f2fs: introduce DATA_GENERIC_ENHANCE Chao Yu
2019-04-15 7:26 ` Chao Yu
2019-04-24 9:36 ` Jaegeuk Kim
2019-04-24 9:36 ` Jaegeuk Kim
2019-04-24 15:06 ` Chao Yu
2019-04-28 13:31 ` Jaegeuk Kim
2019-04-29 9:03 ` Chao Yu
2019-04-29 9:03 ` Chao Yu
2019-04-30 3:14 ` Jaegeuk Kim
2019-04-30 3:18 ` Chao Yu
2019-04-30 3:18 ` Chao Yu
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.