linux-ext4 - Re: ext4_wait_block_bitmap() and ext4_read_block_bitmap_nowait() handle bitmap verification differently

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:	Mon, 7 Oct 2013 10:03:55 -0400
From:	jon ernst <jonernst07@...il.com>
To:	Lukáš Czerner <lczerner@...hat.com>
Cc:	"linux-ext4@...r.kernel.org List" <linux-ext4@...r.kernel.org>
Subject: Re: ext4_wait_block_bitmap() and ext4_read_block_bitmap_nowait()
 handle bitmap verification differently

On Mon, Oct 7, 2013 at 8:45 AM, Lukáš Czerner <lczerner@...hat.com> wrote:
> On Thu, 3 Oct 2013, jon ernst wrote:
>
>> Date: Thu, 3 Oct 2013 22:45:06 -0400
>> From: jon ernst <jonernst07@...il.com>
>> To: "linux-ext4@...r.kernel.org List" <linux-ext4@...r.kernel.org>
>> Subject: ext4_wait_block_bitmap() and ext4_read_block_bitmap_nowait() handle
>>     bitmap verification differently
>>
>> Hi,
>
> Hi Jon,
>
> Btw the patch has some issues and it seems to be badly formatted, or
> even corrupted. You're also missing some Signed-off-by line and the
> subject is not good either. Please see
> Documentation/SubmittingPatches, use git to create patches and use
> email client which does not automatically wrap your lines.

Thanks for your instruction. I will pay attention.
>
>>
>> I found that ext4_wait_block_bitmap() and
>> ext4_read_block_bitmap_nowait() handle bitmap verification
>> differently.
>> wait_block_bitmap() calls ext4_validate_block_bitmap() all the time.
>> But  read_block_bitmap_nowait() checks EXT4_BG_BLOCK_UNINIT, if it
>> meets, it will skip ext4_validate_block_bitmap()
>>
>> In my opinion, they'd better do same thing.
>
> Why ?
>
>> In that way, we can also return "fail" in ext4_valid_block_bitmap()
>> method when we meet FLEX_BG.
>
> This does not make sense at all. Why do you suggest that we should
> "fail" in the case that we have FLEG_BG feature enabled (which is
> default btw) ?
>

I was thinking when we have FLEG_BG feature enabled, we actually don't
have valid bitmap in that block. So semantically , we don't have valid
block bitmap there.

>>
>>
>>
>> diff --git a/fs/ext4/balloc.c b/fs/ext4/balloc.c
>> index dc5d572..366807a 100644
>> --- a/fs/ext4/balloc.c
>> +++ b/fs/ext4/balloc.c
>> @@ -319,7 +319,7 @@ static ext4_fsblk_t ext4_valid_block_bitmap(struct
>> super_block *sb,
>>                  * or it has to also read the block group where the bitmaps
>>                  * are located to verify they are set.
>>                  */
>> -               return 0;
>> +               return 1;
>>         }
>>         group_first_block = ext4_group_first_block_no(sb, block_group);
>>
>> @@ -472,8 +472,12 @@ int ext4_wait_block_bitmap(struct super_block
>> *sb, ext4_group_t block_group,
>>                 return 1;
>>         }
>>         clear_buffer_new(bh);
>> -       /* Panic or remount fs read-only if block bitmap is invalid */
>> -       ext4_validate_block_bitmap(sb, desc, block_group, bh);
>> +
>> +       if (desc->bg_flags & cpu_to_le16(EXT4_BG_BLOCK_UNINIT)) {
>> +        return 0;
>
> This is wrong from multiple reasons. First of all you're not holding
> group lock so what is preventing others to actually initialize the
> bitmap before you return 0 ?
>
Got it.

> Secondly, uninit group will never get that far, because it'll be
> initialized in ext4_read_block_bitmap_nowait() and we will not
> actually need to wait for the buffer.
>

Thank you! Very helpful information.

Best,
Jon

> Thanks!
> -Lukas
>
>> +        }
>> +       /* Panic or remount fs read-only if block bitmap is invalid */
>> +       ext4_validate_block_bitmap(sb, desc, block_group, bh);
>>         /* ...but check for error just in case errors=continue. */
>>         return !buffer_verified(bh);
>>  }
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
>> the body of a message to majordomo@...r.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html