linux-kernel - Re: [PATCH v10 09/21] Replace the XIP page fault handler with the DAX page fault handler

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:	Wed, 24 Sep 2014 11:43:07 -0400
From:	Matthew Wilcox <willy@...ux.intel.com>
To:	Dave Chinner <david@...morbit.com>
Cc:	Matthew Wilcox <willy@...ux.intel.com>,
	Matthew Wilcox <matthew.r.wilcox@...el.com>,
	linux-fsdevel@...r.kernel.org, linux-mm@...ck.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v10 09/21] Replace the XIP page fault handler with the
 DAX page fault handler

On Thu, Sep 11, 2014 at 01:09:26PM +1000, Dave Chinner wrote:
> On Wed, Sep 10, 2014 at 11:23:37AM -0400, Matthew Wilcox wrote:
> > On Wed, Sep 03, 2014 at 05:47:24PM +1000, Dave Chinner wrote:
> > > > +	error = get_block(inode, block, &bh, 0);
> > > > +	if (!error && (bh.b_size < PAGE_SIZE))
> > > > +		error = -EIO;
> > > > +	if (error)
> > > > +		goto unlock_page;
> > > 
> > > page fault into unwritten region, returns buffer_unwritten(bh) ==
> > > true. Hence buffer_written(bh) is false, and we take this branch:
> > > 
> > > > +	if (!buffer_written(&bh) && !vmf->cow_page) {
> > > > +		if (vmf->flags & FAULT_FLAG_WRITE) {
> > > > +			error = get_block(inode, block, &bh, 1);
> > > 
> > > Exactly what are you expecting to happen here? We don't do
> > > allocation because there are already unwritten blocks over this
> > > extent, and so bh will be unchanged when returning. i.e. it will
> > > still be mapping an unwritten extent.
> > 
> > I was expecting calling get_block() on an unwritten extent to convert it
> > to a written extent.  Your suggestion below of using b_end_io() to do that
> > is a better idea.
> > 
> > So this should be:
> > 
> > 	if (!buffer_mapped(&bh) && !vmf->cow_page) {
> > 
> > ... right?
> 
> Yes, that is the conclusion I reached as well. ;)

Now I know why I was expecting get_block() on an unwritten extent to
convert it to a written extent.  That's the way ext4 behaves!

[  236.660772] got bh ffffffffa06e3bd0 1000
[  236.660814] got bh for write ffffffffa06e3bd0 60
[  236.660821] calling end_io ffffffffa06e3bd0 60

(1000 is BH_Unwritten, 60 is BH_Mapped | BH_New)

The code producing this output:

        error = get_block(inode, block, &bh, 0);
printk("got bh %p %lx\n", bh.b_end_io, bh.b_state);
        if (!error && (bh.b_size < PAGE_SIZE))
                error = -EIO;
        if (error)
                goto unlock_page;

        if (!buffer_mapped(&bh) && !vmf->cow_page) {
                if (vmf->flags & FAULT_FLAG_WRITE) {
                        error = get_block(inode, block, &bh, 1);
printk("got bh for write %p %lx\n", bh.b_end_io, bh.b_state);

# xfs_io -f -c "truncate 20k" -c "fiemap -v" -c "falloc 0 20k" -c "fiemap -v" -c "mmap -w 0 20k" -c "fiemap -v" -c "mwrite 4k 4k" -c "fiemap -v" /mnt/ram0/b
/mnt/ram0/b:
/mnt/ram0/b:
 EXT: FILE-OFFSET      BLOCK-RANGE      TOTAL FLAGS
   0: [0..39]:         263176..263215      40 0x801
/mnt/ram0/b:
 EXT: FILE-OFFSET      BLOCK-RANGE      TOTAL FLAGS
   0: [0..39]:         263176..263215      40 0x801
/mnt/ram0/b:
 EXT: FILE-OFFSET      BLOCK-RANGE      TOTAL FLAGS
   0: [0..39]:         263176..263215      40   0x1

Actually, this looks wrong ... ext4 should only have converted one block
of the extent to written, not all of it.  I think that means ext4 is
exposing stale data :-(  I'll keep digging.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/