linux-ext4 - Re: ext4: Fix clear_buffer_dirty() call for mblk_io

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, 31 Jan 2011 16:15:33 -0800
From:	Curt Wohlgemuth <curtw@...gle.com>
To:	ext4 development <linux-ext4@...r.kernel.org>
Subject: Re: ext4: Fix clear_buffer_dirty() call for mblk_io_submit writepages path

Sorry:  I forgot to mention that I tested this on 2.6.37 under KVM
with the postgresql script specified by Ted in
1449032be17abb69116dbc393f67ceb8bd034f92 -- without this commit, and
hence by default using the multiblock writepages submittal code.

Without the patch, I'd hit the corruption problem about 50-70% of the
time.  With the patch, I executed the script > 100 times with no
corruption seen.

Curt

On Mon, Jan 31, 2011 at 12:44 PM, Curt Wohlgemuth <curtw@...gle.com> wrote:
> The ext4 code path to bundle multiple pages for writeback in
> ext4_bio_write_page() had a bug:  we should be clearing buffer head dirty
> flags before we submit the bio, not in the completion routine.
>
> Also don't deference the bio in ext4_end_bio() after the bio_put() call.
>
> (I also added a blank line in ext4_bio_write_page(), 'cause my eyes
> constantly confused the complex 'for' conditions from the first statement
> in the block.)
>
> Signed-off-by: Curt Wohlgemuth <curtw@...gle.com>
>
> ---
>
> diff --git a/fs/ext4/page-io.c b/fs/ext4/page-io.c
> index 6e0e99b..c3d490f 100644
> --- a/fs/ext4/page-io.c
> +++ b/fs/ext4/page-io.c
> @@ -193,6 +193,7 @@ static void ext4_end_bio(struct bio *bio, int error)
>        struct inode *inode;
>        unsigned long flags;
>        int i;
> +       sector_t bi_sector = bio->bi_sector;
>
>        BUG_ON(!io_end);
>        bio->bi_private = NULL;
> @@ -210,9 +211,7 @@ static void ext4_end_bio(struct bio *bio, int error)
>                if (error)
>                        SetPageError(page);
>                BUG_ON(!head);
> -               if (head->b_size == PAGE_CACHE_SIZE)
> -                       clear_buffer_dirty(head);
> -               else {
> +               if (head->b_size != PAGE_CACHE_SIZE) {
>                        loff_t offset;
>                        loff_t io_end_offset = io_end->offset + io_end->size;
>
> @@ -224,7 +223,6 @@ static void ext4_end_bio(struct bio *bio, int error)
>                                        if (error)
>                                                buffer_io_error(bh);
>
> -                                       clear_buffer_dirty(bh);
>                                }
>                                if (buffer_delay(bh))
>                                        partial_write = 1;
> @@ -260,7 +258,7 @@ static void ext4_end_bio(struct bio *bio, int error)
>                             (unsigned long long) io_end->offset,
>                             (long) io_end->size,
>                             (unsigned long long)
> -                            bio->bi_sector >> (inode->i_blkbits - 9));
> +                            bi_sector >> (inode->i_blkbits - 9));
>        }
>
>        /* Add the io_end to per-inode completed io list*/
> @@ -383,6 +381,7 @@ int ext4_bio_write_page(struct ext4_io_submit *io,
>
>        blocksize = 1 << inode->i_blkbits;
>
> +       BUG_ON(!PageLocked(page));
>        BUG_ON(PageWriteback(page));
>        set_page_writeback(page);
>        ClearPageError(page);
> @@ -400,12 +399,14 @@ int ext4_bio_write_page(struct ext4_io_submit *io,
>        for (bh = head = page_buffers(page), block_start = 0;
>             bh != head || !block_start;
>             block_start = block_end, bh = bh->b_this_page) {
> +
>                block_end = block_start + blocksize;
>                if (block_start >= len) {
>                        clear_buffer_dirty(bh);
>                        set_buffer_uptodate(bh);
>                        continue;
>                }
> +               clear_buffer_dirty(bh);
>                ret = io_submit_add_bh(io, io_page, inode, wbc, bh);
>                if (ret) {
>                        /*
>
--
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