Re: [PATCH v2] tcg/ppc: Fix race in goto_tb implementation

[Richard Henderson <richard.henderson@linaro.org>]

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~