Skip to contentSkip to conversation

BUG in x86_64 hugepage support

DISCUSSION Nishanth Aravamudan 3 participants 10 messages Mar 15, 2006

Subsystems (2)
Memory management HugeTLB
lore
Share & tools
lore

Nishanth Aravamudan

author
#0 lore raw

Hello,

While doing some testing of libhugetlbfs, I ran into the following BUGs on my x86_64 box when checking mprotect with hugepages (running make func in libhugetlbfs is all it took here) (distro is Ubuntu Dapper, runs 32-bit userspace).

[  633.480724] ----------- [cut here ] --------- [please bite here ] ---------
[  633.480733] Kernel BUG at ...rc6-mm1/arch/x86_64/mm/../../i386/mm/hugetlbpage.c:31
[  633.480736] invalid opcode: 0000 [1] PREEMPT SMP 
[  633.480740] last sysfs file: /block/sdb/sdb1/stat
[  633.480743] CPU 1 
[  633.480745] Modules linked in:
[  633.480750] Pid: 7872, comm: mprotect Not tainted 2.6.16-rc6-mm1 #1
[  633.480753] RIP: 0010:[<ffffffff80188e46>] <ffffffff80188e46>{huge_pte_alloc+230}
[  633.480764] RSP: 0000:ffff81006675dd18  EFLAGS: 00010283
[  633.480767] RAX: 0000000000000001 RBX: ffff8100648ce008 RCX: 0000000000000000
[  633.480772] RDX: ffff81007c7aa560 RSI: 0000000055800000 RDI: ffff8100648e4480
[  633.480776] RBP: ffff81006675dd38 R08: 00000000556c19fc R09: 00000000ffffd178
[  633.480780] R10: ffff81006675c000 R11: 0000000000000246 R12: 0000000000000560
[  633.480784] R13: ffff8100648e4480 R14: ffff8100648e4480 R15: 0000000000000000
[  633.480788] FS:  00002ac688028e10(0000) GS:ffff81007f1b4740(0063) knlGS:00000000556c68e0
[  633.480792] CS:  0010 DS: 002b ES: 002b CR0: 000000008005003b
[  633.480795] CR2: 0000000055800000 CR3: 000000006593d000 CR4: 00000000000006e0
[  633.480800] Process mprotect (pid: 7872, threadinfo ffff81006675c000, task ffff8100402fc750)
[  633.480803] Stack: 0000000055800000 0000000000000000 0000000000000000 0000000055800000 
[  633.480809]        ffff81006675dd88 ffffffff801c5383 ffff81006675de68 ffff8100734e3c60 
[  633.480817]        ffff81006675dda8 ffff8100734e3c60 
[  633.480821] Call Trace: <ffffffff801c5383>{hugetlb_fault+51} <ffffffff801085ea>{__handle_mm_fault+90}
[  633.480834]        <ffffffff8010a727>{ia32_setup_sigcontext+327} <ffffffff80173fdb>{notifier_call_chain+43}
[  633.480845]        <ffffffff80173b19>{do_page_fault+1241} <ffffffff80171275>{_spin_unlock_irq+21}
[  633.480854]        <ffffffff80134cc5>{sys_rt_sigprocmask+229} <ffffffff8016c741>{error_exit+0}
[  633.480868] 
[  633.480869] Code: 0f 0b 68 08 a6 4e 80 c2 1f 00 48 8b 5d e8 4c 8b 65 f0 48 89 
[  633.480881] RIP <ffffffff80188e46>{huge_pte_alloc+230} RSP <ffff81006675dd18>
[  633.480888]  ----------- [cut here ] --------- [please bite here ] ---------
[  633.492589] Kernel BUG at ...rc6-mm1/arch/x86_64/mm/../../i386/mm/hugetlbpage.c:31
[  633.492593] invalid opcode: 0000 [2] PREEMPT SMP 
[  633.492597] last sysfs file: /block/sdb/sdb1/stat
[  633.492600] CPU 1 
[  633.492602] Modules linked in:
[  633.492606] Pid: 7873, comm: mprotect Not tainted 2.6.16-rc6-mm1 #1
[  633.492610] RIP: 0010:[<ffffffff80188e46>] <ffffffff80188e46>{huge_pte_alloc+230}
[  633.492620] RSP: 0000:ffff81006675fd18  EFLAGS: 00010283
[  633.492624] RAX: 0000000000000001 RBX: ffff810066745550 RCX: 0000000000000000
[  633.492628] RDX: ffff81006596bab0 RSI: 00002aaaaac00000 RDI: ffff8100648e4480
[  633.492632] RBP: ffff81006675fd38 R08: 0000000000000000 R09: 0000000000000000
[  633.492635] R10: 0000000000000008 R11: 0000000000000246 R12: 0000000000000ab0
[  633.492639] R13: ffff8100648e4480 R14: ffff8100648e4480 R15: 0000000000000000
[  633.492644] FS:  00002b745df2ce10(0000) GS:ffff81007f1b4740(0000) knlGS:00000000556ac6c0
[  633.492647] CS:  0010 DS: 0000 ES: 0000 CR0: 000000008005003b
[  633.492651] CR2: 00002aaaaac00000 CR3: 000000006670a000 CR4: 00000000000006e0
[  633.492655] Process mprotect (pid: 7873, threadinfo ffff81006675e000, task ffff8100402fd530)
[  633.492658] Stack: 00002aaaaac00000 0000000000000000 0000000000000003 00002aaaaac00000 
[  633.492665]        ffff81006675fd88 ffffffff801c5383 ffff81006675fe68 ffff8100734e3af0 
[  633.492673]        ffff81006675fd78 ffff8100734e3af0 
[  633.492677] Call Trace: <ffffffff801c5383>{hugetlb_fault+51} <ffffffff801085ea>{__handle_mm_fault+90}
[  633.492690]        <ffffffff80173fdb>{notifier_call_chain+43} <ffffffff80173b19>{do_page_fault+1241}
[  633.492701]        <ffffffff80123652>{sys_mprotect+1522} <ffffffff8016c741>{error_exit+0}
[  633.492715] 
[  633.492716] Code: 0f 0b 68 08 a6 4e 80 c2 1f 00 48 8b 5d e8 4c 8b 65 f0 48 89 
[  633.492728] RIP <ffffffff80188e46>{huge_pte_alloc+230} RSP <ffff81006675fd18>

