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

Message-ID: <3ED34739A4E85E4F894367D57617CDEF011BC7DF5A@LAX-EX-MB2.datadirect.datadirectnet.com>
Date:   Thu, 24 May 2018 23:42:26 +0000
From:   Wang Shilong <wshilong@....com>
To:     Andreas Dilger <adilger@...ger.ca>
CC:     Wang Shilong <wangshilong1991@...il.com>,
        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, thanks for good suggestions.
I will work on this.


Thanks,
Shilong




________________________________________
From: Andreas Dilger [adilger@...ger.ca]
Sent: Friday, May 25, 2018 5:10
To: Wang Shilong
Cc: Wang Shilong; Ext4 Developers List; Shuichi Ihara; Theodore Ts'o
Subject: Re: [PATCH 2/2] ext4: don't RO for bitmaps error in default

On May 23, 2018, at 12:38 AM, Wang Shilong <wshilong@....com> wrote:
> Hello Andreas,
>
> Sorry for late reply, I've got some time to finish this:
>
> Andreas Dilger wrote:
>> 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()/warning() if necessary, but I did not catch your other point,
> you mean we should move ext4_mark_group_bitmap_corrupted() to higher-level
> call?

No, my point was to somehow ensure either ext4_mark_group_bitmap_corrupted()
is called (marking the group corrupt and calling ext4_warning() at least
once on that group), OR call ext4_error().

I think the 1/2 patch is totally OK, since it is just moving the
ext4_mark_group_bitmap_corrupted() calls into ext4_group_locked_error().

I think there are a few things to do to improve this patch:
- add a check in ext4_mark_group_bitmap_corrupted() if the bitmap is already
  marked corrupted then don't print anything at all
- add a mount option like "bitmaps={error,warning}" to allow the user to
  decide if corrupt bitmaps should be considered a fatal error or not, and
  check this in ext4_mark_group_bitmap_corrupted()
- rather than removing the ext4_error() calls in the higher code paths,
  add a check ext4_is_bitmap_corrupted() to check if the bitmap error flag
  for that group is set, and don't call ext4_error() if it is.  That avoids
  hitting ext4_error(), but ensures that it will be called if the underlying
  code did not call ext4_mark_group_bitmap_corrupted() for some reason.

Cheers, Andreas