Re: [PATCH v4 02/16] accel/tcg: Fix check for page writeability in load_atomic16_or_exit

[Peter Maydell]

On Tue, 30 May 2023 at 14:58, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> On 5/30/23 06:44, Peter Maydell wrote:
> > On Fri, 26 May 2023 at 01:24, Richard Henderson
> > <richard.henderson@linaro.org> wrote:
> >>
> >> PAGE_WRITE is current writability, as modified by TB protection;
> >> PAGE_WRITE_ORG is the original page writability.
> >>
> >> Fixes: cdfac37be0d ("accel/tcg: Honor atomicity of loads")
> >> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> >> ---
> >>   accel/tcg/ldst_atomicity.c.inc | 2 +-
> >>   1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/accel/tcg/ldst_atomicity.c.inc 
> >> b/accel/tcg/ldst_atomicity.c.inc
> >> index 0f6b3f8ab6..57163f5ca2 100644
> >> --- a/accel/tcg/ldst_atomicity.c.inc
> >> +++ b/accel/tcg/ldst_atomicity.c.inc
> >> @@ -191,7 +191,7 @@ static Int128 load_atomic16_or_exit(CPUArchState *env, 
> >> uintptr_t ra, void *pv)
> >>        * another process, because the fallback start_exclusive solution
> >>        * provides no protection across processes.
> >>        */
> >> -    if (!page_check_range(h2g(p), 16, PAGE_WRITE)) {
> >> +    if (!page_check_range(h2g(p), 16, PAGE_WRITE_ORG)) {
> >>           return *p;
> >>       }
> >>   #endif
> >> --
> >> 2.34.1
> >
> > load_atomic8_or_exit() has a similar condition, so
> > we should change either both or neither.
> >
> > So, if I understand this correctly, !PAGE_WRITE_ORG is a
> > stricter test than !PAGE_WRITE, so we're saying "don't
> > do a simple non-atomic load if the page was only read-only
> > because we've translated code out of it". Why is it
> > not OK to do the non-atomic load in that case? I guess
> > because we don't have the mmap lock, so some other thread
> > might nip in and do an access that causes us to invalidate
> > the TBs and move the page back to writeable?
>
> This is about falling through to the cmpxchg below: if !PAGE_WRITE_ORG, then 
> the page is
> really not writable, we will SIGSEGV, and handle_sigsegv_accerr_write will 
> kill the process.

Right, but if !PAGE_WRITE_ORG then that implies also !PAGE_WRITE,
so we do that even without this change ?

-- PMM