remap_file_pages - Bug with _PAGE_PROTNONE - is it used in current kernels?

DISCUSSION Blaisorblade 2 participants 3 messages Feb 20, 2006 – Feb 28, 2006 Memory management

Share & tools

Blaisorblade

author

2006-02-20 22:54 UTC #0 lore raw

I've been hitting a bug on a patch I'm working on and have considered (and more or less tested with good results) doing this change:

-#define pte_present(x) ((x).pte_low & (_PAGE_PRESENT | _PAGE_PROTNONE)) +#define pte_present(x) ((x).pte_low & (_PAGE_PRESENT))

(and the corresponding thing on other architecture).

In general, the question is whether __P000 and __S000 in protection_map are ever used except for MAP_POPULATE, and even then if they work well.

I'm seeking for objections to this change and/or anything I'm missing.

This bug showed up while porting remap_file_pages protection support to 2.6.16-rc3. It always existed but couldn't trigger before the PageReserved changes.

Consider a _PAGE_PROTNONE pte, which has then pte_pfn(pte) == 0 (with remap_file_pages you need them to exist). Obviously pte_pfn(pte) on such a PTE doesn't make sense, but since pte_present(pte) gives true the code doesn't know that.

Consider a call to munmap on this range. We get to zap_pte_range() which (in condensed source code):

