x86_64 expertise needed re: BUG() at pageattr:107

x86_64 expertise needed re: BUG() at pageattr:107

[John W. Linville]

The x86_64 version of pageattr.c contains the following definition
of revert_page():

static void revert_page(unsigned long address, pgprot_t ref_prot)
{
       pgd_t *pgd;
       pmd_t *pmd;
       pte_t large_pte;

       pgd = pgd_offset_k(address);
       pmd = pmd_offset(pgd, address);
       BUG_ON(pmd_val(*pmd) & _PAGE_PSE);
       pgprot_val(ref_prot) |= _PAGE_PSE;
       large_pte = mk_pte_phys(__pa(address) & LARGE_PAGE_MASK, ref_prot);
       set_pte((pte_t *)pmd, large_pte);
}

Please notice the BUG_ON() (originally at line pageattr.c:107).
I notice that the x86 version of revert_page() has no similar check.

Would someone please explain:

	a) why is this a bug?;
	b) what is likely to have triggered it?; and,
	c) why is x86_64 different from x86?

I've Googled a few references to this problem (latest 10/10/2004),
but I've seen no solutions.  I do not currently have a reproducible
test case, although I have a report (which dates from around 2.6.7)
of seeing this problem when installing the e1000 driver module.

I'd be happy to see some educated guesses... :-)

Thanks for any input!

John
-- 
John W. Linville
linville@tuxdriver.com

Re: x86_64 expertise needed re: BUG() at pageattr:107

[Jason Baron]

On Mon, 1 Nov 2004, John W. Linville wrote:

> The x86_64 version of pageattr.c contains the following definition
> of revert_page():
> 
> static void revert_page(unsigned long address, pgprot_t ref_prot)
> {
>        pgd_t *pgd;
>        pmd_t *pmd;
>        pte_t large_pte;
> 
>        pgd = pgd_offset_k(address);
>        pmd = pmd_offset(pgd, address);
>        BUG_ON(pmd_val(*pmd) & _PAGE_PSE);
>        pgprot_val(ref_prot) |= _PAGE_PSE;
>        large_pte = mk_pte_phys(__pa(address) & LARGE_PAGE_MASK, ref_prot);
>        set_pte((pte_t *)pmd, large_pte);
> }
> 
> Please notice the BUG_ON() (originally at line pageattr.c:107).
> I notice that the x86 version of revert_page() has no similar check.
> 
> Would someone please explain:
> 
> 	a) why is this a bug?;

In this code path, a pmd is 'reverted' to point to a 2MB/4MB span of 
physical memory. This memory had previously been split from a 2MB/4MB 
span into 4KB segments, adding a pte table in the process, by calling 
change_page_attr. Thus, when it is reverted back it should not be pointing 
to the 2MB/4MB sapn. 


> 	b) what is likely to have triggered it?; and,

not sure, but i think when the page splits the count should be increased 
in the pte page, not the pmd page. as follows:

--- linux/arch/i386/mm/pageattr.c.orig	Tue Nov  2 13:48:39 2004
+++ linux/arch/i386/mm/pageattr.c	Tue Nov  2 13:49:02 2004
@@ -126,7 +126,7 @@ __change_page_attr(struct page *page, pg
 			struct page *split = split_large_page(address, prot); 
 			if (!split)
 				return -ENOMEM;
-			get_page(kpte_page);
+			get_page(split);
 			set_pmd_pte(kpte,address,mk_pte(split, PAGE_KERNEL));
 		}	
 	} else if ((pte_val(*kpte) & _PAGE_PSE) == 0) { 
--- 2.6-10-28-04/arch/x86_64/mm/pageattr.c.orig	Tue Nov  2 13:41:34 2004
+++ 2.6-10-28-04/arch/x86_64/mm/pageattr.c	Tue Nov  2 13:48:28 2004
@@ -134,7 +134,7 @@ __change_page_attr(unsigned long address
 			struct page *split = split_large_page(address, prot, ref_prot); 
 			if (!split)
 				return -ENOMEM;
-			get_page(kpte_page);
+			get_page(split);
 			set_pte(kpte,mk_pte(split, ref_prot));
 		}	
 	} else if ((kpte_flags & _PAGE_PSE) == 0) { 


> 	c) why is x86_64 different from x86?

This should be a BUG condition on x86 as well.

-Jason