Re: [PATCH 1/1] x86: fix text_poke
From: Ingo Molnar
Date: Fri Apr 25 2008 - 12:54:24 EST
* Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx> wrote:
> But there was actually a much worse problem with my patch:
> __set_fixmap() is __init. Which means that my patch was just totally
> broken.
>
> What I really wanted to do was to just follow the page tables and mark
> it writable temporarily over the whole loop, and get rid of the whole
> mess.
>
> (We'd need to make __set_fixmap() non-init, and probably return the
> pte_t pointer that it used, so that we could then just use
> "native_pte_clear()" on the thing after having done the memcpy()).
>
> I suspect I should have just kept using vmap(), even if I do dislike
> just how insanely expensive that likely is.
clear_fixmap() is OK. I've made a tree with all these fixlets, in the
proper order and with the commit logs tidied up:
git://git.kernel.org/pub/scm/linux/kernel/git/mingo/linux-2.6-x86-fixes3.git for-linus
[ i integrated Jiri's commit to before your fix because he really
deserves that commit (and more) for his relentless debugging effort. ]
below is the full shortlog and diff. Minimally tested on 64-bit so far.
Ingo
------------------>
Ingo Molnar (3):
x86: make clear_fixmap() available on 64-bit as well
x86: make __set_fixmap() non-init
x86: harden kernel code patching
Jiri Slaby (1):
x86: fix text_poke()
Linus Torvalds (1):
x86: clean up text_poke()
arch/x86/kernel/alternative.c | 35 +++++++++++++++++++----------------
arch/x86/mm/init_64.c | 5 ++---
include/asm-x86/fixmap.h | 8 ++++++++
include/asm-x86/fixmap_32.h | 8 +++-----
include/asm-x86/fixmap_64.h | 5 +++--
5 files changed, 35 insertions(+), 26 deletions(-)
diff --git a/arch/x86/kernel/alternative.c b/arch/x86/kernel/alternative.c
index df4099d..2e39830 100644
--- a/arch/x86/kernel/alternative.c
+++ b/arch/x86/kernel/alternative.c
@@ -508,24 +508,27 @@ void *text_poke_early(void *addr, const void *opcode, size_t len)
*/
void *__kprobes text_poke(void *addr, const void *opcode, size_t len)
{
- unsigned long flags;
- char *vaddr;
- int nr_pages = 2;
+ static DEFINE_SPINLOCK(poke_lock);
+ unsigned long flags, bits;
+ bits = (unsigned long) addr;
BUG_ON(len > sizeof(long));
- BUG_ON((((long)addr + len - 1) & ~(sizeof(long) - 1))
- - ((long)addr & ~(sizeof(long) - 1)));
- if (kernel_text_address((unsigned long)addr)) {
- struct page *pages[2] = { virt_to_page(addr),
- virt_to_page(addr + PAGE_SIZE) };
- if (!pages[1])
- nr_pages = 1;
- vaddr = vmap(pages, nr_pages, VM_MAP, PAGE_KERNEL);
- BUG_ON(!vaddr);
- local_irq_save(flags);
- memcpy(&vaddr[(unsigned long)addr & ~PAGE_MASK], opcode, len);
- local_irq_restore(flags);
- vunmap(vaddr);
+ BUG_ON(len & (len-1));
+ BUG_ON(bits & (len-1));
+
+ if (core_kernel_text(bits)) {
+ unsigned long phys = __pa(addr);
+ unsigned long offset = phys & ~PAGE_MASK;
+ unsigned long virt = fix_to_virt(FIX_POKE);
+ phys &= PAGE_MASK;
+
+ WARN_ON(!PageReserved(virt_to_page(addr)));
+
+ spin_lock_irqsave(&poke_lock, flags);
+ set_fixmap(FIX_POKE, phys);
+ memcpy((void *)(virt + offset), opcode, len);
+ clear_fixmap(FIX_POKE);
+ spin_unlock_irqrestore(&poke_lock, flags);
} else {
/*
* modules are in vmalloc'ed memory, always writable.
diff --git a/arch/x86/mm/init_64.c b/arch/x86/mm/init_64.c
index 1ff7906..7a81dd0 100644
--- a/arch/x86/mm/init_64.c
+++ b/arch/x86/mm/init_64.c
@@ -135,7 +135,7 @@ static __init void *spp_getpage(void)
return ptr;
}
-static __init void
+static void
set_pte_phys(unsigned long vaddr, unsigned long phys, pgprot_t prot)
{
pgd_t *pgd;
@@ -214,8 +214,7 @@ void __init cleanup_highmap(void)
}
/* NOTE: this is meant to be run only at boot */
-void __init
-__set_fixmap(enum fixed_addresses idx, unsigned long phys, pgprot_t prot)
+void __set_fixmap(enum fixed_addresses idx, unsigned long phys, pgprot_t prot)
{
unsigned long address = __fix_to_virt(idx);
diff --git a/include/asm-x86/fixmap.h b/include/asm-x86/fixmap.h
index 382eb27..5bd2069 100644
--- a/include/asm-x86/fixmap.h
+++ b/include/asm-x86/fixmap.h
@@ -1,5 +1,13 @@
+#ifndef _ASM_FIXMAP_H
+#define _ASM_FIXMAP_H
+
#ifdef CONFIG_X86_32
# include "fixmap_32.h"
#else
# include "fixmap_64.h"
#endif
+
+#define clear_fixmap(idx) \
+ __set_fixmap(idx, 0, __pgprot(0))
+
+#endif
diff --git a/include/asm-x86/fixmap_32.h b/include/asm-x86/fixmap_32.h
index eb16651..e5db7d5 100644
--- a/include/asm-x86/fixmap_32.h
+++ b/include/asm-x86/fixmap_32.h
@@ -10,8 +10,8 @@
* Support of BIGMEM added by Gerhard Wichert, Siemens AG, July 1999
*/
-#ifndef _ASM_FIXMAP_H
-#define _ASM_FIXMAP_H
+#ifndef _ASM_FIXMAP_32_H
+#define _ASM_FIXMAP_32_H
/* used by vmalloc.c, vsyscall.lds.S.
@@ -55,6 +55,7 @@ enum fixed_addresses {
FIX_HOLE,
FIX_VDSO,
FIX_DBGP_BASE,
+ FIX_POKE,
FIX_EARLYCON_MEM_BASE,
#ifdef CONFIG_X86_LOCAL_APIC
FIX_APIC_BASE, /* local (CPU) APIC) -- required for SMP or not */
@@ -121,9 +122,6 @@ extern void reserve_top_address(unsigned long reserve);
#define set_fixmap_nocache(idx, phys) \
__set_fixmap(idx, phys, PAGE_KERNEL_NOCACHE)
-#define clear_fixmap(idx) \
- __set_fixmap(idx, 0, __pgprot(0))
-
#define FIXADDR_TOP ((unsigned long)__FIXADDR_TOP)
#define __FIXADDR_SIZE (__end_of_permanent_fixed_addresses << PAGE_SHIFT)
diff --git a/include/asm-x86/fixmap_64.h b/include/asm-x86/fixmap_64.h
index f3d7685..ba80e6b 100644
--- a/include/asm-x86/fixmap_64.h
+++ b/include/asm-x86/fixmap_64.h
@@ -8,8 +8,8 @@
* Copyright (C) 1998 Ingo Molnar
*/
-#ifndef _ASM_FIXMAP_H
-#define _ASM_FIXMAP_H
+#ifndef _ASM_FIXMAP_64_H
+#define _ASM_FIXMAP_64_H
#include <linux/kernel.h>
#include <asm/apicdef.h>
@@ -37,6 +37,7 @@ enum fixed_addresses {
VSYSCALL_FIRST_PAGE = VSYSCALL_LAST_PAGE
+ ((VSYSCALL_END-VSYSCALL_START) >> PAGE_SHIFT) - 1,
VSYSCALL_HPET,
+ FIX_POKE,
FIX_DBGP_BASE,
FIX_EARLYCON_MEM_BASE,
FIX_HPET_BASE,
--
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/