zap_pte_range()
...

                if (pte_present(ptent)) {
//This test is passed
                        struct page *page = vm_normal_page(vma, addr, ptent);
//Now page points to page 0 - which is wrong, page should be NULL
                        page_remove_rmap(page);

//Which doesn't make any sense.
//If mem_map[0] wasn't mapped we hit a BUG now, if it was we'll hit it later - //i.e. negative page_mapcount().

Now, since this code doesn't work in this situation, I wonder whether PROTNONE is indeed used anywhere in the code *at the moment*, since faults on pages mapped as such are handled with SIGSEGV.

The only possible application, which is only possible in 2.6 and not in 2.4 where _PAGE_PROTNONE still exists, is mmap(MAP_POPULATE) with prot == PROT_NONE.

Instead I need to make use of PROTNONE, so the handling of it may need changes. In particular, I wonder about why:

#define pte_present(x) ((x).pte_low & (_PAGE_PRESENT | _PAGE_PROTNONE))

I see why that _PAGE_PROTNONE can make sense, but in the above code it doesn't.

Signature · 18 lines

Inform me of my mistakes, so I can keep imitating Homer Simpson's "Doh!".
Paolo Giarrusso, aka Blaisorblade (Skype ID "PaoloGiarrusso", ICQ 215621894)
http://www.user-mode-linux.org/~blaisorblade


	

	
		
___________________________________ 
Yahoo! Mail: gratis 1GB per i messaggi e allegati da 10MB 
http://mail.yahoo.it

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

Hugh Dickins

2006-02-21 13:08 UTC #1 lore raw

Quoted 6 lines · Blaisorblade

I've been hitting a bug on a patch I'm working on and have considered (and more or less tested with good results) doing this change:
(quoted diff) +1 -1
-#define pte_present(x)  ((x).pte_low & (_PAGE_PRESENT | _PAGE_PROTNONE))
+#define pte_present(x)  ((x).pte_low & (_PAGE_PRESENT))

Quoted 6 lines

(and the corresponding thing on other architecture).
In general, the question is whether __P000 and __S000 in protection_map are ever used except for MAP_POPULATE, and even then if they work well.
I'm seeking for objections to this change and/or anything I'm missing.

Objection, your honor.

Quoted 8 lines

This bug showed up while porting remap_file_pages protection support to 2.6.16-rc3. It always existed but couldn't trigger before the PageReserved changes.
Consider a _PAGE_PROTNONE pte, which has then pte_pfn(pte) == 0 (with remap_file_pages you need them to exist). Obviously pte_pfn(pte) on such a PTE doesn't make sense, but since pte_present(pte) gives true the code doesn't know that.

I didn't fully understand you there, but I think you've got it the wrong way round: _PAGE_PROTNONE is included in the pte_present() test precisely because there is a valid page there, pfn is set (it might be pfn 0, yes, but much more likely to be pfn non-0).

I've never used PROT_NONE myself (beyond testing), but I think the traditional way it's used is this: mmap(,,PROT_READ|PROT_WRITE,,,), initialize the pages of that mapping, then mprotect(,,PROT_NONE) - which retains all those pages but make them generate SIGSEGVs - so the app can detect accesses and decide if it wants to do something special with them, other than the obvious mprotect(,,PROT_READ) or whatever.

PROT_NONE gives you a way of holding the page present (unlike munmap), yet failing access. And since those pages remain present, they do need to be freed later when you get to zap_pte_range. They are normal pages, but user access to them has been restricted.

Hugh

Quoted 29 lines

Consider a call to munmap on this range. We get to zap_pte_range() which (in condensed source code):
zap_pte_range()
...
                if (pte_present(ptent)) {
//This test is passed
                        struct page *page = vm_normal_page(vma, addr, ptent);
//Now page points to page 0 - which is wrong, page should be NULL
                        page_remove_rmap(page);
//Which doesn't make any sense.
//If mem_map[0] wasn't mapped we hit a BUG now, if it was we'll hit it later - //i.e. negative page_mapcount().
Now, since this code doesn't work in this situation, I wonder whether PROTNONE is indeed used anywhere in the code *at the moment*, since faults on pages mapped as such are handled with SIGSEGV.
The only possible application, which is only possible in 2.6 and not in 2.4 where _PAGE_PROTNONE still exists, is mmap(MAP_POPULATE) with prot == PROT_NONE.
Instead I need to make use of PROTNONE, so the handling of it may need changes. In particular, I wonder about why:
#define pte_present(x) ((x).pte_low & (_PAGE_PRESENT | _PAGE_PROTNONE))
I see why that _PAGE_PROTNONE can make sense, but in the above code it doesn't.

Blaisorblade

author

2006-02-28 16:53 UTC #2 lore raw

Quoted 7 lines · Hugh Dickins

Quoted 6 lines · Blaisorblade
I've been hitting a bug on a patch I'm working on and have considered (and more or less tested with good results) doing this change:
(quoted diff) +1 -1
-#define pte_present(x)  ((x).pte_low & (_PAGE_PRESENT | _PAGE_PROTNONE))
+#define pte_present(x)  ((x).pte_low & (_PAGE_PRESENT))

Quoted 8 lines

Quoted 6 lines
(and the corresponding thing on other architecture).
In general, the question is whether __P000 and __S000 in protection_map are ever used except for MAP_POPULATE, and even then if they work well.
I'm seeking for objections to this change and/or anything I'm missing.
Objection, your honor.

English humor :-) ?

Quoted 13 lines

I didn't fully understand you there, but I think you've got it the wrong way round: _PAGE_PROTNONE is included in the pte_present() test precisely because there is a valid page there, pfn is set (it might be pfn 0, yes, but much more likely to be pfn non-0).
I've never used PROT_NONE myself (beyond testing), but I think the traditional way it's used is this: mmap(,,PROT_READ|PROT_WRITE,,,), initialize the pages of that mapping, then mprotect(,,PROT_NONE) - which retains all those pages but make them generate SIGSEGVs - so the app can detect accesses and decide if it wants to do something special with them, other than the obvious mprotect(,,PROT_READ) or whatever.

Quoted 4 lines

PROT_NONE gives you a way of holding the page present (unlike munmap), yet failing access. And since those pages remain present, they do need to be freed later when you get to zap_pte_range. They are normal pages, but user access to them has been restricted.

Ok, thanks for the explaination.

The bug is born from the patched install_file_pte(). Before there was no need to store the protection bits, now it's needed.

So, it sets a pte_file PTE containing no page, and on PROT_NONE it uses _PAGE_PROTNONE|_PAGE_FILE.

Indeed, what I've actually coded and tested was safer, but I wanted to know if it could be simpler (and faster). For i386 it should be (I've re-tested only UML so far):

-#define pte_present(x) ((x).pte_low & (_PAGE_PRESENT | _PAGE_PROTNONE)) +#define pte_present(x) (((x).pte_low & _PAGE_PRESENT) || \
+ (((x).pte_low & (_PAGE_PROTNONE|_PAGE_FILE)) == _PAGE_PROTNONE))

--- linux-2.6.git.orig/include/asm-um/pgtable.h
+++ linux-2.6.git/include/asm-um/pgtable.h
@@

-#define pte_present(x) pte_get_bits(x, (_PAGE_PRESENT | _PAGE_PROTNONE)) +#define pte_present(x) (pte_get_bits(x, (_PAGE_PRESENT)) || (pte_get_bits(x, (_PAGE_PROTNONE)) && !pte_file(x)))

Signature · 18 lines

Inform me of my mistakes, so I can keep imitating Homer Simpson's "Doh!".
Paolo Giarrusso, aka Blaisorblade (Skype ID "PaoloGiarrusso", ICQ 215621894)
http://www.user-mode-linux.org/~blaisorblade


	

	
		
___________________________________ 
Yahoo! Mail: gratis 1GB per i messaggi e allegati da 10MB 
http://mail.yahoo.it

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