lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <4983e1e3-2dea-89c0-8c48-688691e3a0cd@huawei.com>
Date:   Wed, 2 Aug 2017 17:00:13 +0800
From:   Yunlong Song <yunlong.song@...wei.com>
To:     Chao Yu <yuchao0@...wei.com>, Yunlong Song <sylinux@....com>,
        Chao Yu <chao@...nel.org>
CC:     jaegeuk <jaegeuk@...nel.org>, miaoxie <miaoxie@...wei.com>,
        "bintian.wang" <bintian.wang@...wei.com>,
        linux-fsdevel <linux-fsdevel@...r.kernel.org>,
        linux-f2fs-devel <linux-f2fs-devel@...ts.sourceforge.net>,
        linux-kernel <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] f2fs: update cur_valid_map_mir together with
 cur_valid_map

Agree, it's safe to bug_on or readonly to protect data. This patch is fine.

On 2017/8/2 16:47, Chao Yu wrote:
> Hi Yunlong,
>
> On 2017/8/2 16:02, Yunlong Song wrote:
>> Hi Chao,
>>       For the memory overflow or cache bit-transition case, when CHECK_FS
>> is on, should f2fs
>> enter panic or just provide WARNing information? I prefer WARNing,
>> because it is not f2fs's
>> fault, f2fs does not need to pay for it, why not use WARN_ON instead?
> Actually, if that happens, I think both original and mirror bitmap datas are not
> trustable, it's harmful to continue to trigger write IOs based on those bitmap
> datas. So IMO, it would be better to trigger bug_on.
>
> At least, if only warning is showed, we'd better to turn f2fs to readonly and
> fail current write IO.
>
> Thanks,
>
>> On 2017/8/2 9:34, Chao Yu wrote:
>>> Hi Yunlong,
>>>
>>> On 2017/8/2 0:59, Yunlong Song wrote:
>>>> Hi Chao,
>>>>       I think there is no need to test mirror bitmap when original bitmap's check
>>>> get passed, it is an instruction waste for "test". By the way, previous patch
>>>> uses WARN to skip trigger panic when the memory is flipped, I think it is proper
>>>> to not trigger panic in this situation, because it is not code bug at all in
>>>> such case.
>>> Original idea is trying to use bitmap mirror to detect bitmap corruption caused
>>> by memory overflow or bit-transition of cache.
>>>
>>> So if we encounter inconsistency in between original bitmap and mirror bitmap,
>>> there may be memory overflow or cache bit-transition, we need to detect that;
>>> another case is that both bitmap is consistent, but we are trying to reuse valid
>>> block address or free invalid block address, indeed that is a bug.
>>>
>>> Below modification can cover above two cases, which make original idea of
>>> introducing mirror bitmap check working correctly.
>>>
>>> Thanks,
>>>
>>>> On 08/01/2017 23:44, Chao Yu <mailto:chao@...nel.org> wrote:
>>>>
>>>>       Hi Yunlong,
>>>>
>>>>       How about checking consistence in between original and mirror bitmap all
>>>>       the time as below?
>>>>
>>>>       ---
>>>>       fs/f2fs/segment.c | 41 +++++++++++++++++++++++++++--------------
>>>>       1 file changed, 27 insertions(+), 14 deletions(-)
>>>>
>>>>       diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
>>>>       index a26c24dae70c..f32a19cf486a 100644
>>>>       --- a/fs/f2fs/segment.c
>>>>       +++ b/fs/f2fs/segment.c
>>>>       @@ -1510,6 +1510,7 @@ static void update_sit_entry(struct f2fs_sb_info *sbi,
>>>>       block_t blkaddr, int del)
>>>>          struct seg_entry *se;
>>>>          unsigned int segno, offset;
>>>>          long int new_vblocks;
>>>>       +    bool exist, mir_exist;
>>>>
>>>>          segno = GET_SEGNO(sbi, blkaddr);
>>>>
>>>>       @@ -1526,17 +1527,23 @@ static void update_sit_entry(struct f2fs_sb_info
>>>>       *sbi, block_t blkaddr, int del)
>>>>
>>>>          /* Update valid block bitmap */
>>>>          if (del > 0) {
>>>>       -        if (f2fs_test_and_set_bit(offset, se->cur_valid_map)) {
>>>>       +        exist = f2fs_test_and_set_bit(offset, se->cur_valid_map);
>>>>       #ifdef CONFIG_F2FS_CHECK_FS
>>>>       -            if (f2fs_test_and_set_bit(offset,
>>>>       -                        se->cur_valid_map_mir))
>>>>       -                f2fs_bug_on(sbi, 1);
>>>>       -            else
>>>>       -                WARN_ON(1);
>>>>       -#else
>>>>       +        mir_exist = f2fs_test_and_set_bit(offset,
>>>>       +                        se->cur_valid_map_mir);
>>>>       +        if (exist != mir_exist) {
>>>>       +            f2fs_msg(sbi->sb, KERN_ERR, "Inconsistent error "
>>>>       +                "when setting bitmap, blk:%u, old bit:%d",
>>>>       +                blkaddr, exist);
>>>>                  f2fs_bug_on(sbi, 1);
>>>>       +        }
>>>>       #endif
>>>>       +        if (exist) {
>>>>       +            f2fs_msg(sbi->sb, KERN_ERR,
>>>>       +                "Bitmap was set, blk:%u", blkaddr);
>>>>       +            f2fs_bug_on(sbi, 1);
>>>>              }
>>>>       +
>>>>              if (f2fs_discard_en(sbi) &&
>>>>                  !f2fs_test_and_set_bit(offset, se->discard_map))
>>>>                  sbi->discard_blks--;
>>>>       @@ -1547,17 +1554,23 @@ static void update_sit_entry(struct f2fs_sb_info
>>>>       *sbi, block_t blkaddr, int del)
>>>>                      se->ckpt_valid_blocks++;
>>>>              }
>>>>          } else {
>>>>       -        if (!f2fs_test_and_clear_bit(offset, se->cur_valid_map)) {
>>>>       +        exist = f2fs_test_and_clear_bit(offset, se->cur_valid_map);
>>>>       #ifdef CONFIG_F2FS_CHECK_FS
>>>>       -            if (!f2fs_test_and_clear_bit(offset,
>>>>       -                        se->cur_valid_map_mir))
>>>>       -                f2fs_bug_on(sbi, 1);
>>>>       -            else
>>>>       -                WARN_ON(1);
>>>>       -#else
>>>>       +        mir_exist = f2fs_test_and_clear_bit(offset,
>>>>       +                        se->cur_valid_map_mir);
>>>>       +        if (exist != mir_exist) {
>>>>       +            f2fs_msg(sbi->sb, KERN_ERR, "Inconsistent error "
>>>>       +                "when clearing bitmap, blk:%u, old bit:%d",
>>>>       +                blkaddr, exist);
>>>>                  f2fs_bug_on(sbi, 1);
>>>>       +        }
>>>>       #endif
>>>>       +        if (!exist) {
>>>>       +            f2fs_msg(sbi->sb, KERN_ERR,
>>>>       +                "Bitmap was cleared, blk:%u", blkaddr);
>>>>       +            f2fs_bug_on(sbi, 1);
>>>>              }
>>>>       +
>>>>              if (f2fs_discard_en(sbi) &&
>>>>                  f2fs_test_and_clear_bit(offset, se->discard_map))
>>>>                  sbi->discard_blks++;
>>>>       --
>>>>       2.13.0.90.g1eb437020
>>>>
>>>>       On 2017/8/1 15:56, Yunlong Song wrote:
>>>>       > When cur_valid_map passes the f2fs_test_and_set(,clear)_bit test,
>>>>       > cur_valid_map_mir update is skipped unlikely, so fix it.
>>>>       >
>>>>       > Signed-off-by: Yunlong Song <yunlong.song@...wei.com>
>>>>       > ---
>>>>       >  fs/f2fs/segment.c | 8 ++++++++
>>>>       >  1 file changed, 8 insertions(+)
>>>>       >
>>>>       > diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
>>>>       > index 151968e..6f7731a 100644
>>>>       > --- a/fs/f2fs/segment.c
>>>>       > +++ b/fs/f2fs/segment.c
>>>>       > @@ -1535,6 +1535,10 @@ static void update_sit_entry(struct f2fs_sb_info
>>>>       *sbi, block_t blkaddr, int del)
>>>>       >              f2fs_bug_on(sbi, 1);
>>>>       >  #endif
>>>>       >          }
>>>>       > +#ifdef CONFIG_F2FS_CHECK_FS
>>>>       > +        else
>>>>       > +            f2fs_set_bit(offset, se->cur_valid_map_mir);
>>>>       > +#endif
>>>>       >          if (f2fs_discard_en(sbi) &&
>>>>       >              !f2fs_test_and_set_bit(offset, se->discard_map))
>>>>       >              sbi->discard_blks--;
>>>>       > @@ -1556,6 +1560,10 @@ static void update_sit_entry(struct f2fs_sb_info
>>>>       *sbi, block_t blkaddr, int del)
>>>>       >              f2fs_bug_on(sbi, 1);
>>>>       >  #endif
>>>>       >          }
>>>>       > +#ifdef CONFIG_F2FS_CHECK_FS
>>>>       > +        else
>>>>       > +            f2fs_clear_bit(offset, se->cur_valid_map_mir);
>>>>       > +#endif
>>>>       >          if (f2fs_discard_en(sbi) &&
>>>>       >              f2fs_test_and_clear_bit(offset, se->discard_map))
>>>>       >              sbi->discard_blks++;
>>>>       >
>>>>
>>> .
>>>
>
> .
>

-- 
Thanks,
Yunlong Song


Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