Messages in this thread | | | Date | Wed, 08 Apr 2009 07:57:23 -0700 | From | Masami Hiramatsu <> | Subject | Re: [BUGFIX][PATCH -tip] x86: fix text_poke to handle highmem pages |
| |
Ingo Molnar wrote: > * Masami Hiramatsu <mhiramat@redhat.com> wrote: > >> @@ -514,27 +515,39 @@ void *__kprobes text_poke(void *addr, const void *opcode, size_t len) >> { >> unsigned long flags; >> char *vaddr; >> - struct page *pages[2]; >> + struct page *page; >> int i; >> + unsigned long endp = ((unsigned long)addr + len) & PAGE_MASK; >> >> - if (!core_kernel_text((unsigned long)addr)) { >> - pages[0] = vmalloc_to_page(addr); >> - pages[1] = vmalloc_to_page(addr + PAGE_SIZE); >> - } else { >> - pages[0] = virt_to_page(addr); >> - WARN_ON(!PageReserved(pages[0])); >> - pages[1] = virt_to_page(addr + PAGE_SIZE); >> + /* >> + * If the written range covers 2 pages, we'll split it, because >> + * vmalloc pages are not always continuous -- e.g. 1st page is >> + * lowmem and 2nd page is highmem. >> + */ >> + if (((unsigned long)addr & PAGE_MASK) != endp) { >> + text_poke(addr, opcode, endp - (unsigned long)addr); >> + addr = (void *)endp; >> + opcode = (char *)opcode + (endp - (unsigned long)addr); >> + len -= endp - (unsigned long)addr; >> } >> - BUG_ON(!pages[0]); >> + >> + if (!core_kernel_text((unsigned long)addr)) >> + page = vmalloc_to_page(addr); >> + else >> + page = virt_to_page(addr); > > hm, the bug is upstream now. And your fix turns a > supposed-to-be-simpler kmap based patching thing back into something > fragile looking again. We might be better off with a revert - or we > do a real clean patch. > > Firstly, that core_kernel_text() distinction above looks > artificially open-coded - dont we have a proper, generic > "look-up-the-page" variant in the MM somewhere?
Actually, vmalloc_to_page() is generic one. It decodes the kernel page table directly to find struct page *. virt_to_page() is just a short-cut api.
> >> + BUG_ON(!page); >> local_irq_save(flags); >> - set_fixmap(FIX_TEXT_POKE0, page_to_phys(pages[0])); >> - if (pages[1]) >> - set_fixmap(FIX_TEXT_POKE1, page_to_phys(pages[1])); >> - vaddr = (char *)fix_to_virt(FIX_TEXT_POKE0); >> + if (PageHighMem(page)) >> + vaddr = kmap_atomic(page, KM_TEXT_POKE); >> + else { >> + set_fixmap(FIX_TEXT_POKE, page_to_phys(page)); >> + vaddr = (char *)fix_to_virt(FIX_TEXT_POKE); >> + } > > that too looks artificially complex. Why cannot we kmap lowmem pages > too? If the API isnt available on !HIGHMEM kernels .. then the > solution is to make it available, not to branch our way around it.
Hmm, why don't we enhance fixmap to handle highmem pages? (e.g. adding set_fixmap_page()) Since kmap is only for highmem kernels, I think changing it will effects more users...
Thank you,
> >> memcpy(&vaddr[(unsigned long)addr & ~PAGE_MASK], opcode, len); >> - clear_fixmap(FIX_TEXT_POKE0); >> - if (pages[1]) >> - clear_fixmap(FIX_TEXT_POKE1); >> + if (PageHighMem(page)) >> + kunmap_atomic(vaddr, KM_TEXT_POKE); >> + else >> + clear_fixmap(FIX_TEXT_POKE); > > ditto. > > Thanks, > > Ingo
-- Masami Hiramatsu
Software Engineer Hitachi Computer Products (America) Inc. Software Solutions Division
e-mail: mhiramat@redhat.com
| |