lkml.org 
[lkml]   [2009]   [Apr]   [8]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [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



    \
     
     \ /
      Last update: 2009-04-08 17:01    [W:2.130 / U:0.180 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site