[PATCH v2 2/3] accel/tcg: Fix sense of read-only probes in ldst_atomicity

Richard Henderson posted 3 patches 2 years, 6 months ago
Maintainers: Richard Henderson <richard.henderson@linaro.org>, Paolo Bonzini <pbonzini@redhat.com>, Warner Losh <imp@bsdimp.com>, Kyle Evans <kevans@freebsd.org>, Laurent Vivier <laurent@vivier.eu>
[PATCH v2 2/3] accel/tcg: Fix sense of read-only probes in ldst_atomicity
Posted by Richard Henderson 2 years, 6 months ago
In the initial commit, cdfac37be0d, the sense of the test is incorrect,
as the -1/0 return was confusing.  In bef6f008b981, we mechanically
invert all callers while changing to false/true return, preserving the
incorrectness of the test.

Now that the return sense is sane, it's easy to see that if !write,
then the page is not modifiable (i.e. most likely read-only, with
PROT_NONE handled via SIGSEGV).

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 accel/tcg/ldst_atomicity.c.inc | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/accel/tcg/ldst_atomicity.c.inc b/accel/tcg/ldst_atomicity.c.inc
index 4de0a80492..de70531a7a 100644
--- a/accel/tcg/ldst_atomicity.c.inc
+++ b/accel/tcg/ldst_atomicity.c.inc
@@ -159,7 +159,7 @@ static uint64_t load_atomic8_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(pv), 8, PAGE_WRITE_ORG)) {
+    if (!page_check_range(h2g(pv), 8, PAGE_WRITE_ORG)) {
         uint64_t *p = __builtin_assume_aligned(pv, 8);
         return *p;
     }
@@ -194,7 +194,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_ORG)) {
+    if (!page_check_range(h2g(p), 16, PAGE_WRITE_ORG)) {
         return *p;
     }
 #endif
-- 
2.34.1
Re: [PATCH v2 2/3] accel/tcg: Fix sense of read-only probes in ldst_atomicity
Posted by Peter Maydell 2 years, 6 months ago
On Sat, 22 Jul 2023 at 12:35, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> In the initial commit, cdfac37be0d, the sense of the test is incorrect,
> as the -1/0 return was confusing.  In bef6f008b981, we mechanically
> invert all callers while changing to false/true return, preserving the
> incorrectness of the test.
>
> Now that the return sense is sane, it's easy to see that if !write,
> then the page is not modifiable (i.e. most likely read-only, with
> PROT_NONE handled via SIGSEGV).
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM