[PATCH v3 0/2] target/i386: fix hang when using slow path for ptw_setl

[PATCH v3 0/2] target/i386: fix hang when using slow path for ptw_setl

[Pierrick Bouvier]

Most of the details are available in first patch. Second one is there to ensure
we'll have a useful error message if start_exclusive is called from cpu_exec
again.

I'm a bit puzzled that we never triggered this hang before. Is there something
wrong with the potential slow path for ptw_setl, or is it simply very uncommon?

v2:
- get current cpu from local variable instead of current_cpu global var.
- change condition to check cpu is running as current_cpu will never be NULL.

Pierrick Bouvier (2):
  target/i386: fix hang when using slow path for ptw_setl
  cpu: ensure we don't call start_exclusive from cpu_exec

 cpu-common.c                         | 3 +++
 target/i386/tcg/sysemu/excp_helper.c | 5 +++++
 2 files changed, 8 insertions(+)

-- 
2.39.5

[PATCH v3 1/2] target/i386: fix hang when using slow path for ptw_setl

[Pierrick Bouvier]

When instrumenting memory accesses for plugin, we force memory accesses
to use the slow path for mmu [1]. This create a situation where we end
up calling ptw_setl_slow. This was fixed recently in [2] but the issue
still could appear out of plugins use case.

Since this function gets called during a cpu_exec, start_exclusive then
hangs. This exclusive section was introduced initially for security
reasons [3].

I suspect this code path was never triggered, because ptw_setl_slow
would always be called transitively from cpu_exec, resulting in a hang.

[1] https://gitlab.com/qemu-project/qemu/-/commit/6d03226b42247b68ab2f0b3663e0f624335a4055
[2] https://gitlab.com/qemu-project/qemu/-/commit/115ade42d50144c15b74368d32dc734ea277d853
[3] https://gitlab.com/qemu-project/qemu/-/issues/279

Fixes: https://gitlab.com/qemu-project/qemu/-/issues/2566
Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
---
 target/i386/tcg/sysemu/excp_helper.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/target/i386/tcg/sysemu/excp_helper.c b/target/i386/tcg/sysemu/excp_helper.c
index da187c8792a..ddc51e3e0b8 100644
--- a/target/i386/tcg/sysemu/excp_helper.c
+++ b/target/i386/tcg/sysemu/excp_helper.c
@@ -107,6 +107,10 @@ static bool ptw_setl_slow(const PTETranslate *in, uint32_t old, uint32_t new)
 {
     uint32_t cmp;
 
+    CPUState *cpu = env_cpu(in->env);
+    /* We are in cpu_exec, and start_exclusive can't be called directly.*/
+    g_assert(cpu->running);
+    cpu_exec_end(cpu);
     /* Does x86 really perform a rmw cycle on mmio for ptw? */
     start_exclusive();
     cmp = cpu_ldl_mmuidx_ra(in->env, in->gaddr, in->ptw_idx, 0);
@@ -114,6 +118,7 @@ static bool ptw_setl_slow(const PTETranslate *in, uint32_t old, uint32_t new)
         cpu_stl_mmuidx_ra(in->env, in->gaddr, new, in->ptw_idx, 0);
     }
     end_exclusive();
+    cpu_exec_start(cpu);
     return cmp == old;
 }
 
-- 
2.39.5

[PATCH v3 2/2] cpu: ensure we don't call start_exclusive from cpu_exec

[Pierrick Bouvier]

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
---
 cpu-common.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/cpu-common.c b/cpu-common.c
index 6b262233a3b..0d607bbe493 100644
--- a/cpu-common.c
+++ b/cpu-common.c
@@ -194,6 +194,9 @@ void start_exclusive(void)
     CPUState *other_cpu;
     int running_cpus;
 
+    /* Ensure we are not running, or start_exclusive will be blocked. */
+    g_assert(!current_cpu->running);
+
     if (current_cpu->exclusive_context_count) {
         current_cpu->exclusive_context_count++;
         return;
-- 
2.39.5

Re: [PATCH v3 2/2] cpu: ensure we don't call start_exclusive from cpu_exec

[Philippe Mathieu-Daudé]

On 25/10/24 14:58, Pierrick Bouvier wrote:
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
> Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
> ---
>   cpu-common.c | 3 +++
>   1 file changed, 3 insertions(+)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH v3 1/2] target/i386: fix hang when using slow path for ptw_setl

[Richard Henderson]

On 10/25/24 18:58, Pierrick Bouvier wrote:
> When instrumenting memory accesses for plugin, we force memory accesses
> to use the slow path for mmu [1]. This create a situation where we end
> up calling ptw_setl_slow. This was fixed recently in [2] but the issue
> still could appear out of plugins use case.
> 
> Since this function gets called during a cpu_exec, start_exclusive then
> hangs. This exclusive section was introduced initially for security
> reasons [3].
> 
> I suspect this code path was never triggered, because ptw_setl_slow
> would always be called transitively from cpu_exec, resulting in a hang.
> 
> [1]https://gitlab.com/qemu-project/qemu/-/commit/6d03226b42247b68ab2f0b3663e0f624335a4055
> [2]https://gitlab.com/qemu-project/qemu/-/commit/115ade42d50144c15b74368d32dc734ea277d853
> [3]https://gitlab.com/qemu-project/qemu/-/issues/279
> 
> Fixes:https://gitlab.com/qemu-project/qemu/-/issues/2566
> Signed-off-by: Pierrick Bouvier<pierrick.bouvier@linaro.org>
> ---
>   target/i386/tcg/sysemu/excp_helper.c | 5 +++++
>   1 file changed, 5 insertions(+)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [PATCH v3 0/2] target/i386: fix hang when using slow path for ptw_setl

