Messages in this thread | | | Subject | Re: [PATCH] Add debugging boundary check to pfn_to_page | From | Dave Hansen <> | Date | Wed, 08 Jun 2011 13:49:28 -0700 |
| |
On Wed, 2011-06-08 at 15:18 -0400, Eric B Munson wrote: > -#define __pfn_to_page(pfn) \ > -({ unsigned long __pfn = (pfn); \ > - struct mem_section *__sec = __pfn_to_section(__pfn); \ > - __section_mem_map_addr(__sec) + __pfn; \ > +#ifdef CONFIG_DEBUG_MEMORY_MODEL > +#define __pfn_to_page(pfn) \ > +({ unsigned long __pfn = (pfn); \ > + struct mem_section *__sec = __pfn_to_section(__pfn); \ > + struct page *__page = __section_mem_map_addr(__sec) + __pfn; \ > + WARN_ON(__page->flags == 0); \ > + __page; \
What was the scenario you're trying to catch here? If you give a really crummy __pfn, you'll probably go off the end of one of the mem_section[] arrays, and get garbage back for __sec. You might also get a NULL back from __section_mem_map_addr() if the section is possibly valid, but just not present on this particular system.
I _think_ the only kind of bug this will catch is if you have a valid section, with a valid section_mem_map[] but still manage to find yourself with an 'struct page' unclaimed by any zone and thus uninitialized.
You could catch a lot more cases by being a bit more paranoid:
void check_pfn(unsigned long pfn) { int nid; // hacked in from pfn_to_nid: // Don't actually do this, add a new helper near pfn_to_nid() // Can this even fit in the physnode_map? if (pfn / PAGES_PER_ELEMENT > ARRAY_SIZE(physnode_map)) WARN();
// Is there a valid nid there? nid = pfn_to_nid(pfn); if (nid == -1) WARN(); // check against NODE_DATA(nid)->node_start_pfn; // check against NODE_DATA(nid)->node_spanned_pages; } > }) > +#else > +#define __pfn_to_page(pfn) \ > +({ unsigned long __pfn = (pfn); \ > + struct mem_section *__sec = __pfn_to_section(__pfn); \ > + __section_mem_map_addr(__sec) + __pfn; \ > +}) > +#endif /* CONFIG_DEBUG_MEMORY_MODEL */
Instead of making a completely new __pfn_to_page() in the debugging case, I'd probably do something like this:
#ifdef CONFIG_DEBUG_MEMORY_MODEL #define check_foo(foo) {\ some_check_here(foo);\ WARN_ON(foo->flags);\ } #else #define check_foo(foo) do{}while(0) #endif;
#define __pfn_to_page(pfn) \ ({ unsigned long __pfn = (pfn); \ struct mem_section *__sec = __pfn_to_section(__pfn); \ struct page *__page = __section_mem_map_addr(__sec) + __pfn; \ check_foo(page) \ __page; \ })
That'll make sure that the two copies of __pfn_to_page() don't accidentally diverge. It also makes it a lot easier to read, I think.
-- Dave
| |