[PATCH] Fix virtual address handling in hugetlb fault

[PATCH] Fix virtual address handling in hugetlb fault

[KAMEZAWA Hiroyuki]

Re: [PATCH] Fix virtual address handling in hugetlb fault

[Andrew Morton]

On Mon, 21 Nov 2011 19:48:32 +0900
KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> wrote:

> >From 7c29389be2890c6b6934a80b4841d07a7014fe26 Mon Sep 17 00:00:00 2001
> From: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
> Date: Mon, 21 Nov 2011 19:45:27 +0900
> Subject: [PATCH] Fix virtual address handling in hugetlb fault
> 
> handle_mm_fault() passes 'faulted' address to hugetlb_fault().
> Then, the address is not aligned to hugepage boundary.
> 
> Most of functions for hugetlb pages are aware of that and
> calculate an alignment by itself. Some functions as copy_user_huge_page(),
> and clear_huge_page() doesn't handle alignment by themselves.
> 
> This patch make hugeltb_fault() to calculate the alignment and pass
> aligned addresss (top address of a faulted hugepage) to functions.
> 

Does this actually fix any known user-visible misbehaviour?

It sounds like the code is masking addresses in a lot of different
places.  It would be better to do it once, at the top level.  Perhaps
this patch makes some of the existing masking obsolete?

> index bb28a5f..af37337 100644
> --- a/mm/hugetlb.c
> +++ b/mm/hugetlb.c
> @@ -2629,6 +2629,8 @@ int hugetlb_fault(struct mm_struct *mm, struct vm_area_struct *vma,
>  	static DEFINE_MUTEX(hugetlb_instantiation_mutex);
>  	struct hstate *h = hstate_vma(vma);
>  
> +	address = address & huge_page_mask(h);

--- a/mm/hugetlb.c~mm-hugetlbc-fix-virtual-address-handling-in-hugetlb-fault-fix
+++ a/mm/hugetlb.c
@@ -2639,7 +2639,7 @@ int hugetlb_fault(struct mm_struct *mm, 
 	static DEFINE_MUTEX(hugetlb_instantiation_mutex);
 	struct hstate *h = hstate_vma(vma);
 
-	address = address & huge_page_mask(h);
+	address &= huge_page_mask(h);
 
 	ptep = huge_pte_offset(mm, address);
 	if (ptep) {

is a bit more readable, IMO.

--
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] Fix virtual address handling in hugetlb fault

[KAMEZAWA Hiroyuki]

On Mon, 21 Nov 2011 14:27:20 -0800
Andrew Morton <akpm@linux-foundation.org> wrote:

> On Mon, 21 Nov 2011 19:48:32 +0900
> KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> wrote:
> 
> > >From 7c29389be2890c6b6934a80b4841d07a7014fe26 Mon Sep 17 00:00:00 2001
> > From: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
> > Date: Mon, 21 Nov 2011 19:45:27 +0900
> > Subject: [PATCH] Fix virtual address handling in hugetlb fault
> > 
> > handle_mm_fault() passes 'faulted' address to hugetlb_fault().
> > Then, the address is not aligned to hugepage boundary.
> > 
> > Most of functions for hugetlb pages are aware of that and
> > calculate an alignment by itself. Some functions as copy_user_huge_page(),
> > and clear_huge_page() doesn't handle alignment by themselves.
> > 
> > This patch make hugeltb_fault() to calculate the alignment and pass
> > aligned addresss (top address of a faulted hugepage) to functions.
> > 
> 
> Does this actually fix any known user-visible misbehaviour?
> 

I just found this at reading codes. And I know 'vaddr' is ignored
in most of per-arch implemantation of clear_user_highpage().
It seems, in some arch, vaddr is used for flushing cache. Now,
CONFIG_HUGETLBFS can be set on x86,powerpc,ia64,mips,sh,sparc,tile. (by grep)

it seems mips and sh uses vaddr in clear_user_(high)page.



> It sounds like the code is masking addresses in a lot of different
> places.  It would be better to do it once, at the top level.  Perhaps
> this patch makes some of the existing masking obsolete?
> 

I think so. 
I'd like to check it and post an additional fix if this patch goes.


> > index bb28a5f..af37337 100644
> > --- a/mm/hugetlb.c
> > +++ b/mm/hugetlb.c
> > @@ -2629,6 +2629,8 @@ int hugetlb_fault(struct mm_struct *mm, struct vm_area_struct *vma,
> >  	static DEFINE_MUTEX(hugetlb_instantiation_mutex);
> >  	struct hstate *h = hstate_vma(vma);
> >  
> > +	address = address & huge_page_mask(h);
> 
> --- a/mm/hugetlb.c~mm-hugetlbc-fix-virtual-address-handling-in-hugetlb-fault-fix
> +++ a/mm/hugetlb.c
> @@ -2639,7 +2639,7 @@ int hugetlb_fault(struct mm_struct *mm, 
>  	static DEFINE_MUTEX(hugetlb_instantiation_mutex);
>  	struct hstate *h = hstate_vma(vma);
>  
> -	address = address & huge_page_mask(h);
> +	address &= huge_page_mask(h);
>  
>  	ptep = huge_pte_offset(mm, address);
>  	if (ptep) {
> 
> is a bit more readable, IMO.
> 
Sure.

Thanks,
-Kame

--
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] Fix virtual address handling in hugetlb fault

[Andrew Morton]

On Tue, 22 Nov 2011 09:32:38 +0900
KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> wrote:

> On Mon, 21 Nov 2011 14:27:20 -0800
> Andrew Morton <akpm@linux-foundation.org> wrote:
> 
> > On Mon, 21 Nov 2011 19:48:32 +0900
> > KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> wrote:
> > 
> > > >From 7c29389be2890c6b6934a80b4841d07a7014fe26 Mon Sep 17 00:00:00 2001
> > > From: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
> > > Date: Mon, 21 Nov 2011 19:45:27 +0900
> > > Subject: [PATCH] Fix virtual address handling in hugetlb fault
> > > 
> > > handle_mm_fault() passes 'faulted' address to hugetlb_fault().
> > > Then, the address is not aligned to hugepage boundary.
> > > 
> > > Most of functions for hugetlb pages are aware of that and
> > > calculate an alignment by itself. Some functions as copy_user_huge_page(),
> > > and clear_huge_page() doesn't handle alignment by themselves.
> > > 
> > > This patch make hugeltb_fault() to calculate the alignment and pass
> > > aligned addresss (top address of a faulted hugepage) to functions.
> > > 
> > 
> > Does this actually fix any known user-visible misbehaviour?
> > 
> 
> I just found this at reading codes. And I know 'vaddr' is ignored
> in most of per-arch implemantation of clear_user_highpage().
> It seems, in some arch, vaddr is used for flushing cache. Now,
> CONFIG_HUGETLBFS can be set on x86,powerpc,ia64,mips,sh,sparc,tile. (by grep)
> 
> it seems mips and sh uses vaddr in clear_user_(high)page.

OK.  Those architectures are probably OK with "any address within the
page" anyway.

I'm actually trying to work out which kernel(s) we should merge this
into ;)

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