diff options
author | Jordan Niethe <jniethe5@gmail.com> | 2023-07-17 19:30:01 +1000 |
---|---|---|
committer | Richard Henderson <richard.henderson@linaro.org> | 2023-07-23 17:52:42 +0100 |
commit | 736a1588c104e9995c1831df33554df1f1def8b8 (patch) | |
tree | 86da0a6dc4f2c83be871a2fcc5ebe801c053bc41 /tcg/ppc | |
parent | d1181d29370a4318a9f11ea92065bea6bb159f83 (diff) | |
download | qemu-736a1588c104e9995c1831df33554df1f1def8b8.zip qemu-736a1588c104e9995c1831df33554df1f1def8b8.tar.gz qemu-736a1588c104e9995c1831df33554df1f1def8b8.tar.bz2 |
tcg/ppc: Fix race in goto_tb implementation
Commit 20b6643324 ("tcg/ppc: Reorg goto_tb implementation") modified
goto_tb to ensure only a single instruction was patched to prevent
incorrect behavior if a thread was in the middle of multiple
instructions when they were replaced. However this introduced a race
between loading the jmp target into TCG_REG_TB and patching and
executing the direct branch.
The relevant part of the goto_tb implementation:
ld TCG_REG_TB, TARGET_ADDR_LOCATION(TCG_REG_TB)
patch_location:
mtctr TCG_REG_TB
bctr
tb_target_set_jmp_target() will replace 'patch_location' with a direct
branch if the target is in range. The direct branch now relies on
TCG_REG_TB being set up correctly by the ld. Prior to this commit
multiple instructions were patched in for the direct branch case; these
instructions would initialize TCG_REG_TB to the same value as the branch
target.
Imagine the following sequence:
1) Thread A is executing the goto_tb sequence and loads the jmp
target into TCG_REG_TB.
2) Thread B updates the jmp target address and calls
tb_target_set_jmp_target(). This patches a new direct branch into the
goto_tb sequence.
3) Thread A executes the newly patched direct branch. The value in
TCG_REG_TB still contains the old jmp target.
TCG_REG_TB MUST contain the translation block's tc.ptr. Execution will
eventually crash after performing memory accesses generated from a
faulty value in TCG_REG_TB.
This presents as segfaults or illegal instruction exceptions.
Do not revert commit 20b6643324 as it did fix a different race
condition. Instead remove the direct branch optimization and always use
indirect branches.
The direct branch optimization can be re-added later with a race free
sequence.
Fixes: 20b6643324 ("tcg/ppc: Reorg goto_tb implementation")
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1726
Reported-by: Anushree Mathur <anushree.mathur@linux.vnet.ibm.com>
Tested-by: Anushree Mathur <anushree.mathur@linux.vnet.ibm.com>
Tested-by: Michael Tokarev <mjt@tls.msk.ru>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Co-developed-by: Benjamin Gray <bgray@linux.ibm.com>
Signed-off-by: Jordan Niethe <jniethe5@gmail.com>
Signed-off-by: Benjamin Gray <bgray@linux.ibm.com>
Message-Id: <20230717093001.13167-1-jniethe5@gmail.com>
Diffstat (limited to 'tcg/ppc')
-rw-r--r-- | tcg/ppc/tcg-target.c.inc | 9 |
1 files changed, 5 insertions, 4 deletions
diff --git a/tcg/ppc/tcg-target.c.inc b/tcg/ppc/tcg-target.c.inc index c866f2c..511e14b 100644 --- a/tcg/ppc/tcg-target.c.inc +++ b/tcg/ppc/tcg-target.c.inc @@ -2496,11 +2496,10 @@ static void tcg_out_goto_tb(TCGContext *s, int which) ptrdiff_t offset = tcg_tbrel_diff(s, (void *)ptr); tcg_out_mem_long(s, LD, LDX, TCG_REG_TB, TCG_REG_TB, offset); - /* Direct branch will be patched by tb_target_set_jmp_target. */ + /* TODO: Use direct branches when possible. */ set_jmp_insn_offset(s, which); tcg_out32(s, MTSPR | RS(TCG_REG_TB) | CTR); - /* When branch is out of range, fall through to indirect. */ tcg_out32(s, BCCTR | BO_ALWAYS); /* For the unlinked case, need to reset TCG_REG_TB. */ @@ -2528,10 +2527,12 @@ void tb_target_set_jmp_target(const TranslationBlock *tb, int n, intptr_t diff = addr - jmp_rx; tcg_insn_unit insn; + if (USE_REG_TB) { + return; + } + if (in_range_b(diff)) { insn = B | (diff & 0x3fffffc); - } else if (USE_REG_TB) { - insn = MTSPR | RS(TCG_REG_TB) | CTR; } else { insn = NOP; } |