Re: [BUGFIX][PATCH -tip] x86: fix text_poke to handle highmem pages

[Ingo Molnar]

* Masami Hiramatsu <mhiramat@xxxxxxxxxx> 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?

> +	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.

>  	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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/