Re: [PATCH] ioremap: fix iounmap numpages

[Jeremy Fitzhardinge <jeremy@goop.org>]

Dave Young wrote:
>> On 6/29/07, Jeremy Fitzhardinge <jeremy@goop.org> wrote:
>> Dave Young wrote:
>> > Hi,
>> > The second parameter of change_page_attr in iounmap is wrong, it 
>> should be (p->size - 1) >> PAGE_SHIFT
>> >
>>
>> Why's that?  Isn't p->size always going to be a pagesize multiple; in
>> which case, why would you want to change_page_attr on n-1 pages?
>>
>> Are you seeing a problem that this patch fixes?
>>
>>     J
>>
> Hi,
> Please read the  ioremap_nocache function, the page number is 
> calculated by:
>
> last_addr = phys_addr + size - 1;
> npages = (last_addr - phys_addr) >> PAGE_SHIFT;
>
> but the pages number in iounmap is p->size >> PAGE_SHIFT, the result
> is not consistent.
>
> If there's no netsc520 device then the netsc520 mtd driver
> initializing will cause oops.
> I debugged it with some printk messages, find that the ioremap_nocache
> call change_page_attr 256 times, but the iounmap call change_page_attr
> more than 256 times, so kernel oops. please finid the oops message:

OK, so the problem is that get_vm_area allocates the vm_area with an 
extra page added as a guard page.  iounmap uses p->size directly, 
without taking the guard page into account.

I don't see why this doesn't cause more problems.  I guess uncached 
iomappings are not used that much?

Anyway, I think this is the right fix:

Subject: fix iounmap's use of vm_struct's size field

get_vm_area always returns an area with an adjacent guard page.  That
guard page is included in vm_struct.size.  iounmap uses vm_struct.size
to determine how much address space needs to have change_page_attr
applied to it, which will BUG if applied to the guard page.

This patch adds a helper function - get_vm_area_size() in
linux/vmalloc.h - to return the actual size of a vm area, and uses it
to make iounmap do the right thing.  There are probably other places
which should be using get_vm_area_size().

Thanks to Dave Young <hidave.darkstar@gmail.com> for debugging the
problem.

Signed-off-by: Jeremy Fitzhardinge <jeremy@xensource.com>
Cc: Dave Young <hidave.darkstar@gmail.com>
Cc: Chuck Ebbert <cebbert@redhat.com>
Cc: Andi Kleen <ak@suse.de>

---
 arch/i386/mm/ioremap.c  |    2 +-
 include/linux/vmalloc.h |    6 ++++++
 2 files changed, 7 insertions(+), 1 deletion(-)

===================================================================
--- a/arch/i386/mm/ioremap.c
+++ b/arch/i386/mm/ioremap.c
@@ -193,7 +193,7 @@ void iounmap(volatile void __iomem *addr
 	/* Reset the direct mapping. Can block */
 	if ((p->flags >> 20) && p->phys_addr < virt_to_phys(high_memory) - 1) {
 		change_page_attr(virt_to_page(__va(p->phys_addr)),
-				 p->size >> PAGE_SHIFT,
+				 get_vm_area_size(p) >> PAGE_SHIFT,
 				 PAGE_KERNEL);
 		global_flush_tlb();
 	} 
===================================================================
--- a/include/linux/vmalloc.h
+++ b/include/linux/vmalloc.h
@@ -70,6 +70,12 @@ extern int map_vm_area(struct vm_struct 
 			struct page ***pages);
 extern void unmap_kernel_range(unsigned long addr, unsigned long size);
 
+static inline size_t get_vm_area_size(const struct vm_struct *area)
+{
+	/* return actual size without guard page */
+	return area->size - PAGE_SIZE;
+}
+
 /* Allocate/destroy a 'vmalloc' VM area. */
 extern struct vm_struct *alloc_vm_area(size_t size);
 extern void free_vm_area(struct vm_struct *area);