Message ID | 20230717093001.13167-1-jniethe5@gmail.com |
---|---|
State | New |
Headers | show |
Series | [v2] tcg/ppc: Fix race in goto_tb implementation | expand |
On 7/17/23 10:30, Jordan Niethe wrote: > 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> > > --- > v2: - Use braces > - Collect tags > --- > tcg/ppc/tcg-target.c.inc | 9 +++++---- > 1 file changed, 5 insertions(+), 4 deletions(-) Applied to tcg-next. We can re-add direct branches next cycle. r~
diff --git a/tcg/ppc/tcg-target.c.inc b/tcg/ppc/tcg-target.c.inc index 8d6899cf40..329319ab8a 100644 --- a/tcg/ppc/tcg-target.c.inc +++ b/tcg/ppc/tcg-target.c.inc @@ -2533,11 +2533,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. */ @@ -2565,10 +2564,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; }