Skip to content

Commit

Permalink
accel/tcg: Clear PAGE_WRITE before translation
Browse files Browse the repository at this point in the history 
translate_insn() implementations fetch instruction bytes piecemeal,
which can cause qemu-user to generate inconsistent translations if
another thread modifies them concurrently [1].

Fix by making pages containing translated instruction non-writable
right before loading instruction bytes from them.

[1] https://lists.nongnu.org/archive/html/qemu-devel/2021-08/msg00644.html

Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
Message-Id: <20210805204835.158918-1-iii@linux.ibm.com>
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
  • Loading branch information
@iii-i @rth7680
iii-i authored and rth7680 committed Sep 14, 2021
1 parent 4e11689 commit f025692
Show file tree
Hide file tree
Showing 4 changed files with 97 additions and 41 deletions.
59 changes: 34 additions & 25 deletions accel/tcg/translate-all.c
View file
Expand Up @@ -1297,31 +1297,8 @@ static inline void tb_page_add(PageDesc *p, TranslationBlock *tb,
invalidate_page_bitmap(p);

#if defined(CONFIG_USER_ONLY)
if (p->flags & PAGE_WRITE) {
target_ulong addr;
PageDesc *p2;
int prot;

/* force the host page as non writable (writes will have a
page fault + mprotect overhead) */
page_addr &= qemu_host_page_mask;
prot = 0;
for (addr = page_addr; addr < page_addr + qemu_host_page_size;
addr += TARGET_PAGE_SIZE) {

p2 = page_find(addr >> TARGET_PAGE_BITS);
if (!p2) {
continue;
}
prot |= p2->flags;
p2->flags &= ~PAGE_WRITE;
}
mprotect(g2h_untagged(page_addr), qemu_host_page_size,
(prot & PAGE_BITS) & ~PAGE_WRITE);
if (DEBUG_TB_INVALIDATE_GATE) {
printf("protecting code page: 0x" TB_PAGE_ADDR_FMT "\n", page_addr);
}
}
/* translator_loop() must have made all TB pages non-writable */
assert(!(p->flags & PAGE_WRITE));
#else
/* if some code is already present, then the pages are already
protected. So we handle the case where only the first TB is
Expand Down Expand Up @@ -2394,6 +2371,38 @@ int page_check_range(target_ulong start, target_ulong len, int flags)
return 0;
}

void page_protect(tb_page_addr_t page_addr)
{
target_ulong addr;
PageDesc *p;
int prot;

p = page_find(page_addr >> TARGET_PAGE_BITS);
if (p && (p->flags & PAGE_WRITE)) {
/*
* Force the host page as non writable (writes will have a page fault +
* mprotect overhead).
*/
page_addr &= qemu_host_page_mask;
prot = 0;
for (addr = page_addr; addr < page_addr + qemu_host_page_size;
addr += TARGET_PAGE_SIZE) {

p = page_find(addr >> TARGET_PAGE_BITS);
if (!p) {
continue;
}
prot |= p->flags;
p->flags &= ~PAGE_WRITE;
}
mprotect(g2h_untagged(page_addr), qemu_host_page_size,
(prot & PAGE_BITS) & ~PAGE_WRITE);
if (DEBUG_TB_INVALIDATE_GATE) {
printf("protecting code page: 0x" TB_PAGE_ADDR_FMT "\n", page_addr);
}
}
}

/* called from signal handler: invalidate the code and unprotect the
* page. Return 0 if the fault was not handled, 1 if it was handled,
* and 2 if it was handled but the caller must cause the TB to be
Expand Down
39 changes: 39 additions & 0 deletions accel/tcg/translator.c
View file
Expand Up @@ -42,6 +42,15 @@ bool translator_use_goto_tb(DisasContextBase *db, target_ulong dest)
return ((db->pc_first ^ dest) & TARGET_PAGE_MASK) == 0;
}

static inline void translator_page_protect(DisasContextBase *dcbase,
target_ulong pc)
{
#ifdef CONFIG_USER_ONLY
dcbase->page_protect_end = pc | ~TARGET_PAGE_MASK;
page_protect(pc);
#endif
}

void translator_loop(const TranslatorOps *ops, DisasContextBase *db,
CPUState *cpu, TranslationBlock *tb, int max_insns)
{
Expand All @@ -56,6 +65,7 @@ void translator_loop(const TranslatorOps *ops, DisasContextBase *db,
db->num_insns = 0;
db->max_insns = max_insns;
db->singlestep_enabled = cflags & CF_SINGLE_STEP;
translator_page_protect(db, db->pc_next);

ops->init_disas_context(db, cpu);
tcg_debug_assert(db->is_jmp == DISAS_NEXT); /* no early exit */
Expand Down Expand Up @@ -137,3 +147,32 @@ void translator_loop(const TranslatorOps *ops, DisasContextBase *db,
}
#endif
}

