linux-kernel - Re: [PATCH] mm: fix page_mkclean_one (was: 2.6.19 file content corruption on ext3)

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: <Pine.LNX.4.64.0612201548410.3576@woody.osdl.org>
Date:	Wed, 20 Dec 2006 15:55:14 -0800 (PST)
From:	Linus Torvalds <torvalds@...l.org>
To:	Andrew Morton <akpm@...l.org>
cc:	Martin Michlmayr <tbm@...ius.com>,
	Peter Zijlstra <a.p.zijlstra@...llo.nl>,
	Hugh Dickins <hugh@...itas.com>,
	Nick Piggin <nickpiggin@...oo.com.au>,
	Arjan van de Ven <arjan@...radead.org>,
	Andrei Popa <andrei.popa@...eo.ro>,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
	Florian Weimer <fw@...eb.enyo.de>,
	Marc Haber <mh+linux-kernel@...schlus.de>,
	Martin Schwidefsky <schwidefsky@...ibm.com>,
	Heiko Carstens <heiko.carstens@...ibm.com>,
	Arnd Bergmann <arnd.bergmann@...ibm.com>,
	gordonfarquharson@...il.com,
	"Chen, Kenneth W" <kenneth.w.chen@...el.com>
Subject: Re: [PATCH] mm: fix page_mkclean_one (was: 2.6.19 file content
 corruption on ext3)



On Wed, 20 Dec 2006, Andrew Morton wrote:
> 
> > +void cancel_dirty_page(struct page *page, unsigned int account_size)
> > +{
> > +	/* If we're cancelling the page, it had better not be mapped any more */
> > +	if (page_mapped(page)) {
> > +		static unsigned int warncount;
> > +
> > +		WARN_ON(++warncount < 5);
> > +	}
> > +		
> > +	if (TestClearPageDirty(page) && account_size)
> > +		task_io_account_cancelled_write(account_size);
> > +}
> 
> This doesn't clear the radix-tree dirty tags.  I'm not sure what effect
> that would have on a truncated mapping.  Perhaps just a bit of extra work
> in radix-tree lookup during writeback.

This should _only_ be a valid thing to do when we're removing the page 
from a mapping anyway, so I'd most definitely hope that the code 
immediately after (or before) will have done a "remove_from_page_cache()"

In which case the tags should not matter.

There is _no_ excuse for cancelling a page and leaving it in the page 
cache that I can see. Because your page contents will be _indeterminate_.

> > @@ -386,12 +399,8 @@ int invalidate_inode_pages2_range(struct address_space *mapping,
> 
> invalidate_complete_page2() is pretty gruesome.  We're handling the case
> where someone went and redirtied the page (and hence its buffers) after the
> invalidate_inode_pages2() caller (generic_file_direct_IO) synced it to
> disk.
> 
> I'd prefer to just fail the direct-io if someone did that, but then
> people's tests fail and they whine.

So with my change, afaik, we will just return EIO to the invalidate, and 
do the write. Which should be ok. In fact, it appears to be the only 
possibly valid thing to do.

It really boils down to that same thing: if you remove the dirty bit, 
there is NO CONCEIVABLE GOOD THING YOU CAN DO EXCEPT FOR:
 - do the damn IO already ("clear_page_dirty_for_io()")
 - truncate the page (unmap and destroy it both from page cache AND from 
   any user-visible filesystem cases)

Anything else is simpyl a bug. Always has been. My patch just makes that 
very clear.

> With your change I think what'll happen is that we'll correctly handle the
> case where the page and its buffers are dirty (it gets left in place), but
> we'll needlessy fail in the case where the page is dirty but the buffers
> are clean.  How important that will be in practice I do not know.  People
> will get -EIOs where they used not to.

People will now get -EIO where they used to get an inconsistent system 
image. I really think it sounds like an improvement.

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