* [RFC PATCH] target/i386: allow access_ptr to force slow path on failed probe
@ 2024-08-07 16:02 Alex Bennée
2024-08-07 16:12 ` Alexandre IOOSS
2024-08-07 23:10 ` Richard Henderson
0 siblings, 2 replies; 5+ messages in thread
From: Alex Bennée @ 2024-08-07 16:02 UTC (permalink / raw)
To: qemu-devel
Cc: Alex Bennée, Paolo Bonzini, Richard Henderson,
Eduardo Habkost
When we are using TCG plugin memory callbacks probe_access_internal
will return TLB_MMIO to force the slow path for memory access. This
results in probe_access returning NULL but the x86 access_ptr function
happily accepts an empty haddr resulting in segfault hilarity.
Check for an empty haddr to prevent the segfault and enable plugins to
track all the memory operations for the x86 save/restore helpers.
Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2489
Fixes: 6d03226b42 (plugins: force slow path when plugins instrument memory ops)
---
target/i386/tcg/access.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/target/i386/tcg/access.c b/target/i386/tcg/access.c
index 56a1181ea5..8ea5c453a0 100644
--- a/target/i386/tcg/access.c
+++ b/target/i386/tcg/access.c
@@ -58,6 +58,10 @@ static void *access_ptr(X86Access *ac, vaddr addr, unsigned len)
assert(addr >= ac->vaddr);
+ if (!ac->haddr1) {
+ return NULL;
+ }
+
#ifdef CONFIG_USER_ONLY
assert(offset <= ac->size1 - len);
return ac->haddr1 + offset;
--
2.39.2
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [RFC PATCH] target/i386: allow access_ptr to force slow path on failed probe
2024-08-07 16:02 [RFC PATCH] target/i386: allow access_ptr to force slow path on failed probe Alex Bennée
@ 2024-08-07 16:12 ` Alexandre IOOSS
2024-08-07 23:10 ` Richard Henderson
1 sibling, 0 replies; 5+ messages in thread
From: Alexandre IOOSS @ 2024-08-07 16:12 UTC (permalink / raw)
To: Alex Bennée, qemu-devel
Cc: Paolo Bonzini, Richard Henderson, Eduardo Habkost
On 8/7/24 18:02, Alex Bennée wrote:
> When we are using TCG plugin memory callbacks probe_access_internal
> will return TLB_MMIO to force the slow path for memory access. This
> results in probe_access returning NULL but the x86 access_ptr function
> happily accepts an empty haddr resulting in segfault hilarity.
>
> Check for an empty haddr to prevent the segfault and enable plugins to
> track all the memory operations for the x86 save/restore helpers.
>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2489
> Fixes: 6d03226b42 (plugins: force slow path when plugins instrument memory ops)
> ---
> target/i386/tcg/access.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/target/i386/tcg/access.c b/target/i386/tcg/access.c
> index 56a1181ea5..8ea5c453a0 100644
> --- a/target/i386/tcg/access.c
> +++ b/target/i386/tcg/access.c
> @@ -58,6 +58,10 @@ static void *access_ptr(X86Access *ac, vaddr addr, unsigned len)
>
> assert(addr >= ac->vaddr);
>
> + if (!ac->haddr1) {
> + return NULL;
> + }
> +
> #ifdef CONFIG_USER_ONLY
> assert(offset <= ac->size1 - len);
> return ac->haddr1 + offset;
I confirm that this patch fixes the crash I reported here:
https://gitlab.com/qemu-project/qemu/-/issues/2489
Reviewed-by: Alexandre Iooss <erdnaxe@crans.org>
Thanks,
--
Alexandre
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [RFC PATCH] target/i386: allow access_ptr to force slow path on failed probe
2024-08-07 16:02 [RFC PATCH] target/i386: allow access_ptr to force slow path on failed probe Alex Bennée
2024-08-07 16:12 ` Alexandre IOOSS
@ 2024-08-07 23:10 ` Richard Henderson
2024-08-13 13:12 ` Alex Bennée
1 sibling, 1 reply; 5+ messages in thread
From: Richard Henderson @ 2024-08-07 23:10 UTC (permalink / raw)
To: Alex Bennée, qemu-devel; +Cc: Paolo Bonzini, Eduardo Habkost
On 8/8/24 02:02, Alex Bennée wrote:
> When we are using TCG plugin memory callbacks probe_access_internal
> will return TLB_MMIO to force the slow path for memory access. This
> results in probe_access returning NULL but the x86 access_ptr function
> happily accepts an empty haddr resulting in segfault hilarity.
>
> Check for an empty haddr to prevent the segfault and enable plugins to
> track all the memory operations for the x86 save/restore helpers.
>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2489
> Fixes: 6d03226b42 (plugins: force slow path when plugins instrument memory ops)
> ---
> target/i386/tcg/access.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/target/i386/tcg/access.c b/target/i386/tcg/access.c
> index 56a1181ea5..8ea5c453a0 100644
> --- a/target/i386/tcg/access.c
> +++ b/target/i386/tcg/access.c
> @@ -58,6 +58,10 @@ static void *access_ptr(X86Access *ac, vaddr addr, unsigned len)
>
> assert(addr >= ac->vaddr);
>
> + if (!ac->haddr1) {
> + return NULL;
> + }
> +
> #ifdef CONFIG_USER_ONLY
> assert(offset <= ac->size1 - len);
> return ac->haddr1 + offset;
You need to remove the test_ptr macro below as well.
r~
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [RFC PATCH] target/i386: allow access_ptr to force slow path on failed probe
2024-08-07 23:10 ` Richard Henderson
@ 2024-08-13 13:12 ` Alex Bennée
2024-08-13 14:48 ` Richard Henderson
0 siblings, 1 reply; 5+ messages in thread
From: Alex Bennée @ 2024-08-13 13:12 UTC (permalink / raw)
To: Richard Henderson; +Cc: qemu-devel, Paolo Bonzini, Eduardo Habkost
Richard Henderson <richard.henderson@linaro.org> writes:
> On 8/8/24 02:02, Alex Bennée wrote:
>> When we are using TCG plugin memory callbacks probe_access_internal
>> will return TLB_MMIO to force the slow path for memory access. This
>> results in probe_access returning NULL but the x86 access_ptr function
>> happily accepts an empty haddr resulting in segfault hilarity.
>> Check for an empty haddr to prevent the segfault and enable plugins
>> to
>> track all the memory operations for the x86 save/restore helpers.
>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2489
>> Fixes: 6d03226b42 (plugins: force slow path when plugins instrument memory ops)
>> ---
>> target/i386/tcg/access.c | 4 ++++
>> 1 file changed, 4 insertions(+)
>> diff --git a/target/i386/tcg/access.c b/target/i386/tcg/access.c
>> index 56a1181ea5..8ea5c453a0 100644
>> --- a/target/i386/tcg/access.c
>> +++ b/target/i386/tcg/access.c
>> @@ -58,6 +58,10 @@ static void *access_ptr(X86Access *ac, vaddr addr, unsigned len)
>> assert(addr >= ac->vaddr);
>> + if (!ac->haddr1) {
>> + return NULL;
>> + }
>> +
>> #ifdef CONFIG_USER_ONLY
>> assert(offset <= ac->size1 - len);
>> return ac->haddr1 + offset;
>
> You need to remove the test_ptr macro below as well.
So we fall-back to the slow path for linux-user as well?
--
Alex Bennée
Virtualisation Tech Lead @ Linaro
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [RFC PATCH] target/i386: allow access_ptr to force slow path on failed probe
2024-08-13 13:12 ` Alex Bennée
@ 2024-08-13 14:48 ` Richard Henderson
0 siblings, 0 replies; 5+ messages in thread
From: Richard Henderson @ 2024-08-13 14:48 UTC (permalink / raw)
To: Alex Bennée; +Cc: qemu-devel, Paolo Bonzini, Eduardo Habkost
On 8/13/24 23:12, Alex Bennée wrote:
> Richard Henderson <richard.henderson@linaro.org> writes:
>
>> On 8/8/24 02:02, Alex Bennée wrote:
>>> When we are using TCG plugin memory callbacks probe_access_internal
>>> will return TLB_MMIO to force the slow path for memory access. This
>>> results in probe_access returning NULL but the x86 access_ptr function
>>> happily accepts an empty haddr resulting in segfault hilarity.
>>> Check for an empty haddr to prevent the segfault and enable plugins
>>> to
>>> track all the memory operations for the x86 save/restore helpers.
>>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>>> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2489
>>> Fixes: 6d03226b42 (plugins: force slow path when plugins instrument memory ops)
>>> ---
>>> target/i386/tcg/access.c | 4 ++++
>>> 1 file changed, 4 insertions(+)
>>> diff --git a/target/i386/tcg/access.c b/target/i386/tcg/access.c
>>> index 56a1181ea5..8ea5c453a0 100644
>>> --- a/target/i386/tcg/access.c
>>> +++ b/target/i386/tcg/access.c
>>> @@ -58,6 +58,10 @@ static void *access_ptr(X86Access *ac, vaddr addr, unsigned len)
>>> assert(addr >= ac->vaddr);
>>> + if (!ac->haddr1) {
>>> + return NULL;
>>> + }
>>> +
>>> #ifdef CONFIG_USER_ONLY
>>> assert(offset <= ac->size1 - len);
>>> return ac->haddr1 + offset;
>>
>> You need to remove the test_ptr macro below as well.
>
> So we fall-back to the slow path for linux-user as well?
So we don't ignore the null, and dereference it anyway.
r~
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2024-08-13 14:49 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-08-07 16:02 [RFC PATCH] target/i386: allow access_ptr to force slow path on failed probe Alex Bennée
2024-08-07 16:12 ` Alexandre IOOSS
2024-08-07 23:10 ` Richard Henderson
2024-08-13 13:12 ` Alex Bennée
2024-08-13 14:48 ` Richard Henderson
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).