Re: [PATCH] ioremap: fix iounmap numpages

Re: [PATCH] ioremap: fix iounmap numpages

[Jeremy Fitzhardinge]

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

[PATCH] ioremap: fix iounmap numpages

[Dave Young]

Hi,
The second parameter of change_page_attr in iounmap is wrong, it should be (p->size - 1) >> PAGE_SHIFT

Signed-off-by: Dave Young <hidave.darkstar@gmail.com> 
---

diff -upr linux/arch/i386/mm/ioremap.c linux.new/arch/i386/mm/ioremap.c
--- linux/arch/i386/mm/ioremap.c	2007-06-29 16:48:40.000000000 +0000
+++ linux.new/arch/i386/mm/ioremap.c	2007-06-29 16:50:09.000000000 +0000
@@ -196,7 +196,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,
+				 (p->size - 1) >> PAGE_SHIFT,
 				 PAGE_KERNEL);
 		global_flush_tlb();
 	} 

Regards
dave

Re: [PATCH] ioremap: fix iounmap numpages

[Dave Young]

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

------------[ cut here ]------------
kernel BUG at arch/i386/mm/pageattr.c:175!
invalid opcode: 0000 [#1]
PREEMPT SMP
Modules linked in: cfi_probe gen_probe netsc520 mtdpart mtdcore
chipreg map_funcs snd_seq_dummy snd_seq_oss snd_seq_midi_event
snd_seq snd_seq_device snd_pcm_oss snd_mixer_oss snd_hda_intel snd_pcm
snd_timer snd soundcore snd_page_alloc capability common
cap e100 mii agpgart pcspkr psmouse
CPU:    0
EIP:    0060:[<c0118ff5>]    Not tainted VLI
EFLAGS: 00010082   (2.6.22-rc6 #15)
EIP is at __change_page_attr+0x185/0x1a0
eax: 0000afe0   ebx: c1006000   ecx: c100afe0   edx: c1000000
esi: c057fc00   edi: c0300000   ebp: 00000163   esp: f74c7f40
ds: 007b   es: 007b   fs: 00d8  gs: 0033  ss: 0068
Process modprobe (pid: 1440, ti=f74c6000 task=f742aa20 task.ti=f74c6000)
Stack: 00000163 c1006000 00000100 00000000 00000101 c011904b 00000202 00000163
       f7b46ee0 f8980000 c1004000 f74c6000 c0118c60 f886e237 f88810a6 f8881600
       0805f9c8 00000001 f88830d2 f88810cc 00100000 00000000 00200000 00000000
Call Trace:
 [<c011904b>] change_page_attr+0x3b/0x70
 [<c0118c60>] iounmap+0xd0/0xe0
 [<f88830d2>] init_netsc520+0xd2/0xf5 [netsc520]
 [<c01451a9>] sys_init_module+0xd9/0x140
 [<c0104238>] syscall_call+0x7/0xb
 [<c0420000>] packet_rcv+0x90/0x320
 =======================
Code: c3 85 db b8 f4 ff ff ff 74 af a1 80 15 5a c0 89 d9 89 fa 29 c1
c1 f9 05 89 f0 c1 e1 0c 09 e9 e8 22 fe ff ff 89 d9 e9 07 f
f ff ff <0f> 0b eb fe 0f 0b eb fe 0f 0b eb fe eb 0d 90 90 90 90 90 90 90
EIP: [<c0118ff5>] __change_page_attr+0x185/0x1a0 SS:ESP 0068:f74c7f40

Regards
dave

Re: [PATCH] ioremap: fix iounmap numpages

[Dmitry Monakhov]

On 00:33 Пнд 02 Июл     , 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.
It's absolutely true in fact last page is just guard page witch can't be
toched. And (size -1) magic was used in all io**map code before,
but when someone remove this magic by occasion :)
BTW: XEN currently use this magic.
arch/i386/mm/ioremap-xen.c: iounmap() {
....
	/* p->size includes the guard page, but cpa doesn't like that */
	change_page_attr(virt_to_page(bus_to_virt(p->phys_addr)),
		(p->size - PAGE_SIZE) >> PAGE_SHIFT

> 
>  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:
> 
>  ------------[ cut here ]------------
>  kernel BUG at arch/i386/mm/pageattr.c:175!
>  invalid opcode: 0000 [#1]
>  PREEMPT SMP
>  Modules linked in: cfi_probe gen_probe netsc520 mtdpart mtdcore
>  chipreg map_funcs snd_seq_dummy snd_seq_oss snd_seq_midi_event
>  snd_seq snd_seq_device snd_pcm_oss snd_mixer_oss snd_hda_intel snd_pcm
>  snd_timer snd soundcore snd_page_alloc capability common
>  cap e100 mii agpgart pcspkr psmouse
>  CPU:    0
>  EIP:    0060:[<c0118ff5>]    Not tainted VLI
>  EFLAGS: 00010082   (2.6.22-rc6 #15)
>  EIP is at __change_page_attr+0x185/0x1a0
>  eax: 0000afe0   ebx: c1006000   ecx: c100afe0   edx: c1000000
>  esi: c057fc00   edi: c0300000   ebp: 00000163   esp: f74c7f40
>  ds: 007b   es: 007b   fs: 00d8  gs: 0033  ss: 0068
>  Process modprobe (pid: 1440, ti=f74c6000 task=f742aa20 task.ti=f74c6000)
>  Stack: 00000163 c1006000 00000100 00000000 00000101 c011904b 00000202 
>  00000163
>        f7b46ee0 f8980000 c1004000 f74c6000 c0118c60 f886e237 f88810a6 
>  f8881600
>        0805f9c8 00000001 f88830d2 f88810cc 00100000 00000000 00200000 
>  00000000
>  Call Trace:
>  [<c011904b>] change_page_attr+0x3b/0x70
>  [<c0118c60>] iounmap+0xd0/0xe0
>  [<f88830d2>] init_netsc520+0xd2/0xf5 [netsc520]
>  [<c01451a9>] sys_init_module+0xd9/0x140
>  [<c0104238>] syscall_call+0x7/0xb
>  [<c0420000>] packet_rcv+0x90/0x320
>  =======================
>  Code: c3 85 db b8 f4 ff ff ff 74 af a1 80 15 5a c0 89 d9 89 fa 29 c1
>  c1 f9 05 89 f0 c1 e1 0c 09 e9 e8 22 fe ff ff 89 d9 e9 07 f
>  f ff ff <0f> 0b eb fe 0f 0b eb fe 0f 0b eb fe eb 0d 90 90 90 90 90 90 90
>  EIP: [<c0118ff5>] __change_page_attr+0x185/0x1a0 SS:ESP 0068:f74c7f40
> 
>  Regards
>  dave
>  -
>  To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
>  the body of a message to majordomo@vger.kernel.org
>  More majordomo info at  http://vger.kernel.org/majordomo-info.html
>  Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] ioremap: fix iounmap numpages

[Jeremy Fitzhardinge]

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