Re: [RFC] block integrity: Fix write after checksum calculation problem

[Chris Mason]

Excerpts from Jan Kara's message of 2011-03-21 10:04:51 -0400:
> On Fri 18-03-11 17:07:55, Darrick J. Wong wrote:
> > > > Ok, here's what I have so far.  I took everyone's suggestions of where to add
> > > > calls to wait_on_page_writeback, which seems to handle the multiple-write case
> > > > adequately.  Unfortunately, it is still possible to generate checksum errors by
> > > > scribbling furiously on a mmap'd region, even after adding the writeback wait
> > > > in the ext4 writepage function.  Oddly, I couldn't break btrfs with mmap by
> > > > removing its wait_for_page_writeback call, so I suspect there's a bit more
> > > > going on in btrfs than I've been able to figure out.
> > 
> > I wonder, is it possible for this to happen:
> > 
> > 1. Thread A mmaps a page and tries to write to it.  ext4_page_mkwrite executes,
> >    but there's no ongoing writeback, so it returns without delay.
> > 2. Thread A starts writing furiously to the page.
> > 3. Thread B runs fsync() or something that results in the page being
> >    checksummed and scheduled for writeout.
> > 4. Thread A continues to write furiously(!) on that same page before the
> >    controller finishes the DMA transfer.
> > 5. Disk gets the page, which now doesn't match its checksum, and *boom*
>   What happens on writepage (see mm/page-writeback.c:write_cache_pages())
> is:
>   lock_page(page)
>   ...
>   clear_page_dirty_for_io() - removes PageDirty, marks page as read-only in
>     PTE
>   ...
>   set_page_writeback() (happens e.g. in __block_write_full_page() called
> from filesystem's writepage implementation).
>   unlock_page(page)
> 
>   So if you compute the checksum after set_page_writeback() is done in the
> writepage() implementation (you cannot use __block_write_full_page() in
> that case) and you call wait_on_page_writeback() in ext4_page_mkwrite()
> under page lock, you should be safe. If you do all this and still see
> errors, something is broken I'd say...

Looking at the ext4_page_mkwrite, it does this:

lock the page
check for holes
unlock the page
if (no_holes)
	return;

write_begin/write_end
return

So, to have page_mkwrite work, you need to wait for writeback with the
page locked in both the no holes case and after the
write_begin/write_end.  write_begin will dirty the page, so someone can
wander in and start the IO while we are still in page_mkwrite.

This is untested and uncompiled, but it should
do the trick.

Jan, did you get rid of all the buffer head based writeback for
data=ordered in ext4?  That's my only other idea, that someone is doing
writeback directly without taking the page lock.

diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index 9f7f9e4..8a75e12 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -5880,6 +5880,7 @@ int ext4_page_mkwrite(struct vm_area_struct *vma, struct vm_fault *vmf)
 	if (page_has_buffers(page)) {
 		if (!walk_page_buffers(NULL, page_buffers(page), 0, len, NULL,
 					ext4_bh_unmapped)) {
+			wait_on_page_writeback(page);
 			unlock_page(page);
 			goto out_unlock;
 		}
@@ -5901,6 +5902,16 @@ int ext4_page_mkwrite(struct vm_area_struct *vma, struct vm_fault *vmf)
 	if (ret < 0)
 		goto out_unlock;
 	ret = 0;
+
+	/*
+	 * write_begin/end might have created a dirty page and someone
+	 * could wander in and start the IO.  Make sure that hasn't
+	 * happened
+	 */
+	lock_page(page);
+	wait_on_page_writeback(page);
+	unlock_page(page);
+
 out_unlock:
 	if (ret)
 		ret = VM_FAULT_SIGBUS;

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/