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

    * 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


    \
     
     \ /
      Last update: 2009-04-08 17:05    [W:0.025 / U:209.104 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site