The line in question (arch/i386/mm/hugetlbpage.c:31) in 2.6.16-rc6-mm1 is:

	BUG_ON(pte && !pte_none(*pte) && !pte_huge(*pte));

We are trying to verify that if the pte was succesfully allocated that it is filled in and that it is a hugetlb pte.

After some discussion with Adam Litke, I added some debugging to see what pte_val we were getting:

	huge_pte_alloc failed: pte == 800000003d800027

which indicates our flags = 0x27 or 00100111.

On x86_64, pte_huge is defined to be:

	#define __LARGE_PTE (_PAGE_PSE|_PAGE_PRESENT) // __LARGE_PTE = 10000001
	static inline int pte_huge(pte_t pte)           { return (pte_val(pte) & __LARGE_PTE) == __LARGE_PTE; }

Clearly, pte_huge() is going to return 0, as

	pte_val(pte) & __LARGE_PTE == 0x1 != __LARGE_PTE

in this case.

I believe the issue occurs due to the following code path:

	sys_mprotect() --> hugetlb_change_protection() --> pte_modify()

On x86_64, that last call is:

	#defined _PAGE_CHG_MASK	(PTE_MASK | _PAGE_ACCESSED | _PAGE_DIRTY) // upper bits all 1, lower 11 bits = 00001100000
	unsigned long __supported_pte_mask __read_mostly = ~0UL;
	static inline pte_t pte_modify(pte_t pte, pgprot_t newprot)
	{
		pte_val(pte) &= _PAGE_CHG_MASK;
		pte_val(pte) |= pgprot_val(newprot);
		pte_val(pte) &= __supported_pte_mask;
		return pte;
	}

