[PATCH] mm/huge_memory: Clean up typo when copying user highpage

[PATCH] mm/huge_memory: Clean up typo when copying user highpage

[Hillf Danton]

Hi Andrea

When copying user highpage, the PAGE_SHIFT in the third parameter is a typo,
I think, and is replaced with PAGE_SIZE.

When configuring transparent hugepage, it depends on x86 and MMU.
Would you please tippoint why other archs with MMU, say MIPS, are masked out?

Thanks

Signed-off-by: Hillf Danton <dhillf@gmail.com>
---

--- a/mm/huge_memory.c	Sat Aug 13 11:45:14 2011
+++ b/mm/huge_memory.c	Wed Oct 12 22:26:15 2011
@@ -829,7 +829,7 @@ static int do_huge_pmd_wp_page_fallback(

 	for (i = 0; i < HPAGE_PMD_NR; i++) {
 		copy_user_highpage(pages[i], page + i,
-				   haddr + PAGE_SHIFT*i, vma);
+				   haddr + PAGE_SIZE * i, vma);
 		__SetPageUptodate(pages[i]);
 		cond_resched();
 	}

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mm/huge_memory: Clean up typo when copying user highpage

[Johannes Weiner]

On Wed, Oct 12, 2011 at 10:39:36PM +0800, Hillf Danton wrote:
> Hi Andrea
> 
> When copying user highpage, the PAGE_SHIFT in the third parameter is a typo,
> I think, and is replaced with PAGE_SIZE.

This is a pretty nasty data corruption bug, so 'clean up' might be a
bit of an understatement ;-)

Nice catch.

Would you mind extending the changelog to include a problem
description?  Feel free to steal from this:

	The THP copy-on-write handler falls back to regular-sized
	pages for a huge page replacement upon allocation failure or
	if THP has been individually disabled in the target VMA.  The
	loop responsible for copying page-sized chunks accidentally
	uses multiples of PAGE_SHIFT instead of PAGE_SIZE as the byte
	offset into the original huge page, though, and the
	COW-breaking task ends up with a corrupt copy of the data.

> Signed-off-by: Hillf Danton <dhillf@gmail.com>

Acked-by: Johannes Weiner <jweiner@redhat.com>

> --- a/mm/huge_memory.c	Sat Aug 13 11:45:14 2011
> +++ b/mm/huge_memory.c	Wed Oct 12 22:26:15 2011
> @@ -829,7 +829,7 @@ static int do_huge_pmd_wp_page_fallback(
> 
>  	for (i = 0; i < HPAGE_PMD_NR; i++) {
>  		copy_user_highpage(pages[i], page + i,
> -				   haddr + PAGE_SHIFT*i, vma);
> +				   haddr + PAGE_SIZE * i, vma);
>  		__SetPageUptodate(pages[i]);
>  		cond_resched();
>  	}

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mm/huge_memory: Clean up typo when copying user highpage

[Andrea Arcangeli]

On Wed, Oct 12, 2011 at 10:39:36PM +0800, Hillf Danton wrote:
> Hi Andrea
> 
> When copying user highpage, the PAGE_SHIFT in the third parameter is a typo,
> I think, and is replaced with PAGE_SIZE.

That looks correct. I wonder how it was not noticed yet. Because it
can't go out of bound, it didn't risk to crash the kernel and it didn't
not risk to expose random data to the cowing task. So it shouldn't
have security implications as far as I can tell, but the app could
malfunction and crash (userland corruption only).

I grepped for other PAGE_SHIFT and PAGE_SIZE in the same file and
there seem to be no more of these... Pretty hard for this to go
unnoticed too, I guess the cows aren't as frequent enough to be
capable of triggering compaction failures. I added a
/sys/kernel/mm/transparent_hugepage/debug_cow exactly to catch bugs
like this very one, I guess nobody enabled debug_cow = 1 long
enough... If only I would have tested debug_cow = 1 with 0x00 0x01
0x02 0x03 for the whole 2M I should have noticed it...

> When configuring transparent hugepage, it depends on x86 and MMU.
> Would you please tippoint why other archs with MMU, say MIPS, are masked out?

Because nobody implemented it yet? Some archs may not make it in
hardware too, depends if you can mix large and small pages in the same
vma, then yes the arch could make it by adjusting the pmd size right
in the software pmd_t so that it matches an hardware soft-tlb filled
hash.

> Signed-off-by: Hillf Danton <dhillf@gmail.com>
> ---
> 
> --- a/mm/huge_memory.c	Sat Aug 13 11:45:14 2011
> +++ b/mm/huge_memory.c	Wed Oct 12 22:26:15 2011
> @@ -829,7 +829,7 @@ static int do_huge_pmd_wp_page_fallback(
> 
>  	for (i = 0; i < HPAGE_PMD_NR; i++) {
>  		copy_user_highpage(pages[i], page + i,
> -				   haddr + PAGE_SHIFT*i, vma);
> +				   haddr + PAGE_SIZE * i, vma);
>  		__SetPageUptodate(pages[i]);
>  		cond_resched();
>  	}

Reviewed-by: Andrea Arcangeli <aarcange@redhat.com>

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mm/huge_memory: Clean up typo when copying user highpage

[Andrew Morton]

On Wed, 12 Oct 2011 19:51:48 +0200
Andrea Arcangeli <aarcange@redhat.com> wrote:

> On Wed, Oct 12, 2011 at 10:39:36PM +0800, Hillf Danton wrote:
> > Hi Andrea
> > 
> > When copying user highpage, the PAGE_SHIFT in the third parameter is a typo,
> > I think, and is replaced with PAGE_SIZE.
> 
> That looks correct. I wonder how it was not noticed yet. Because it
> can't go out of bound, it didn't risk to crash the kernel and it didn't
> not risk to expose random data to the cowing task. So it shouldn't
> have security implications as far as I can tell, but the app could
> malfunction and crash (userland corruption only).

Which architectures care about the copy_user_page() `vaddr' argument? 
mips, perhaps?  I suspect the intersection between those architectures
and archs-which-implement-hugepages is the empty set.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mm/huge_memory: Clean up typo when copying user highpage

[Andrea Arcangeli]

On Wed, Oct 12, 2011 at 01:42:24PM -0700, Andrew Morton wrote:
> On Wed, 12 Oct 2011 19:51:48 +0200
> Andrea Arcangeli <aarcange@redhat.com> wrote:
> 
> > On Wed, Oct 12, 2011 at 10:39:36PM +0800, Hillf Danton wrote:
> > > Hi Andrea
> > > 
> > > When copying user highpage, the PAGE_SHIFT in the third parameter is a typo,
> > > I think, and is replaced with PAGE_SIZE.
> > 
> > That looks correct. I wonder how it was not noticed yet. Because it
> > can't go out of bound, it didn't risk to crash the kernel and it didn't
> > not risk to expose random data to the cowing task. So it shouldn't
> > have security implications as far as I can tell, but the app could
> > malfunction and crash (userland corruption only).
> 
> Which architectures care about the copy_user_page() `vaddr' argument? 
> mips, perhaps?  I suspect the intersection between those architectures
> and archs-which-implement-hugepages is the empty set.

Yes it's not happening. debug_cow was specifically meant to trap this
very case so there was little chance it could go unnoticed.

Never mind.... still the patch is correct and good idea to apply as cleanup.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>