linux-ext4 - RE: [PATCH 2/2] ext4: don't RO for bitmaps error in default

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] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 23 May 2018 06:38:14 +0000
From:   Wang Shilong <wshilong@....com>
To:     Andreas Dilger <adilger@...ger.ca>,
        Wang Shilong <wangshilong1991@...il.com>
CC:     Ext4 Developers List <linux-ext4@...r.kernel.org>,
        Shuichi Ihara <sihara@....com>, Theodore Ts'o <tytso@....edu>
Subject: RE: [PATCH 2/2] ext4: don't RO for bitmaps error in default

Hello Andreas,

Sorry for late reply, I've got some time to finish this:

<...SNIP...>

>
> Suggested-by: Andreas Dilger <adilger.kernel@...ger.ca>
> Signed-off-by: Wang Shilong <wshilong@....com>
> ---

I think this looks pretty reasonable, but the replacement of ext4_error()
on some of the error paths with ext4_warning() seems like there is a
chance that an error might be "lost" if one of the underlying functions
misses its call to ext4_mark_group_bitmap_corrupted()->save_error_info().

I looked at the patch and the previous one together, and it _looks_ like
all of the error paths are handled at some point lower down in the stack,
but this has the potential to break in the future if a change doesn't
call ext4_mark_group_bitmap_corrupted() itself for some reason.

One option would be instead of ext4_warning() (or dropping completely)
the ext4_error() messages, use ext4_mark_group_bitmap_corrupted() and
change that function to not print an error message if the inode bitmap
or block bitmap is already flagged as corrupted.  That gives us code
safety (the higher-level error check/message will catch any unhandled
errors from below), while also reducing console spam (duplicate errors
will not be printed, and the "most specific" error will print once to
the console).

--------> could you please more specific for this? I understood that we could add
the check in ext4_mark_group_bitmap_corrupted() that only call ext4_error()/warnging()
if necessary, but I did not catch your other point, you mean we should move ext4_mark_group_bitmap_corrupted()
to higher-level call? 

Thanks,
Shilong