| 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
| ||
|
Message-ID: <20070308055004.GB24605@wotan.suse.de>
Date: Thu, 8 Mar 2007 06:50:04 +0100
From: Nick Piggin <npiggin@...e.de>
To: Andrew Morton <akpm@...ux-foundation.org>
Cc: miklos@...redi.hu,
Linux Memory Management List <linux-mm@...ck.org>,
linux-kernel@...r.kernel.org,
Linus Torvalds <torvalds@...ux-foundation.org>
Subject: Re: [patch 8/6] mm: fix cpdfio vs fault race
On Wed, Mar 07, 2007 at 01:02:14PM -0800, Andrew Morton wrote:
> On Wed, 7 Mar 2007 12:31:21 +0100
> Nick Piggin <npiggin@...e.de> wrote:
>
> > Index: linux-2.6/mm/memory.c
> > ===================================================================
> > --- linux-2.6.orig/mm/memory.c
> > +++ linux-2.6/mm/memory.c
> > @@ -1664,6 +1664,15 @@ gotten:
> > unlock:
> > pte_unmap_unlock(page_table, ptl);
> > if (dirty_page) {
> > + /*
> > + * Yes, Virginia, this is actually required to prevent a race
> > + * with clear_page_dirty_for_io() from clearing the page dirty
> > + * bit after it clear all dirty ptes, but before a racing
> > + * do_wp_page installs a dirty pte.
> > + *
> > + * do_no_page is protected similarly.
> > + */
> > + wait_on_page_locked(dirty_page);
> > set_page_dirty_balance(dirty_page);
> > put_page(dirty_page);
> > }
> > @@ -2316,6 +2325,7 @@ retry:
> > unlock:
> > pte_unmap_unlock(page_table, ptl);
> > if (dirty_page) {
> > + wait_on_page_locked(dirty_page);
> > set_page_dirty_balance(dirty_page);
> > put_page(dirty_page);
> > }
> > Index: linux-2.6/mm/page-writeback.c
>
> now that's scary - applying this on top of your
> lock-the-page-in-the-fault-handler patches gives:
>
> if (dirty_page) {
> /*
> * Yes, Virginia, this is actually required to prevent a race
> * with clear_page_dirty_for_io() from clearing the page dirty
> * bit after it clear all dirty ptes, but before a racing
> * do_wp_page installs a dirty pte.
> *
> * do_no_page is protected similarly.
> */
> wait_on_page_locked(dirty_page);
> wait_on_page_locked(dirty_page);
> set_page_dirty_balance(dirty_page);
> put_page(dirty_page);
> }
>
> One wonders how on earth patch(1) managed to do that. If it has inserted
> the comment twice as well then it might be explicable..
Ouch ;) Yeah that patch I sent was supposed to apply underneath
the previous ones, sorry I wasn't clear.
> Oh well, let's try this:
Yeah that looks like the correct one for applying on top. Thanks.
>
> From: Nick Piggin <npiggin@...e.de>
>
> Fix msync data loss and (less importantly) dirty page accounting
> inaccuracies due to the race remaining in clear_page_dirty_for_io().
>
> The deleted comment explains what the race was, and the added comments
> explain how it is fixed.
>
> Signed-off-by: Nick Piggin <npiggin@...e.de>
> Cc: Linus Torvalds <torvalds@...ux-foundation.org>
> Cc: Miklos Szeredi <miklos@...redi.hu>
> Signed-off-by: Andrew Morton <akpm@...l.org>
> ---
>
> mm/memory.c | 9 +++++++++
> mm/page-writeback.c | 17 ++++++++++++-----
> 2 files changed, 21 insertions(+), 5 deletions(-)
>
> diff -puN mm/memory.c~mm-fix-cpdfio-vs-fault-race mm/memory.c
> --- a/mm/memory.c~mm-fix-cpdfio-vs-fault-race
> +++ a/mm/memory.c
> @@ -1669,6 +1669,15 @@ gotten:
> unlock:
> pte_unmap_unlock(page_table, ptl);
> if (dirty_page) {
> + /*
> + * Yes, Virginia, this is actually required to prevent a race
> + * with clear_page_dirty_for_io() from clearing the page dirty
> + * bit after it clear all dirty ptes, but before a racing
> + * do_wp_page installs a dirty pte.
> + *
> + * do_no_page is protected similarly.
> + */
> + wait_on_page_locked(dirty_page);
> set_page_dirty_balance(dirty_page);
> put_page(dirty_page);
> }
> diff -puN mm/page-writeback.c~mm-fix-cpdfio-vs-fault-race mm/page-writeback.c
> --- a/mm/page-writeback.c~mm-fix-cpdfio-vs-fault-race
> +++ a/mm/page-writeback.c
> @@ -903,6 +903,8 @@ int clear_page_dirty_for_io(struct page
> {
> struct address_space *mapping = page_mapping(page);
>
> + BUG_ON(!PageLocked(page));
> +
> if (mapping && mapping_cap_account_dirty(mapping)) {
> /*
> * Yes, Virginia, this is indeed insane.
> @@ -928,14 +930,19 @@ int clear_page_dirty_for_io(struct page
> * We basically use the page "master dirty bit"
> * as a serialization point for all the different
> * threads doing their things.
> - *
> - * FIXME! We still have a race here: if somebody
> - * adds the page back to the page tables in
> - * between the "page_mkclean()" and the "TestClearPageDirty()",
> - * we might have it mapped without the dirty bit set.
> */
> if (page_mkclean(page))
> set_page_dirty(page);
> + /*
> + * We carefully synchronise fault handlers against
> + * installing a dirty pte and marking the page dirty
> + * at this point. We do this by having them hold the
> + * page lock at some point after installing their
> + * pte, but before marking the page dirty.
> + * Pages are always locked coming in here, so we get
> + * the desired exclusion. See mm/memory.c:do_wp_page()
> + * for more comments.
> + */
> if (TestClearPageDirty(page)) {
> dec_zone_page_state(page, NR_FILE_DIRTY);
> return 1;
> _
-
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/
Powered by blists - more mailing lists