linux-ext4 - Re: RFC PATCH: ext4 no journal corruption with locale-gen

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]

Message-ID: <6601abe90906291050s4bc268b2mca1f2addb45fddb1@mail.gmail.com>
Date:	Mon, 29 Jun 2009 10:50:26 -0700
From:	Curt Wohlgemuth <curtw@...gle.com>
To:	Theodore Tso <tytso@....edu>
Cc:	ext4 development <linux-ext4@...r.kernel.org>
Subject: Re: RFC PATCH: ext4 no journal corruption with locale-gen

Ted, did you have any comment or objections to this patch?

Thanks,
Curt


On Mon, Jun 22, 2009 at 9:42 AM, Curt Wohlgemuth<curtw@...gle.com> wrote:
> Hi Ted:
>
> I think the following patch is sufficient.  It explicitly sets the aops to
> ext4_writeback_aops if there is no delayed allocation and no journal.
>
> I tested the locale-gen example with all combinations of
>
>   data=writeback
>   data=ordered
>   data=journal
>   <no journal at all>
>
> and
>
>   delalloc
>   nodelalloc
>
> and it works correctly now.  The paths for writeback seem fine to me for an
> inode w/o a journal.
>
>
>       Signed-off-by: Curt Wohlgemuth <curtw@...gle.com>
> ---
> --- 2.6.26/fs/ext4/inode.c.orig 2009-06-09 20:05:27.000000000 -0700
> +++ 2.6.26/fs/ext4/inode.c      2009-06-22 08:55:13.000000000 -0700
> @@ -3442,15 +3442,12 @@ static const struct address_space_operat
>
>  void ext4_set_aops(struct inode *inode)
>  {
> -       if (ext4_should_order_data(inode) &&
> -               test_opt(inode->i_sb, DELALLOC))
> +       if (test_opt(inode->i_sb, DELALLOC))
>                inode->i_mapping->a_ops = &ext4_da_aops;
>        else if (ext4_should_order_data(inode))
>                inode->i_mapping->a_ops = &ext4_ordered_aops;
> -       else if (ext4_should_writeback_data(inode) &&
> -                test_opt(inode->i_sb, DELALLOC))
> -               inode->i_mapping->a_ops = &ext4_da_aops;
> -       else if (ext4_should_writeback_data(inode))
> +       else if (ext4_should_writeback_data(inode) ||
> +                                EXT4_JOURNAL(inode) == NULL)
>                inode->i_mapping->a_ops = &ext4_writeback_aops;
>        else
>                inode->i_mapping->a_ops = &ext4_journalled_aops;
>
>
> On Wed, Jun 17, 2009 at 4:46 PM, Theodore Tso<tytso@....edu> wrote:
>> Hi Curt,
>>
>> Thanks for your analysis of the bug.  The reason for the strange logic
>> in ext4_set_aops() is because at the moment the code doesn't support
>> the combination of data=journalled && delalloc.  That's why it was
>> explicitly checking for ext4_should_order_data() and
>> ext4_should_writeback_data().
>>
>> We have a check for this in ext4_fill_super(), so your patch should be
>> safe, since the combination of ext4_should_journal_data &&
>> test_opt(inode->i_sb, DELALLOC) should never happen.
>>
>> As to your question of whether the nodelalloc and nojournal case
>> should really be ext4_journalled_aops, I suspect ext4_writeback_aops
>> makes more sense.  I haven't audited all of the code paths to make
>> sure they DTRT in the non-journalled case yet, though.
>>
>>                                                        - Ted
>> --
>> 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