[PATCH RFC 0/1] tcg: Always pass the full write size to notdirty_write()

[PATCH RFC 0/1] tcg: Always pass the full write size to notdirty_write()

[Ilya Leoshkevich]

Hi,

this is the fix for an issue I complained about a few days back on the
IRC. Unfortunately my reproducer [1] does not work anymore, so I'm
sending this separately and as an RFC.

The user-visible effect was that:

- If a TB writes to itself;
- The address of the write is before the TB start;
- The newly written instruction causes an exception;
- Then single-stepping the instruction that does a write gets you to
  an interrupt handler and not to the newly written "bad" instruction.

You can still see that something is wrong with the softmmu reproducer
in the debugger though. In helper_vstl(), probe_write_access() does not
lead to cpu_loop_exit_noexc() - which I think it should, but
cpu_stq_be_data_ra() still does, masking the issue.

Best regards,
Ilya

[1] https://lists.gnu.org/archive/html/qemu-devel/2023-08/msg01081.html

Ilya Leoshkevich (1):
  tcg: Always pass the full write size to notdirty_write()

 accel/tcg/cputlb.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

-- 
2.41.0

[PATCH RFC 1/1] tcg: Always pass the full write size to notdirty_write()

[Ilya Leoshkevich]

One of notdirty_write()'s responsibilities is detecting self-modifying
code. Some functions pass the full size of a write to it, some pass 1.
When a write to a code section begins before a TB start, but then
overlaps the TB, the paths that pass 1 don't flush a TB and don't
return to the translator loop.

This may be masked, one example being HELPER(vstl). There,
probe_write_access() ultimately calls notdirty_write() with a size of
1 and misses self-modifying code. However, cpu_stq_be_data_ra()
ultimately calls mmu_watch_or_dirty(), which in turn calls
notdirty_write() with the full size.

It's still worth improving this, because there may still be
user-visible adverse effects in other helpers.

Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
---
 accel/tcg/cputlb.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/accel/tcg/cputlb.c b/accel/tcg/cputlb.c
