Re: [PATCH v7 05/20] linux-user: Clear translations and tb_jmp_cache on mprotect()

[Richard Henderson]

On 8/31/22 00:17, Ilya Leoshkevich wrote:

  page_set_flags(start, start + len, page_flags);
+    tb_invalidate_phys_range(start, start + len);
+
+    CPU_FOREACH(cpu) {
+    cpu_tb_jmp_cache_clear(cpu);
+    }
+
  mmap_unlock();
  return 0;
  error:


I think adding tb_invalidate_phys_range() obviates the need for
cpu_tb_jmp_cache_clear()? The lookup may still find an invalidated tb,
but it will have CF_INVALID set.


Quite right.  And we definitely don't want to have to touch a list of all threads if its 
not necessary.



r~

Re: [PATCH v7 05/20] linux-user: Clear translations and tb_jmp_cache on mprotect()

[Ilya Leoshkevich]

On Tue, 2022-08-23 at 15:05 -0700, Richard Henderson wrote:
> From: Ilya Leoshkevich 
> 
> Currently it's possible to execute pages that do not have PAGE_EXEC
> if there is an existing translation block. Fix by clearing
> tb_jmp_cache
> and invalidating TBs, which forces recheck of permission bits.
> 
> Signed-off-by: Ilya Leoshkevich 
> Message-Id: <20220817150506.592862-2-...@linux.ibm.com>
> [rth: Invalidate is required -- e.g. riscv fallthrough cross test]
> Signed-off-by: Richard Henderson 
> 
> fixup mprotect
> ---
>  linux-user/mmap.c | 8 
>  1 file changed, 8 insertions(+)
> 
> diff --git a/linux-user/mmap.c b/linux-user/mmap.c
> index 048c4135af..e9dc8848be 100644
> --- a/linux-user/mmap.c
> +++ b/linux-user/mmap.c
> @@ -115,6 +115,7 @@ int target_mprotect(abi_ulong start, abi_ulong
> len, int target_prot)
>  {
>  abi_ulong end, host_start, host_end, addr;
>  int prot1, ret, page_flags, host_prot;
> +    CPUState *cpu;
>  
>  trace_target_mprotect(start, len, target_prot);
>  
> @@ -177,7 +178,14 @@ int target_mprotect(abi_ulong start, abi_ulong
> len, int target_prot)
>  goto error;
>  }
>  }
> +
>  page_set_flags(start, start + len, page_flags);
> +    tb_invalidate_phys_range(start, start + len);
> +
> +    CPU_FOREACH(cpu) {
> +    cpu_tb_jmp_cache_clear(cpu);
> +    }
> +
>  mmap_unlock();
>  return 0;
>  error:

I think adding tb_invalidate_phys_range() obviates the need for
cpu_tb_jmp_cache_clear()? The lookup may still find an invalidated tb,
but it will have CF_INVALID set.

The following worked for me:

diff --git a/linux-user/mmap.c b/linux-user/mmap.c
index e9dc8848bed..b58e3eeb198 100644
--- a/linux-user/mmap.c
+++ b/linux-user/mmap.c
@@ -115,7 +115,6 @@ int target_mprotect(abi_ulong start, abi_ulong len,
int target_prot)
 {
 abi_ulong end, host_start, host_end, addr;
 int prot1, ret, page_flags, host_prot;
-CPUState *cpu;
 
 trace_target_mprotect(start, len, target_prot);
 
@@ -182,10 +181,6 @@ int target_mprotect(abi_ulong start, abi_ulong
len, int target_prot)
 page_set_flags(start, start + len, page_flags);
 tb_invalidate_phys_range(start, start + len);
 
-CPU_FOREACH(cpu) {
-cpu_tb_jmp_cache_clear(cpu);
-}
-
 mmap_unlock();
 return 0;
 error: