linux-ext4 - Re: [Cluster-devel] iomap preparations for GFS2 v2

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: <CAHc6FU4DWqFVpJE9SJvPbABgbH0goSG9ByFRD2YA1ydLF8_wYA@mail.gmail.com>
Date:   Tue, 19 Jun 2018 17:14:31 +0200
From:   Andreas Gruenbacher <agruenba@...hat.com>
To:     Christoph Hellwig <hch@....de>
Cc:     Steven Whitehouse <swhiteho@...hat.com>,
        cluster-devel <cluster-devel@...hat.com>,
        linux-xfs@...r.kernel.org,
        linux-fsdevel <linux-fsdevel@...r.kernel.org>,
        Dan Williams <dan.j.williams@...el.com>,
        linux-ext4 <linux-ext4@...r.kernel.org>
Subject: Re: [Cluster-devel] iomap preparations for GFS2 v2

On 19 June 2018 at 16:35, Christoph Hellwig <hch@....de> wrote:
> On Tue, Jun 19, 2018 at 01:08:12PM +0200, Andreas Gruenbacher wrote:
>> What I'm seeing in the readpage address space operation is pages which
>> are not PageUptodate(), with a page-size buffer head that is
>> buffer_uptodate(). The filesystem doesn't bother keeping the page
>> flags in sync with the buffer head flags, nothing unusual.
>
> It is in fact highly unusual, as all the generic routines do set
> the page uptodate once all buffers are uptodate.
>
>> When
>> iomap_readpage is called on such a page, it will replace the current
>> contents with what's on disk, losing the changes in memory. So we
>> cannot just call iomap_readpages, we need to check the buffer head
>> flags as well. Or, since the old code is still needed for page size !=
>> block size anyway, we can fall back to that for pages that have
>> buffers for now.
>
> I'd like to understand where that buffer_head comes from, something
> seems fishy here.

Ok, here is one test case that triggered the problem for me.

Starting from commit bd926eb58b13 on the iomap-readpage branch,

  https://git.kernel.org/pub/scm/linux/kernel/git/gfs2/linux-gfs2.git/log/?h=iomap-readpage

with this patch on top which causes iomap_readpage to be called even
for pages with buffers:

--- a/fs/gfs2/aops.c
+++ b/fs/gfs2/aops.c
@@ -511,8 +511,7 @@ static int __gfs2_readpage(void *file, struct page *page)

        int error;

-       if (i_blocksize(page->mapping->host) == PAGE_SIZE &&
-           !page_has_buffers(page)) {
+       if (i_blocksize(page->mapping->host) == PAGE_SIZE) {
                error = iomap_readpage(page, &gfs2_iomap_ops);
        } else if (gfs2_is_stuffed(ip)) {
                error = stuffed_readpage(ip, page);

The following fsx operations, stored as junk.fsxops:

  write 0x11400 0x1800 0x6e6d4 *
  punch_hole 0xfa7a 0x2410 0x0 *
  mapread 0xd000 0x78ea 0x34200 *

Can be replayed as:

  # mkfs.gfs2 -O -b 4096 -p lock_nolock $DEV
  # mount $DEV $MNT
  # ltp/fsx -N 10000 -o 32768 -l 500000 -r 4096 -t 512 -w 512 -Z -W
--replay-ops junk.fsxops $MNT/junk

(Most of the above fsx options could probably be removed ...)

The hole in this example is unaligned, so punch_hole will zero the end
of the first as well as the beginning of the last page of the hole.
This will leave at least the last page of the hole as not
PageUptodate(), with a buffer_uptodate() buffer head. The mapread will
call into iomap_readpage. Because the page has buffers, the
WARN_ON_ONCE(page_has_buffers(page)) in iomap_readpage will trigger.
And iomap_readpage will reread the page from disk, overwriting the
zeroes written by punch_hole. This will cause fsx to complain because
it doesn't see the zeroes it expects.

This could be a bug in __gfs2_punch_hole => gfs2_block_zero_range as
well, but it's not clear to me how.

Andreas