linux-kernel - Re: [PATCH 1/6] mm/hwpoison: fix lose PG_dirty flag for errors on mlocked pages

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:	Thu, 22 Aug 2013 11:51:32 -0400
From:	Naoya Horiguchi <n-horiguchi@...jp.nec.com>
To:	Wanpeng Li <liwanp@...ux.vnet.ibm.com>
Cc:	Andrew Morton <akpm@...ux-foundation.org>,
	Andi Kleen <andi@...stfloor.org>,
	Fengguang Wu <fengguang.wu@...el.com>,
	Tony Luck <tony.luck@...el.com>, gong.chen@...ux.intel.com,
	linux-mm@...ck.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/6] mm/hwpoison: fix lose PG_dirty flag for errors on
 mlocked pages

Hi Wanpeng,

On Thu, Aug 22, 2013 at 05:48:22PM +0800, Wanpeng Li wrote:
> memory_failure() store the page flag of the error page before doing unmap, 
> and (only) if the first check with page flags at the time decided the error 
> page is unknown, it do the second check with the stored page flag since 
> memory_failure() does unmapping of the error pages before doing page_action(). 
> This unmapping changes the page state, especially page_remove_rmap() (called 
> from try_to_unmap_one()) clears PG_mlocked, so page_action() can't catch 
> mlocked pages after that. 
> 
> However, memory_failure() can't handle memory errors on dirty mlocked pages 
> correctly. try_to_unmap_one will move the dirty bit from pte to the physical 
> page, the second check lose it since it check the stored page flag. This patch 
> fix it by restore PG_dirty flag to stored page flag if the page is dirty.

Right. And I'm guessing that the discrepancy between pte_dirty and PageDirty
can happen on the situations rather than mlocked pages.
Anyway, using both of page flags before and after unmapping looks right to me.

Reviewed-by: Naoya Horiguchi <n-horiguchi@...jp.nec.com>


> Testcase:
> 
> #define _GNU_SOURCE
> #include <stdlib.h>
> #include <stdio.h>
> #include <sys/mman.h>
> #include <sys/types.h>
> #include <errno.h>
> 
> #define PAGES_TO_TEST 2
> #define PAGE_SIZE	4096
> 
> int main(void)
> {
> 	char *mem;
> 	int i;
> 
> 	mem = mmap(NULL, PAGES_TO_TEST * PAGE_SIZE,
> 			PROT_READ | PROT_WRITE, MAP_PRIVATE | MAP_ANONYMOUS | MAP_LOCKED, 0, 0);
> 
> 	for (i = 0; i < PAGES_TO_TEST; i++)
> 		mem[i * PAGE_SIZE] = 'a';
> 
> 	if (madvise(mem, PAGES_TO_TEST * PAGE_SIZE, MADV_HWPOISON) == -1)
> 		return -1;
> 
> 	return 0;
> }
> 
> Before patch:
> 
> [  912.839247] Injecting memory failure for page 7dfb8 at 7f6b4e37b000
> [  912.839257] MCE 0x7dfb8: clean mlocked LRU page recovery: Recovered
> [  912.845550] MCE 0x7dfb8: clean mlocked LRU page still referenced by 1 users
> [  912.852586] Injecting memory failure for page 7e6aa at 7f6b4e37c000
> [  912.852594] MCE 0x7e6aa: clean mlocked LRU page recovery: Recovered
> [  912.858936] MCE 0x7e6aa: clean mlocked LRU page still referenced by 1 users
> 
> After patch:
> 
> [  163.590225] Injecting memory failure for page 91bc2f at 7f9f5b0e5000
> [  163.590264] MCE 0x91bc2f: dirty mlocked LRU page recovery: Recovered
> [  163.596680] MCE 0x91bc2f: dirty mlocked LRU page still referenced by 1 users
> [  163.603831] Injecting memory failure for page 91cdd3 at 7f9f5b0e6000
> [  163.603852] MCE 0x91cdd3: dirty mlocked LRU page recovery: Recovered
> [  163.610305] MCE 0x91cdd3: dirty mlocked LRU page still referenced by 1 users
> 
> Signed-off-by: Wanpeng Li <liwanp@...ux.vnet.ibm.com>
> ---
>  mm/memory-failure.c |    3 +++
>  1 files changed, 3 insertions(+), 0 deletions(-)
> 
> diff --git a/mm/memory-failure.c b/mm/memory-failure.c
> index bee58d8..e156084 100644
> --- a/mm/memory-failure.c
> +++ b/mm/memory-failure.c
> @@ -1206,6 +1206,9 @@ int memory_failure(unsigned long pfn, int trapno, int flags)
>  	for (ps = error_states;; ps++)
>  		if ((p->flags & ps->mask) == ps->res)
>  			break;
> +
> +	page_flags |= (p->flags & (1UL << PG_dirty));
> +
>  	if (!ps->mask)
>  		for (ps = error_states;; ps++)
>  			if ((page_flags & ps->mask) == ps->res)
> -- 
> 1.7.7.6
>
--
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/