linux-ext4 - Re: [PATCH 0/9 v3] ext4: Punch hole and DAX fixes

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: <20151110000027.GM19199@dastard>
Date:	Tue, 10 Nov 2015 11:00:27 +1100
From:	Dave Chinner <david@...morbit.com>
To:	Jan Kara <jack@...e.cz>
Cc:	"Boylston, Brian" <brian.boylston@....com>,
	Jan Kara <jack@...e.com>, Ted Tso <tytso@....edu>,
	"linux-ext4@...r.kernel.org" <linux-ext4@...r.kernel.org>,
	Ross Zwisler <ross.zwisler@...ux.intel.com>,
	"dan.j.williams@...el.com" <dan.j.williams@...el.com>,
	dchinner@...hat.com
Subject: Re: [PATCH 0/9 v3] ext4: Punch hole and DAX fixes

On Mon, Nov 09, 2015 at 05:51:49PM +0100, Jan Kara wrote:
> On Mon 09-11-15 17:22:56, Jan Kara wrote:
> >   Hi,
> > 
> > On Fri 06-11-15 17:57:04, Boylston, Brian wrote:
> > > I've written a test tool (included below) that exercises page faults on
> > > hole-y portions of an mmapped file.  The file is created, sized using
> > > various methods, mmapped, and then two threads race to write a marker to
> > > different offsets within each mapped page.  Once the threads have
> > > finished marking each page, the pages are checked for the presence of
> > > the markers.
> > > 
> > > With vanilla 4.2 and 4.3 kernels, this test easily exposes corruption on
> > > pmem-backed, DAX-mounted xfs and ext4 file systems.
> > > 
> > > With 4.3 and this ext4 patch set, the data corruption is still seen:
> > > 
> > > $ ./holetest -f /pmem1/brian/holetest 1000
> > > holetest r207
> > 
> > Thanks for the test. I'll try to reproduce locally and have a look why
> > my block zeroing patch didn't work as expected.
> 
> Ah, OK, I see what's going on. So ext4 with my patches still returns
> buffer_new buffer even though it is zeroed out and thus generic DAX code
> still tries to zero out the buffer again which indeed causes the corrution
> (will test everything tomorrow with that code disabled).  Now I have
> decided that block mapping function should return buffer_new buffer even
> though it is zeroed out because e.g. if block zeroing was used for page
> cache writes, we'd still need code in fs/buffer.c to do proper zeroing of
> parts of the block that are not written. And that happens based on
> buffer_new flag.

XFS special cases this for DAX in __xfs_get_blocks():

        if (IS_DAX(inode) && create) {
                ASSERT(!ISUNWRITTEN(&imap));
                /* zeroing is not needed at a higher layer */
                new = 0;
        }

And so will not set the buffer_new() fo rhte DAX case as we've
already directly zeroed the region the DAX code s about to write
into...

> The zeroing code in __dax_fault() needs to go away anyway so whether we
> return buffer_new buffer is not really substantial but I'd like to get some
> agreement and consistency among filesystems in with which flags zeroed
> blocks are returned. Thoughts?

There is no consistency to begin with, especially w.r.t. unwritten
extent behaviour as the upper layers don't all understand that
buffer_unwritten is a valid flag for getblock to return. Hence we
have hacks in XFS setting buffer_new() in strange cases to get the
upper level code to zero stuff that really needs zeroing...

Cheers,

Dave.
-- 
Dave Chinner
david@...morbit.com
--
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