[Pierrick Bouvier]

Hi,

On 10/25/24 10:58, Pierrick Bouvier wrote:
> Most of the details are available in first patch. Second one is there to ensure
> we'll have a useful error message if start_exclusive is called from cpu_exec
> again.
> 
> I'm a bit puzzled that we never triggered this hang before. Is there something
> wrong with the potential slow path for ptw_setl, or is it simply very uncommon?
> 
> v2:
> - get current cpu from local variable instead of current_cpu global var.
> - change condition to check cpu is running as current_cpu will never be NULL.
> 
> Pierrick Bouvier (2):
>    target/i386: fix hang when using slow path for ptw_setl
>    cpu: ensure we don't call start_exclusive from cpu_exec
> 
>   cpu-common.c                         | 3 +++
>   target/i386/tcg/sysemu/excp_helper.c | 5 +++++
>   2 files changed, 8 insertions(+)
> 

Gentle ping. Could this series be pulled by someone for 9.2?

Thanks,
Pierrick

Re: [PATCH v3 0/2] target/i386: fix hang when using slow path for ptw_setl

[Richard Henderson]

On 10/25/24 10:58, Pierrick Bouvier wrote:
> Most of the details are available in first patch. Second one is there to ensure
> we'll have a useful error message if start_exclusive is called from cpu_exec
> again.
> 
> I'm a bit puzzled that we never triggered this hang before. Is there something
> wrong with the potential slow path for ptw_setl, or is it simply very uncommon?
> 
> v2:
> - get current cpu from local variable instead of current_cpu global var.
> - change condition to check cpu is running as current_cpu will never be NULL.
> 
> Pierrick Bouvier (2):
>    target/i386: fix hang when using slow path for ptw_setl
>    cpu: ensure we don't call start_exclusive from cpu_exec
> 
>   cpu-common.c                         | 3 +++
>   target/i386/tcg/sysemu/excp_helper.c | 5 +++++
>   2 files changed, 8 insertions(+)
> 

Queued, thanks.


r~

Re: [PATCH v3 1/2] target/i386: fix hang when using slow path for ptw_setl

[Michael Tokarev]

25.10.2024 20:58, Pierrick Bouvier wrote:
> When instrumenting memory accesses for plugin, we force memory accesses
> to use the slow path for mmu [1]. This create a situation where we end
> up calling ptw_setl_slow. This was fixed recently in [2] but the issue
> still could appear out of plugins use case.
> 
> Since this function gets called during a cpu_exec, start_exclusive then
> hangs. This exclusive section was introduced initially for security
> reasons [3].
> 
> I suspect this code path was never triggered, because ptw_setl_slow
> would always be called transitively from cpu_exec, resulting in a hang.
> 
> [1] https://gitlab.com/qemu-project/qemu/-/commit/6d03226b42247b68ab2f0b3663e0f624335a4055
> [2] https://gitlab.com/qemu-project/qemu/-/commit/115ade42d50144c15b74368d32dc734ea277d853
> [3] https://gitlab.com/qemu-project/qemu/-/issues/279
> 
> Fixes: https://gitlab.com/qemu-project/qemu/-/issues/2566
> Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>

[1] is in 8.2.x. [2] is in 9.2.tobe, and marked as should be picked up
for stable (I picked it up for 8.2.x, 9.0.x and 9.1.x).

Shouldn't this one be picked up for stable too, as an addition fix
ontop of [2]?  Or is it not important? (I guess since it's reported
in our issue tracker, it is problematic for someone already).

I picked this one up for 8.2, 9.0 and 9.1 stable series -- please
let me know if I should not.

Also, what about the 2/2 in this series, "cpu: ensure we don't call
start_exclusive from cpu_exec", which is 779f30a01af8566780cefc8639505b758950afb3
in master now?

Thanks,

/mjt

