linux-ext4 - Re: [RFC PATCH] mark buffer_head mapping preallocate area as new during write

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: <20090428093145.GA13719@skywalker>
Date:	Tue, 28 Apr 2009 15:01:45 +0530
From:	"Aneesh Kumar K.V" <aneesh.kumar@...ux.vnet.ibm.com>
To:	Mingming Cao <cmm@...ibm.com>
Cc:	tytso@....edu, sandeen@...hat.com, linux-ext4@...r.kernel.org
Subject: Re: [RFC PATCH] mark buffer_head mapping preallocate area as new
	during write_begin with delayed allocation

On Tue, Apr 28, 2009 at 09:50:49AM +0530, Aneesh Kumar K.V wrote:
> On Mon, Apr 27, 2009 at 04:04:54PM -0700, Mingming Cao wrote:
> .....
> 
> > 
> > Index: linux-2.6.28-rc6/fs/ext4/inode.c
> > ===================================================================
> > --- linux-2.6.28-rc6.orig/fs/ext4/inode.c	2009-03-12 10:21:05.000000000 -0700
> > +++ linux-2.6.28-rc6/fs/ext4/inode.c	2009-04-27 14:35:21.000000000 -0700
> > @@ -2177,7 +2177,10 @@ static int ext4_da_get_block_prep(struct
> >  		set_buffer_new(bh_result);
> >  		set_buffer_delay(bh_result);
> >  	} else if (ret > 0) {
> > +		if (buffer_unwritten(bh_result))
> > +			set_buffer_new(bh_result);
> >  		bh_result->b_size = (ret << inode->i_blkbits);
> > +		bh_result->b_bdev = inode->i_sb->s_bdev;
> 
> 
> Updated patch to set bh_result->b_dev. I also added comments in the
> source to explain whey we need to mark buffer_head new. Also updated
> single line patch summary. I will send the update (-v2) patch.

Looking at the source again i guess setting just b_dev is not enough.
unmap_underlying_metadata looks at the mapping block number, which we
don't have in case on unwritten buffer_head. How about the below patch ?
It involve vfs changes. But i guess it is correct with respect to the
meaning of BH_New (Disk mapping was newly created by get_block). I guess
BH_New implies BH_Mapped.

I haven't tested the patch yet. Also it should be split into multiple
patches. It also a fix a problem where we missed an
unamp_underlying_metadata in case of delayed allocated blocks. I guess
that can also cause corruption with delayed allocation.


From: Aneesh Kumar K.V <aneesh.kumar@...ux.vnet.ibm.com>
Subject: [PATCH -V3] ext4: Fix sub-block zeroing for buffered writes into unwritten extents.

We need to mark the  buffer_head mapping prealloc space
as new during write_begin. Otherwise we don't zero out the
page cache content properly for a partial write. This will
cause file corruption with preallocation.

Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@...ux.vnet.ibm.com>

---
 fs/buffer.c     |    9 ++++++++-
 fs/ext4/inode.c |    8 +++++---
 2 files changed, 13 insertions(+), 4 deletions(-)

diff --git a/fs/buffer.c b/fs/buffer.c
index b3e5be7..13f0d52 100644
--- a/fs/buffer.c
+++ b/fs/buffer.c
@@ -1867,15 +1867,22 @@ static int __block_prepare_write(struct inode *inode, struct page *page,
 			err = get_block(inode, block, bh, 1);
 			if (err)
 				break;
-			if (buffer_new(bh)) {
+			if (buffer_new(bh))
 				unmap_underlying_metadata(bh->b_bdev,
 							bh->b_blocknr);
+			if (buffer_new(bh) || buffer_unwritten(bh) ||
+					buffer_delay(bh)) {
 				if (PageUptodate(page)) {
 					clear_buffer_new(bh);
 					set_buffer_uptodate(bh);
 					mark_buffer_dirty(bh);
 					continue;
 				}
+				/*
+				 * sub-block writes into unwritten or
+				 * delayed buffer should result in zero out
+				 * of the rest of the buffer
+				 */
 				if (block_end > to || block_start < from)
 					zero_user_segments(page,
 						to, block_end,
diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index e91f978..504afb7 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -1892,13 +1892,17 @@ static void mpage_put_bnr_to_bhs(struct mpage_da_data *mpd, sector_t logical,
 				if (buffer_delay(bh)) {
 					bh->b_blocknr = pblock;
 					clear_buffer_delay(bh);
+					set_buffer_mapped(bh);
 					bh->b_bdev = inode->i_sb->s_bdev;
+					unmap_underlying_metadata(bh->b_bdev,
+								pblock);
 				} else if (buffer_unwritten(bh)) {
 					bh->b_blocknr = pblock;
 					clear_buffer_unwritten(bh);
 					set_buffer_mapped(bh);
-					set_buffer_new(bh);
 					bh->b_bdev = inode->i_sb->s_bdev;
+					unmap_underlying_metadata(bh->b_bdev,
+								pblock);
 				} else if (buffer_mapped(bh))
 					BUG_ON(bh->b_blocknr != pblock);
 
@@ -2318,8 +2322,6 @@ static int ext4_da_get_block_prep(struct inode *inode, sector_t iblock,
 			/* not enough space to reserve */
 			return ret;
 
-		map_bh(bh_result, inode->i_sb, 0);
-		set_buffer_new(bh_result);
 		set_buffer_delay(bh_result);
 	} else if (ret > 0) {
 		bh_result->b_size = (ret << inode->i_blkbits);
-- 
tg: (2084a87..) preallocate_corruption (depends on: ext4_lock_group_conversion)

--
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