static inline void translator_maybe_page_protect(DisasContextBase *dcbase,
target_ulong pc, size_t len)
{
#ifdef CONFIG_USER_ONLY
target_ulong end = pc + len - 1;

if (end > dcbase->page_protect_end) {
translator_page_protect(dcbase, end);
}
#endif
}

#define GEN_TRANSLATOR_LD(fullname, type, load_fn, swap_fn) \
type fullname ## _swap(CPUArchState *env, DisasContextBase *dcbase, \
abi_ptr pc, bool do_swap) \
{ \
translator_maybe_page_protect(dcbase, pc, sizeof(type)); \
type ret = load_fn(env, pc); \
if (do_swap) { \
ret = swap_fn(ret); \
} \
plugin_insn_append(&ret, sizeof(ret)); \
return ret; \
}

FOR_EACH_TRANSLATOR_LD(GEN_TRANSLATOR_LD)

#undef GEN_TRANSLATOR_LD
1 change: 1 addition & 0 deletions include/exec/translate-all.h
View file
Expand Up @@ -33,6 +33,7 @@ void tb_invalidate_phys_page_range(tb_page_addr_t start, tb_page_addr_t end);
void tb_check_watchpoint(CPUState *cpu, uintptr_t retaddr);

#ifdef CONFIG_USER_ONLY
void page_protect(tb_page_addr_t page_addr);
int page_unprotect(target_ulong address, uintptr_t pc);
#endif

Expand Down
39 changes: 23 additions & 16 deletions include/exec/translator.h
View file
Expand Up @@ -23,6 +23,7 @@
#include "exec/exec-all.h"
#include "exec/cpu_ldst.h"
#include "exec/plugin-gen.h"
#include "exec/translate-all.h"
#include "tcg/tcg.h"


Expand Down Expand Up @@ -74,6 +75,17 @@ typedef struct DisasContextBase {
int num_insns;
int max_insns;
bool singlestep_enabled;
#ifdef CONFIG_USER_ONLY
/*
* Guest address of the last byte of the last protected page.
*
* Pages containing the translated instructions are made non-writable in
* order to achieve consistency in case another thread is modifying the
* code while translate_insn() fetches the instruction bytes piecemeal.
* Such writer threads are blocked on mmap_lock() in page_unprotect().
*/
target_ulong page_protect_end;
#endif
} DisasContextBase;

/**
Expand Down Expand Up @@ -156,28 +168,23 @@ bool translator_use_goto_tb(DisasContextBase *db, target_ulong dest);
*/

#define GEN_TRANSLATOR_LD(fullname, type, load_fn, swap_fn) \
static inline type \
fullname ## _swap(CPUArchState *env, DisasContextBase *dcbase, \
abi_ptr pc, bool do_swap) \
{ \
type ret = load_fn(env, pc); \
if (do_swap) { \
ret = swap_fn(ret); \
} \
plugin_insn_append(&ret, sizeof(ret)); \
return ret; \
} \
type fullname ## _swap(CPUArchState *env, DisasContextBase *dcbase, \
abi_ptr pc, bool do_swap); \
static inline type fullname(CPUArchState *env, \
DisasContextBase *dcbase, abi_ptr pc) \
{ \
return fullname ## _swap(env, dcbase, pc, false); \
}

GEN_TRANSLATOR_LD(translator_ldub, uint8_t, cpu_ldub_code, /* no swap */)
GEN_TRANSLATOR_LD(translator_ldsw, int16_t, cpu_ldsw_code, bswap16)
GEN_TRANSLATOR_LD(translator_lduw, uint16_t, cpu_lduw_code, bswap16)
GEN_TRANSLATOR_LD(translator_ldl, uint32_t, cpu_ldl_code, bswap32)
GEN_TRANSLATOR_LD(translator_ldq, uint64_t, cpu_ldq_code, bswap64)
#define FOR_EACH_TRANSLATOR_LD(F) \
F(translator_ldub, uint8_t, cpu_ldub_code, /* no swap */) \
F(translator_ldsw, int16_t, cpu_ldsw_code, bswap16) \
F(translator_lduw, uint16_t, cpu_lduw_code, bswap16) \
F(translator_ldl, uint32_t, cpu_ldl_code, bswap32) \
F(translator_ldq, uint64_t, cpu_ldq_code, bswap64)

FOR_EACH_TRANSLATOR_LD(GEN_TRANSLATOR_LD)

#undef GEN_TRANSLATOR_LD

#endif /* EXEC__TRANSLATOR_H */

0 comments on commit f025692

Please sign in to comment.