So, the first &= results in the lower 11 bits of pte_val(pte) being all 0s. By my analysis, this is the problem, pte_modify() on x86_64 is clearing the bits we check to see if a pte is a hugetlb one. To see if this might be an accurate analysis, I modified _PAGE_CHG_MASK as follows:

	-#define _PAGE_CHG_MASK	(PTE_MASK | _PAGE_ACCESSED | _PAGE_DIRTY)
	+#define _PAGE_CHG_MASK	(PTE_MASK | _PAGE_ACCESSED | _PAGE_DIRTY | _PAGE_PSE | _PAGE_PRESENT)

That is, forcing the bits we care about to get set in pte_modify(). This removed the BUG()s I was seeing in our testing.

This obviously isn't a solution, though, but I don't know what is :) I am hoping somebody with a bit more VM (or x86-64) experience can figure out the right fix. I would appreciate any input, or corrections to my analysis.

Thanks,
Nish

Signature · 8 lines
Nishanth Aravamudan <[email protected]>
IBM Linux Technology Center

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
#1 lore raw

Nishanth Aravamudan wrote on Tuesday, March 14, 2006 5:20 PM

Quoted 11 lines

While doing some testing of libhugetlbfs, I ran into the following BUGs on my x86_64 box when checking mprotect with hugepages (running make func in libhugetlbfs is all it took here) (distro is Ubuntu Dapper, runs 32-bit userspace).

So, the first &= results in the lower 11 bits of pte_val(pte) being all 0s. By my analysis, this is the problem, pte_modify() on x86_64 is clearing the bits we check to see if a pte is a hugetlb one. To see if this might be an accurate analysis, I modified _PAGE_CHG_MASK as follows:

Quoted 5 lines
	-#define _PAGE_CHG_MASK	(PTE_MASK | _PAGE_ACCESSED | _PAGE_DIRTY)
	+#define _PAGE_CHG_MASK	(PTE_MASK | _PAGE_ACCESSED | _PAGE_DIRTY | _PAGE_PSE | _PAGE_PRESENT)

That is, forcing the bits we care about to get set in pte_modify(). This removed the BUG()s I was seeing in our testing.

I think your analysis looked correct. Though I don't think you want to add _PAGE_PRESENT to _PAGE_CHG_MASK. The reason being newprot suppose to have correct present bit (based on what the new protection is) and it will be or'ed to form new pte.

I think _PAGE_PSE bit should be in _PAGE_CHG_MASK.

- Ken

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

author
#2 lore raw
Quoted 15 lines · Kenneth W

Nishanth Aravamudan wrote on Tuesday, March 14, 2006 5:20 PM

Quoted 14 lines

While doing some testing of libhugetlbfs, I ran into the following BUGs on my x86_64 box when checking mprotect with hugepages (running make func in libhugetlbfs is all it took here) (distro is Ubuntu Dapper, runs 32-bit userspace).

So, the first &= results in the lower 11 bits of pte_val(pte) being all 0s. By my analysis, this is the problem, pte_modify() on x86_64 is clearing the bits we check to see if a pte is a hugetlb one. To see if this might be an accurate analysis, I modified _PAGE_CHG_MASK as follows:

	-#define _PAGE_CHG_MASK	(PTE_MASK | _PAGE_ACCESSED | _PAGE_DIRTY)
	+#define _PAGE_CHG_MASK	(PTE_MASK | _PAGE_ACCESSED | _PAGE_DIRTY | _PAGE_PSE | _PAGE_PRESENT)
Quoted 7 lines
Quoted 2 lines

That is, forcing the bits we care about to get set in pte_modify(). This removed the BUG()s I was seeing in our testing.

I think your analysis looked correct. Though I don't think you want to add _PAGE_PRESENT to _PAGE_CHG_MASK. The reason being newprot suppose to have correct present bit (based on what the new protection is) and it will be or'ed to form new pte.

Thanks for the response!

Quoted 1 line

I think _PAGE_PSE bit should be in _PAGE_CHG_MASK.

