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]

Date:	Wed, 20 Dec 2006 15:55:40 +0100
From:	Martin Schwidefsky <schwidefsky@...ibm.com>
To:	Peter Zijlstra <a.p.zijlstra@...llo.nl>
Cc:	Arjan van de Ven <arjan@...radead.org>,
	Linus Torvalds <torvalds@...l.org>,
	Andrei Popa <andrei.popa@...eo.ro>,
	Andrew Morton <akpm@...l.org>,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
	Hugh Dickins <hugh@...itas.com>,
	Florian Weimer <fw@...eb.enyo.de>,
	Marc Haber <mh+linux-kernel@...schlus.de>,
	Martin Michlmayr <tbm@...ius.com>,
	Heiko Carstens <heiko.carstens@...ibm.com>,
	Arnd Bergmann <arnd.bergmann@...ibm.com>
Subject: Re: [PATCH] mm: fix page_mkclean_one (was: 2.6.19 file content
	corruption on ext3)

On Wed, 2006-12-20 at 12:26 +0100, Peter Zijlstra wrote:
> fix page_mkclean_one()
> 
> it had several issues:
>  - it failed to flush the cache
>  - it failed to flush the tlb
>  - it failed to do s390 (s390 guys, please verify this is now correct)

Sorry, page_mkclean is broken for s390. But it has already been broken
before your change. It is only more broken now.

> @@ -440,22 +440,23 @@ static int page_mkclean_one(struct page
>  	if (address == -EFAULT)
>  		goto out;
> 
> -	pte = page_check_address(page, mm, address, &ptl);
> -	if (!pte)
> +	ptep = page_check_address(page, mm, address, &ptl);
> +	if (!ptep)
>  		goto out;
> 
> -	if (!pte_dirty(*pte) && !pte_write(*pte))
> -		goto unlock;
> -
> -	entry = ptep_get_and_clear(mm, address, pte);
> -	entry = pte_mkclean(entry);
> -	entry = pte_wrprotect(entry);
> -	ptep_establish(vma, address, pte, entry);
> -	lazy_mmu_prot_update(entry);
> -	ret = 1;
> +	while (pte_dirty(*ptep) || pte_write(*ptep)) {
> +		pte_t entry = ptep_get_and_clear(mm, address, ptep);
> +		flush_cache_page(vma, address, pte_pfn(entry));
> +		flush_tlb_page(vma, address);
> +		(void)page_test_and_clear_dirty(page); /* do the s390 thing */
> +		entry = pte_wrprotect(entry);
> +		entry = pte_mkclean(entry);
> +		set_pte_at(vma, address, ptep, entry);
> +		lazy_mmu_prot_update(entry);
> +		ret = 1;
> +	}
> 
> -unlock:
> -	pte_unmap_unlock(pte, ptl);
> +	pte_unmap_unlock(ptep, ptl);
>  out:
>  	return ret;
>  }

1) pte_dirty() is always false. The reason is that s390 keeps the dirty
bit information in the storage key and not the pte. If pte_write is
false as well nothing is done. There really should be a 
	if (page_test_and_clear_dirty(page))
		ret = 1;
at the end of page_mkclean.

2) Please use ptep_clear_flush instead of ptep_get_and_clear +
flush_tlb_page. The former uses an optimization on s390 that flushes
just one TLB, the later flushes every TLB of the current mm.

My try to fix this up is attached. It moves the flush_cache_page after
the flush_tlb_page (see asm-generic/pgtable.h for the generic definition
of ptep_clear_flush that is used for i386). I hope this doesn't break
anything else.

-- 
blue skies,
  Martin.

Martin Schwidefsky
Linux for zSeries Development & Services
IBM Deutschland Entwicklung GmbH

"Reality continues to ruin my life." - Calvin.

---
 mm/rmap.c |    6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff -urpN linux-2.6/mm/rmap.c linux-2.6-mkclean/mm/rmap.c
--- linux-2.6/mm/rmap.c	2006-12-20 15:49:01.000000000 +0100
+++ linux-2.6-mkclean/mm/rmap.c	2006-12-20 15:51:14.000000000 +0100
@@ -445,10 +445,8 @@ static int page_mkclean_one(struct page 
 		goto out;
 
 	while (pte_dirty(*ptep) || pte_write(*ptep)) {
-		pte_t entry = ptep_get_and_clear(mm, address, ptep);
+		pte_t entry = ptep_clear_flush(vma, address, ptep);
 		flush_cache_page(vma, address, pte_pfn(entry));
-		flush_tlb_page(vma, address);
-		(void)page_test_and_clear_dirty(page); /* do the s390 thing */
 		entry = pte_wrprotect(entry);
 		entry = pte_mkclean(entry);
 		set_pte_at(vma, address, ptep, entry);
@@ -490,6 +488,8 @@ int page_mkclean(struct page *page)
 		if (mapping)
 			ret = page_mkclean_file(mapping, page);
 	}
+	if (page_test_and_clear_dirty(page))
+		ret = 1;
 
 	return ret;
 }


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