Messages in this thread | | | Date | Wed, 8 Apr 2009 16:59:53 +0200 | From | Ingo Molnar <> | Subject | Re: [BUGFIX][PATCH -tip] x86: fix text_poke to handle highmem pages |
| |
* Masami Hiramatsu <mhiramat@redhat.com> wrote:
> 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,
Sure, whichever way looks more logical to you - my only condition is that it should result in a clean patch! :-)
Ingo
| |