index d68fa6867ce..aa3cffbc11a 100644
--- a/accel/tcg/cputlb.c
+++ b/accel/tcg/cputlb.c
@@ -1582,7 +1582,7 @@ int probe_access_full(CPUArchState *env, vaddr addr, int size,
 
     /* Handle clean RAM pages.  */
     if (unlikely(flags & TLB_NOTDIRTY)) {
-        notdirty_write(env_cpu(env), addr, 1, *pfull, retaddr);
+        notdirty_write(env_cpu(env), addr, size, *pfull, retaddr);
         flags &= ~TLB_NOTDIRTY;
     }
 
@@ -1605,7 +1605,7 @@ int probe_access_full_mmu(CPUArchState *env, vaddr addr, int size,
 
     /* Handle clean RAM pages.  */
     if (unlikely(flags & TLB_NOTDIRTY)) {
-        notdirty_write(env_cpu(env), addr, 1, *pfull, 0);
+        notdirty_write(env_cpu(env), addr, size, *pfull, 0);
         flags &= ~TLB_NOTDIRTY;
     }
 
@@ -1626,7 +1626,7 @@ int probe_access_flags(CPUArchState *env, vaddr addr, int size,
 
     /* Handle clean RAM pages. */
     if (unlikely(flags & TLB_NOTDIRTY)) {
-        notdirty_write(env_cpu(env), addr, 1, full, retaddr);
+        notdirty_write(env_cpu(env), addr, size, full, retaddr);
         flags &= ~TLB_NOTDIRTY;
     }
 
@@ -1661,7 +1661,7 @@ void *probe_access(CPUArchState *env, vaddr addr, int size,
 
         /* Handle clean RAM pages.  */
         if (flags & TLB_NOTDIRTY) {
-            notdirty_write(env_cpu(env), addr, 1, full, retaddr);
+            notdirty_write(env_cpu(env), addr, size, full, retaddr);
         }
     }
 
-- 
2.41.0

Re: [PATCH RFC 1/1] tcg: Always pass the full write size to notdirty_write()

[Richard Henderson]

On 8/7/23 06:56, Ilya Leoshkevich wrote:
> One of notdirty_write()'s responsibilities is detecting self-modifying
> code. Some functions pass the full size of a write to it, some pass 1.
> When a write to a code section begins before a TB start, but then
> overlaps the TB, the paths that pass 1 don't flush a TB and don't
> return to the translator loop.
> 
> This may be masked, one example being HELPER(vstl). There,
> probe_write_access() ultimately calls notdirty_write() with a size of
> 1 and misses self-modifying code. However, cpu_stq_be_data_ra()
> ultimately calls mmu_watch_or_dirty(), which in turn calls
> notdirty_write() with the full size.
> 
> It's still worth improving this, because there may still be
> user-visible adverse effects in other helpers.
> 
> Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>

IIRC there are some uses of probe_access_* that set size == 0.
Should we adjust addr+size to cover the whole page for that case?
That seems to be the intent, anyway.


r~

> ---
>   accel/tcg/cputlb.c | 8 ++++----
>   1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/accel/tcg/cputlb.c b/accel/tcg/cputlb.c
> index d68fa6867ce..aa3cffbc11a 100644
> --- a/accel/tcg/cputlb.c
> +++ b/accel/tcg/cputlb.c
> @@ -1582,7 +1582,7 @@ int probe_access_full(CPUArchState *env, vaddr addr, int size,
>   
>       /* Handle clean RAM pages.  */
>       if (unlikely(flags & TLB_NOTDIRTY)) {
> -        notdirty_write(env_cpu(env), addr, 1, *pfull, retaddr);
> +        notdirty_write(env_cpu(env), addr, size, *pfull, retaddr);
>           flags &= ~TLB_NOTDIRTY;
>       }
>   
> @@ -1605,7 +1605,7 @@ int probe_access_full_mmu(CPUArchState *env, vaddr addr, int size,
>   
>       /* Handle clean RAM pages.  */
>       if (unlikely(flags & TLB_NOTDIRTY)) {
> -        notdirty_write(env_cpu(env), addr, 1, *pfull, 0);
> +        notdirty_write(env_cpu(env), addr, size, *pfull, 0);
>           flags &= ~TLB_NOTDIRTY;
>       }
>   
> @@ -1626,7 +1626,7 @@ int probe_access_flags(CPUArchState *env, vaddr addr, int size,
>   
>       /* Handle clean RAM pages. */
>       if (unlikely(flags & TLB_NOTDIRTY)) {
> -        notdirty_write(env_cpu(env), addr, 1, full, retaddr);
> +        notdirty_write(env_cpu(env), addr, size, full, retaddr);
>           flags &= ~TLB_NOTDIRTY;
>       }
>   
> @@ -1661,7 +1661,7 @@ void *probe_access(CPUArchState *env, vaddr addr, int size,
>   
>           /* Handle clean RAM pages.  */
>           if (flags & TLB_NOTDIRTY) {
> -            notdirty_write(env_cpu(env), addr, 1, full, retaddr);
> +            notdirty_write(env_cpu(env), addr, size, full, retaddr);
>           }
>       }
>   

Re: [PATCH RFC 1/1] tcg: Always pass the full write size to notdirty_write()

[Ilya Leoshkevich]

On Mon, 2023-08-07 at 11:21 -0700, Richard Henderson wrote:
> On 8/7/23 06:56, Ilya Leoshkevich wrote:
> > One of notdirty_write()'s responsibilities is detecting self-
> > modifying
> > code. Some functions pass the full size of a write to it, some pass
> > 1.
> > When a write to a code section begins before a TB start, but then
> > overlaps the TB, the paths that pass 1 don't flush a TB and don't
> > return to the translator loop.
> > 
> > This may be masked, one example being HELPER(vstl). There,
> > probe_write_access() ultimately calls notdirty_write() with a size
> > of
> > 1 and misses self-modifying code. However, cpu_stq_be_data_ra()
> > ultimately calls mmu_watch_or_dirty(), which in turn calls
> > notdirty_write() with the full size.
> > 
> > It's still worth improving this, because there may still be
> > user-visible adverse effects in other helpers.
> > 
> > Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
> 
> IIRC there are some uses of probe_access_* that set size == 0.
> Should we adjust addr+size to cover the whole page for that case?
> That seems to be the intent, anyway.

There is a comment that says we shouldn't do watchpoint/smc detection
in this case:

    /* Per the interface, size == 0 merely faults the access. */
    if (size == 0) {
        return NULL;
    }

Come to think of it, qemu-user works this way too: SMC is detected on
the actual access, not the probe:

    helper_vstl()
      cpu_stq_be_data_ra()
        ...
           stq_he_p()
             <signal handler called>
               host_signal_handler()
                 handle_sigsegv_accerr_write()
                   page_unprotect()
                     tb_invalidate_phys_page_unwind()
                   cpu_loop_exit_noexc()

So all this is probably fine, I now think it's better to leave the code
as is, especially given that I cannot reproduce the original problem
anymore.

Re: [PATCH RFC 1/1] tcg: Always pass the full write size to notdirty_write()

[Richard Henderson]

On 8/8/23 02:59, Ilya Leoshkevich wrote:
> On Mon, 2023-08-07 at 11:21 -0700, Richard Henderson wrote:
>> IIRC there are some uses of probe_access_* that set size == 0.
>> Should we adjust addr+size to cover the whole page for that case?
>> That seems to be the intent, anyway.
> 
> There is a comment that says we shouldn't do watchpoint/smc detection
> in this case:
> 
>      /* Per the interface, size == 0 merely faults the access. */
>      if (size == 0) {
>          return NULL;
>      }
> 
> Come to think of it, qemu-user works this way too: SMC is detected on
> the actual access, not the probe:
> 
>      helper_vstl()
>        cpu_stq_be_data_ra()
>          ...
>             stq_he_p()
>               <signal handler called>
>                 host_signal_handler()
>                   handle_sigsegv_accerr_write()
>                     page_unprotect()
>                       tb_invalidate_phys_page_unwind()
>                     cpu_loop_exit_noexc()
> 
> So all this is probably fine, I now think it's better to leave the code
> as is, especially given that I cannot reproduce the original problem
> anymore.

Ok then.


r~