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

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

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.


r~