linux-ext4 - Re: [PATCH 3/7] ext2: Avoid DAX zeroing to corrupt data

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:	Tue, 17 May 2016 06:52:47 +0000
From:	"Verma, Vishal L" <vishal.l.verma@...el.com>
To:	"ross.zwisler@...ux.intel.com" <ross.zwisler@...ux.intel.com>,
	"jack@...e.cz" <jack@...e.cz>
CC:	"linux-ext4@...r.kernel.org" <linux-ext4@...r.kernel.org>,
	"Williams, Dan J" <dan.j.williams@...el.com>,
	"linux-nvdimm@...ts.01.org" <linux-nvdimm@...ts.01.org>,
	"tytso@....edu" <tytso@....edu>,
	"linux-fsdevel@...r.kernel.org" <linux-fsdevel@...r.kernel.org>
Subject: Re: [PATCH 3/7] ext2: Avoid DAX zeroing to corrupt data

On Mon, 2016-05-16 at 17:22 +0200, Jan Kara wrote:
> On Thu 12-05-16 12:45:22, Ross Zwisler wrote:
> > 
> > On Wed, May 11, 2016 at 11:58:49AM +0200, Jan Kara wrote:
> > > 
> > > Currently ext2 zeroes any data blocks allocated for DAX inode
> > > however it
> > > still returns them as BH_New. Thus DAX code zeroes them again in
> > > dax_insert_mapping() which can possibly overwrite the data that
> > > has been
> > > already stored to those blocks by a racing dax_io(). Avoid
> > > marking
> > > pre-zeroed buffers as new.
> > > 
> > > Reviewed-by: Ross Zwisler <ross.zwisler@...ux.intel.com>
> > > Signed-off-by: Jan Kara <jack@...e.cz>
> > > ---
> > >  fs/ext2/inode.c | 4 ++--
> > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/fs/ext2/inode.c b/fs/ext2/inode.c
> > > index 6bd58e6ff038..1f07b758b968 100644
> > > --- a/fs/ext2/inode.c
> > > +++ b/fs/ext2/inode.c
> > > @@ -745,11 +745,11 @@ static int ext2_get_blocks(struct inode
> > > *inode,
> > >  			mutex_unlock(&ei->truncate_mutex);
> > >  			goto cleanup;
> > >  		}
> > > -	}
> > > +	} else
> > > +		set_buffer_new(bh_result);
> > >  
> > >  	ext2_splice_branch(inode, iblock, partial,
> > > indirect_blks, count);
> > >  	mutex_unlock(&ei->truncate_mutex);
> > > -	set_buffer_new(bh_result);
> > >  got_it:
> > >  	map_bh(bh_result, inode->i_sb, le32_to_cpu(chain[depth-
> > > 1].key));
> > >  	if (count > blocks_to_boundary)
> > > -- 
> > > 2.6.6
> > Interestingly this change is causing a bunch of xfstests
> > regressions for me
> > with ext2 + DAX.  All of these tests pass without this one change.
> Good catch. Attached patch fixes this issue for me. Preferably it
> should be
> merged before the above ext2 change.
> 
> 								Honza

Hey Jan,

In my patch 3 of the error handling series, I have:

-               err = dax_clear_sectors(inode->i_sb->s_bdev,
-                               le32_to_cpu(chain[depth-1].key) <<
-                               (inode->i_blkbits - 9),
-                               1 << inode->i_blkbits);
+               err = sb_issue_zeroout(inode->i_sb,
+                               le32_to_cpu(chain[depth-1].key), 1, GFP_NOFS);

Does this mean I have to change to send the sb_issue_zeroout for
'count' blocks.. i.e.

-               err = dax_clear_sectors(inode->i_sb->s_bdev,
-                               le32_to_cpu(chain[depth-1].key) <<
-                               (inode->i_blkbits - 9),
-                               1 << inode->i_blkbits);
+               err = sb_issue_zeroout(inode->i_sb,
+                               le32_to_cpu(chain[depth-1].key), count, GFP_NOFS);

If so, I'll update my series tomorrow to include in both of these changes.

Thanks,
	-Vishal