>   target/i386/tcg/sysemu/excp_helper.c | 5 +++++
>   1 file changed, 5 insertions(+)
> 
> diff --git a/target/i386/tcg/sysemu/excp_helper.c b/target/i386/tcg/sysemu/excp_helper.c
> index da187c8792a..ddc51e3e0b8 100644
> --- a/target/i386/tcg/sysemu/excp_helper.c
> +++ b/target/i386/tcg/sysemu/excp_helper.c
> @@ -107,6 +107,10 @@ static bool ptw_setl_slow(const PTETranslate *in, uint32_t old, uint32_t new)
>   {
>       uint32_t cmp;
>   
> +    CPUState *cpu = env_cpu(in->env);
> +    /* We are in cpu_exec, and start_exclusive can't be called directly.*/
> +    g_assert(cpu->running);
> +    cpu_exec_end(cpu);
>       /* Does x86 really perform a rmw cycle on mmio for ptw? */
>       start_exclusive();
>       cmp = cpu_ldl_mmuidx_ra(in->env, in->gaddr, in->ptw_idx, 0);
> @@ -114,6 +118,7 @@ static bool ptw_setl_slow(const PTETranslate *in, uint32_t old, uint32_t new)
>           cpu_stl_mmuidx_ra(in->env, in->gaddr, new, in->ptw_idx, 0);
>       }
>       end_exclusive();
> +    cpu_exec_start(cpu);
>       return cmp == old;
>   }
>   


-- 
GPG Key transition (from rsa2048 to rsa4096) since 2024-04-24.
New key: rsa4096/61AD3D98ECDF2C8E  9D8B E14E 3F2A 9DD7 9199  28F1 61AD 3D98 ECDF 2C8E
Old key: rsa2048/457CE0A0804465C5  6EE1 95D1 886E 8FFB 810D  4324 457C E0A0 8044 65C5
Transition statement: http://www.corpit.ru/mjt/gpg-transition-2024.txt

Re: [PATCH v3 1/2] target/i386: fix hang when using slow path for ptw_setl

[Pierrick Bouvier]

Hi Michael,

On 11/16/24 19:49, Michael Tokarev wrote:
> 25.10.2024 20:58, Pierrick Bouvier wrote:
>> When instrumenting memory accesses for plugin, we force memory accesses
>> to use the slow path for mmu [1]. This create a situation where we end
>> up calling ptw_setl_slow. This was fixed recently in [2] but the issue
>> still could appear out of plugins use case.
>>
>> Since this function gets called during a cpu_exec, start_exclusive then
>> hangs. This exclusive section was introduced initially for security
>> reasons [3].
>>
>> I suspect this code path was never triggered, because ptw_setl_slow
>> would always be called transitively from cpu_exec, resulting in a hang.
>>
>> [1] https://gitlab.com/qemu-project/qemu/-/commit/6d03226b42247b68ab2f0b3663e0f624335a4055
>> [2] https://gitlab.com/qemu-project/qemu/-/commit/115ade42d50144c15b74368d32dc734ea277d853
>> [3] https://gitlab.com/qemu-project/qemu/-/issues/279
>>
>> Fixes: https://gitlab.com/qemu-project/qemu/-/issues/2566
>> Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
> 
> [1] is in 8.2.x. [2] is in 9.2.tobe, and marked as should be picked up
> for stable (I picked it up for 8.2.x, 9.0.x and 9.1.x).
> 
> Shouldn't this one be picked up for stable too, as an addition fix
> ontop of [2]?  Or is it not important? (I guess since it's reported
> in our issue tracker, it is problematic for someone already).
> 
> I picked this one up for 8.2, 9.0 and 9.1 stable series -- please
> let me know if I should not.
> 

You can safely integrate this on all stable branches. It will fix the 
issue with plugins, and might possibly fix an issue with the memory 
access if someone hit that path. The original problematic change was 
missing this fix from the start.

> Also, what about the 2/2 in this series, "cpu: ensure we don't call
> start_exclusive from cpu_exec", which is 779f30a01af8566780cefc8639505b758950afb3
> in master now?
> 

It's an assert to ensure we don't hit the same blocking situation in the 
future (with new changes), so I don't think there is an added value to 
integrate that on stable branches.

Thanks,
Pierrick

> Thanks,
> 
> /mjt
> 
>>    target/i386/tcg/sysemu/excp_helper.c | 5 +++++
>>    1 file changed, 5 insertions(+)
>>
>> diff --git a/target/i386/tcg/sysemu/excp_helper.c b/target/i386/tcg/sysemu/excp_helper.c
>> index da187c8792a..ddc51e3e0b8 100644
>> --- a/target/i386/tcg/sysemu/excp_helper.c
>> +++ b/target/i386/tcg/sysemu/excp_helper.c
>> @@ -107,6 +107,10 @@ static bool ptw_setl_slow(const PTETranslate *in, uint32_t old, uint32_t new)
>>    {
>>        uint32_t cmp;
>>    
>> +    CPUState *cpu = env_cpu(in->env);
>> +    /* We are in cpu_exec, and start_exclusive can't be called directly.*/
>> +    g_assert(cpu->running);
>> +    cpu_exec_end(cpu);
>>        /* Does x86 really perform a rmw cycle on mmio for ptw? */
>>        start_exclusive();
>>        cmp = cpu_ldl_mmuidx_ra(in->env, in->gaddr, in->ptw_idx, 0);
>> @@ -114,6 +118,7 @@ static bool ptw_setl_slow(const PTETranslate *in, uint32_t old, uint32_t new)
>>            cpu_stl_mmuidx_ra(in->env, in->gaddr, new, in->ptw_idx, 0);
>>        }
>>        end_exclusive();
>> +    cpu_exec_start(cpu);
>>        return cmp == old;
>>    }
>>    
> 
>