I can try a kernel with just the _PAGE_PSE bit added to _PAGE_CHG_MASK and see if that helps. I think it will still BUG() in my case, however, as __LARGE_PTE is 10000001, so only setting the 8th bit will be insufficient. So maybe there is also something wrong with what is being generated by pgprot_val(newprot)? I will try adding some more debugging output to see what is happening in pte_modify.

Thanks,
Nish

Signature · 8 lines
Nishanth Aravamudan <[email protected]>
IBM Linux Technology Center

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
#3 lore raw

Nishanth Aravamudan wrote on Tuesday, March 14, 2006 8:36 PM

Quoted 8 lines
Quoted 1 line

I think _PAGE_PSE bit should be in _PAGE_CHG_MASK.

I can try a kernel with just the _PAGE_PSE bit added to _PAGE_CHG_MASK and see if that helps. I think it will still BUG() in my case, however, as __LARGE_PTE is 10000001, so only setting the 8th bit will be insufficient. So maybe there is also something wrong with what is being generated by pgprot_val(newprot)? I will try adding some more debugging output to see what is happening in pte_modify.

Forget about preserving _PAGE_PSE BIT, It won't work. I just realized that _PAGE_PROTNONE clashes with _PAGE_PSE (both use bit 7). Still thinking ...

- Ken

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

author
#4 lore raw
Quoted 10 lines · Kenneth W

Nishanth Aravamudan wrote on Tuesday, March 14, 2006 8:36 PM

Quoted 8 lines
Quoted 1 line

I think _PAGE_PSE bit should be in _PAGE_CHG_MASK.

I can try a kernel with just the _PAGE_PSE bit added to _PAGE_CHG_MASK and see if that helps. I think it will still BUG() in my case, however, as __LARGE_PTE is 10000001, so only setting the 8th bit will be insufficient. So maybe there is also something wrong with what is being generated by pgprot_val(newprot)? I will try adding some more debugging output to see what is happening in pte_modify.

Quoted 2 lines

Forget about preserving _PAGE_PSE BIT, It won't work. I just realized that _PAGE_PROTNONE clashes with _PAGE_PSE (both use bit 7). Still thinking ...

And here I was rather proud of the following patch :)

I figured, if we know that we're dealing with a hugepage pte, then we probably can set _PAGE_PSE in newprot, no? This fixes the BUGs here, but may have side affects I'm unaware of (and I was unable to test on an i386 kernel, since all of a sudden -mm decided to stop building with Ubuntu's biarch gcc -- another topic altogether).

Here's the patch I used.

Description: We currently fail mprotect testing in libhugetlbfs because the PSE bit in the hugepage PTEs gets unset. In the case where we know that a filled hugetlb PTE is going to have its protection changed, make sure it stays a hugetlb PTE by setting the PSE bit in the new protection flags.

Signed-off-by Nishanth Aravamudan <[email protected]>
mm/hugetlb.c +1 -0
diff -urpN 2.6.16-rc6-mm1/mm/hugetlb.c 2.6.16-rc6-mm1-dev/mm/hugetlb.c
@@ -740,6 +740,7 @@ void hugetlb_change_protection(struct vm
 			continue;
 		if (!pte_none(*ptep)) {
 			pte = huge_ptep_get_and_clear(mm, address, ptep);
+			pgprot_val(newprot) |= _PAGE_PSE;
 			pte = pte_modify(pte, newprot);
 			set_huge_pte_at(mm, address, ptep, pte);
 			lazy_mmu_prot_update(pte);
Signature · 8 lines
Nishanth Aravamudan <[email protected]>
IBM Linux Technology Center

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
#5 lore raw

mm/hugetlb.c void hugetlb_change_protection(struct vm · line 746

Quoted 11 lines

diff -urpN 2.6.16-rc6-mm1/mm/hugetlb.c 2.6.16-rc6-mm1-dev/mm/hugetlb.c

mm/hugetlb.c +1 -0
@@ -740,6 +740,7 @@ void hugetlb_change_protection(struct vm
 			continue;
 		if (!pte_none(*ptep)) {
 			pte = huge_ptep_get_and_clear(mm, address, ptep);
+			pgprot_val(newprot) |= _PAGE_PSE;
 			pte = pte_modify(pte, newprot);
 			set_huge_pte_at(mm, address, ptep, pte);
 			lazy_mmu_prot_update(pte);

This is architecture independent code - you shouldn't be using _PAGE_PSE here. Probably x86-64 (and then likely also i386) should define their own set_huge_pte_at(), and use that# to or in the needed flag?

Jan

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

#6 lore raw

Nishanth Aravamudan wrote on Tuesday, March 14, 2006 11:31 PM

Quoted 5 lines

Description: We currently fail mprotect testing in libhugetlbfs because the PSE bit in the hugepage PTEs gets unset. In the case where we know that a filled hugetlb PTE is going to have its protection changed, make sure it stays a hugetlb PTE by setting the PSE bit in the new protection flags.

Jan Beulich wrote on Wednesday, March 15, 2006 12:50 AM

Quoted 4 lines

This is architecture independent code - you shouldn't be using _PAGE_PSE here. Probably x86-64 (and then likely also i386) should define their own set_huge_pte_at(), and use that# to or in the needed flag?

Yeah, that will do. i386, x86_64 should also clean up pte_mkhuge() macro. The unconditional setting of _PAGE_PRESENT bit was a leftover stuff from the good'old day of pre-faulting hugetlb page.

[patch] fix i386/x86-64 _PAGE_PSE bit when changing page protection

On i386 and x86-64, pte flag _PAGE_PSE collides with _PAGE_PROTNONE. The identify of hugetlb pte is lost when changing page protection via mprotect. A page fault occurs later will trigger a bug check in huge_pte_alloc().

The fix is to always make new pte a hugetlb pte and also to clean up legacy code where _PAGE_PRESENT is forced on in the pre-faulting day.

Signed-off-by Ken Chen <[email protected]>
include/asm-i386/pgtable.h +2 -3
diff -Nurp linux-2.6.15/include/asm-i386/pgtable.h linux-2.6.15-mm/include/asm-i386/pgtable.h
@@ -219,13 +219,12 @@ extern unsigned long pg0[];
  * The following only work if pte_present() is true.
  * Undefined behaviour if not..
  */
-#define __LARGE_PTE (_PAGE_PSE | _PAGE_PRESENT)
 static inline int pte_user(pte_t pte)		{ return (pte).pte_low & _PAGE_USER; }
 static inline int pte_read(pte_t pte)		{ return (pte).pte_low & _PAGE_USER; }
 static inline int pte_dirty(pte_t pte)		{ return (pte).pte_low & _PAGE_DIRTY; }
 static inline int pte_young(pte_t pte)		{ return (pte).pte_low & _PAGE_ACCESSED; }
 static inline int pte_write(pte_t pte)		{ return (pte).pte_low & _PAGE_RW; }
-static inline int pte_huge(pte_t pte)		{ return ((pte).pte_low & __LARGE_PTE) == __LARGE_PTE; }
+static inline int pte_huge(pte_t pte)		{ return (pte).pte_low & _PAGE_PSE; }
 
 /*
  * The following only works if pte_present() is not true.

@@ -242,7 +241,7 @@ static inline pte_t pte_mkexec(pte_t pte
 static inline pte_t pte_mkdirty(pte_t pte)	{ (pte).pte_low |= _PAGE_DIRTY; return pte; }
 static inline pte_t pte_mkyoung(pte_t pte)	{ (pte).pte_low |= _PAGE_ACCESSED; return pte; }
 static inline pte_t pte_mkwrite(pte_t pte)	{ (pte).pte_low |= _PAGE_RW; return pte; }
-static inline pte_t pte_mkhuge(pte_t pte)	{ (pte).pte_low |= __LARGE_PTE; return pte; }
+static inline pte_t pte_mkhuge(pte_t pte)	{ (pte).pte_low |= _PAGE_PSE; return pte; }
 
 #ifdef CONFIG_X86_PAE
 # include <asm/pgtable-3level.h>
include/asm-ia64/pgtable.h +1 -1
diff -Nurp linux-2.6.15/include/asm-ia64/pgtable.h linux-2.6.15-mm/include/asm-ia64/pgtable.h
@@ -314,7 +314,7 @@ ia64_phys_addr_valid (unsigned long addr
 #define pte_mkyoung(pte)	(__pte(pte_val(pte) | _PAGE_A))
 #define pte_mkclean(pte)	(__pte(pte_val(pte) & ~_PAGE_D))
 #define pte_mkdirty(pte)	(__pte(pte_val(pte) | _PAGE_D))
-#define pte_mkhuge(pte)		(__pte(pte_val(pte) | _PAGE_P))
+#define pte_mkhuge(pte)		(__pte(pte_val(pte)))
 
 /*
  * Macro to a page protection value as "uncacheable".  Note that "protection" is really a
include/asm-x86_64/pgtable.h +2 -2
diff -Nurp linux-2.6.15/include/asm-x86_64/pgtable.h linux-2.6.15-mm/include/asm-x86_64/pgtable.h
@@ -273,7 +272,7 @@ static inline int pte_dirty(pte_t pte)		
 static inline int pte_young(pte_t pte)		{ return pte_val(pte) & _PAGE_ACCESSED; }
 static inline int pte_write(pte_t pte)		{ return pte_val(pte) & _PAGE_RW; }
 static inline int pte_file(pte_t pte)		{ return pte_val(pte) & _PAGE_FILE; }
-static inline int pte_huge(pte_t pte)		{ return (pte_val(pte) & __LARGE_PTE) == __LARGE_PTE; }
+static inline int pte_huge(pte_t pte)		{ return pte_val(pte) & _PAGE_PSE; }
 
 static inline pte_t pte_rdprotect(pte_t pte)	{ set_pte(&pte, __pte(pte_val(pte) & ~_PAGE_USER)); return pte; }
 static inline pte_t pte_exprotect(pte_t pte)	{ set_pte(&pte, __pte(pte_val(pte) & ~_PAGE_USER)); return pte; }

@@ -285,7 +284,7 @@ static inline pte_t pte_mkexec(pte_t pte
 static inline pte_t pte_mkdirty(pte_t pte)	{ set_pte(&pte, __pte(pte_val(pte) | _PAGE_DIRTY)); return pte; }
 static inline pte_t pte_mkyoung(pte_t pte)	{ set_pte(&pte, __pte(pte_val(pte) | _PAGE_ACCESSED)); return pte; }
 static inline pte_t pte_mkwrite(pte_t pte)	{ set_pte(&pte, __pte(pte_val(pte) | _PAGE_RW)); return pte; }
-static inline pte_t pte_mkhuge(pte_t pte)	{ set_pte(&pte, __pte(pte_val(pte) | __LARGE_PTE)); return pte; }
+static inline pte_t pte_mkhuge(pte_t pte)	{ set_pte(&pte, __pte(pte_val(pte) | _PAGE_PSE)); return pte; }
 
 struct vm_area_struct;
mm/hugetlb.c +1 -1
diff -Nurp linux-2.6.15/mm/hugetlb.c linux-2.6.15-mm/mm/hugetlb.c
@@ -731,7 +731,7 @@ void hugetlb_change_protection(struct vm
 			continue;
 		if (!pte_none(*ptep)) {
 			pte = huge_ptep_get_and_clear(mm, address, ptep);
-			pte = pte_modify(pte, newprot);
+			pte = pte_mkhuge(pte_modify(pte, newprot));
 			set_huge_pte_at(mm, address, ptep, pte);
 			lazy_mmu_prot_update(pte);
 		}

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

author
#7 lore raw
Quoted 14 lines · Kenneth W

Nishanth Aravamudan wrote on Tuesday, March 14, 2006 11:31 PM

Quoted 5 lines

Description: We currently fail mprotect testing in libhugetlbfs because the PSE bit in the hugepage PTEs gets unset. In the case where we know that a filled hugetlb PTE is going to have its protection changed, make sure it stays a hugetlb PTE by setting the PSE bit in the new protection flags.

Jan Beulich wrote on Wednesday, March 15, 2006 12:50 AM

Quoted 4 lines

This is architecture independent code - you shouldn't be using _PAGE_PSE here. Probably x86-64 (and then likely also i386) should define their own set_huge_pte_at(), and use that# to or in the needed flag?

Quoted 3 lines

Yeah, that will do. i386, x86_64 should also clean up pte_mkhuge() macro. The unconditional setting of _PAGE_PRESENT bit was a leftover stuff from the good'old day of pre-faulting hugetlb page.

Patch looks correct, I'll reboot with it applied and make sure it fixes the BUGs (and doesn't affect any of the other tests).

Thanks,
Nish

Signature · 8 lines
Nishanth Aravamudan <[email protected]>
IBM Linux Technology Center

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
author
#8 lore raw
In reply to Chen, Kenneth W
Quoted 27 lines · Kenneth W

Nishanth Aravamudan wrote on Tuesday, March 14, 2006 11:31 PM

Quoted 5 lines

Description: We currently fail mprotect testing in libhugetlbfs because the PSE bit in the hugepage PTEs gets unset. In the case where we know that a filled hugetlb PTE is going to have its protection changed, make sure it stays a hugetlb PTE by setting the PSE bit in the new protection flags.

Jan Beulich wrote on Wednesday, March 15, 2006 12:50 AM

Quoted 4 lines

This is architecture independent code - you shouldn't be using _PAGE_PSE here. Probably x86-64 (and then likely also i386) should define their own set_huge_pte_at(), and use that# to or in the needed flag?

Yeah, that will do. i386, x86_64 should also clean up pte_mkhuge() macro. The unconditional setting of _PAGE_PRESENT bit was a leftover stuff from the good'old day of pre-faulting hugetlb page.

[patch] fix i386/x86-64 _PAGE_PSE bit when changing page protection

On i386 and x86-64, pte flag _PAGE_PSE collides with _PAGE_PROTNONE. The identify of hugetlb pte is lost when changing page protection via mprotect. A page fault occurs later will trigger a bug check in huge_pte_alloc().

Quoted 5 lines

The fix is to always make new pte a hugetlb pte and also to clean up legacy code where _PAGE_PRESENT is forced on in the pre-faulting day.

Signed-off-by Ken Chen <[email protected]>

I can confirm this fixes the BUGs I was seeing on x86_64 testing of libhugetlbfs' mprotect support.

Signed-off-by Nishanth Aravamudan <[email protected]>

Thanks,
Nish

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

author
#9 lore raw
In reply to Jan Beulich
Quoted 8 lines · Jan Beulich
Quoted 8 lines

diff -urpN 2.6.16-rc6-mm1/mm/hugetlb.c 2.6.16-rc6-mm1-dev/mm/hugetlb.c

mm/hugetlb.c +1 -0
@@ -740,6 +740,7 @@ void hugetlb_change_protection(struct vm
 			continue;
 		if (!pte_none(*ptep)) {
 			pte = huge_ptep_get_and_clear(mm, address, ptep);
+			pgprot_val(newprot) |= _PAGE_PSE;
Quoted 8 lines

mm/hugetlb.c void hugetlb_change_protection(struct vm · line 746

Quoted 3 lines
mm/hugetlb.c +0 -0
 			pte = pte_modify(pte, newprot);
 			set_huge_pte_at(mm, address, ptep, pte);
 			lazy_mmu_prot_update(pte);

This is architecture independent code - you shouldn't be using _PAGE_PSE here. Probably x86-64 (and then likely also i386) should define their own set_huge_pte_at(), and use that# to or in the needed flag?

Good point, makes sense to me.

Thanks,
Nish

Signature · 8 lines
Nishanth Aravamudan <[email protected]>
IBM Linux Technology Center

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>