* [PATCH] target/i386: Check NULL monitor pointer when injecting MCE
@ 2024-03-20 5:21 Tao Su
2024-03-20 6:02 ` Markus Armbruster
0 siblings, 1 reply; 5+ messages in thread
From: Tao Su @ 2024-03-20 5:21 UTC (permalink / raw)
To: qemu-devel; +Cc: pbonzini, xiaoyao.li, alex.bennee, tao1.su
monitor_puts() doesn't check the monitor pointer, but do_inject_x86_mce()
may have a parameter with NULL monitor pointer. Check the monitor pointer
before calling monitor_puts().
Fixes: bf0c50d4aa85 (monitor: expose monitor_puts to rest of code)
Reviwed-by: Xiaoyao Li <xiaoyao.li@intel.com>
Signed-off-by: Tao Su <tao1.su@linux.intel.com>
---
target/i386/helper.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/target/i386/helper.c b/target/i386/helper.c
index 2070dd0dda..a9ff830a17 100644
--- a/target/i386/helper.c
+++ b/target/i386/helper.c
@@ -430,7 +430,8 @@ static void do_inject_x86_mce(CPUState *cs, run_on_cpu_data data)
if (need_reset) {
emit_guest_memory_failure(MEMORY_FAILURE_ACTION_RESET, ar,
recursive);
- monitor_puts(params->mon, msg);
+ if (params->mon)
+ monitor_puts(params->mon, msg);
qemu_log_mask(CPU_LOG_RESET, "%s\n", msg);
qemu_system_reset_request(SHUTDOWN_CAUSE_GUEST_RESET);
return;
--
2.34.1
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] target/i386: Check NULL monitor pointer when injecting MCE
2024-03-20 5:21 [PATCH] target/i386: Check NULL monitor pointer when injecting MCE Tao Su
@ 2024-03-20 6:02 ` Markus Armbruster
2024-03-20 7:17 ` Philippe Mathieu-Daudé
2024-03-20 7:44 ` Tao Su
0 siblings, 2 replies; 5+ messages in thread
From: Markus Armbruster @ 2024-03-20 6:02 UTC (permalink / raw)
To: Tao Su; +Cc: qemu-devel, pbonzini, xiaoyao.li, alex.bennee
Tao Su <tao1.su@linux.intel.com> writes:
> monitor_puts() doesn't check the monitor pointer, but do_inject_x86_mce()
> may have a parameter with NULL monitor pointer. Check the monitor pointer
> before calling monitor_puts().
>
> Fixes: bf0c50d4aa85 (monitor: expose monitor_puts to rest of code)
> Reviwed-by: Xiaoyao Li <xiaoyao.li@intel.com>
> Signed-off-by: Tao Su <tao1.su@linux.intel.com>
> ---
> target/i386/helper.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/target/i386/helper.c b/target/i386/helper.c
> index 2070dd0dda..a9ff830a17 100644
> --- a/target/i386/helper.c
> +++ b/target/i386/helper.c
> @@ -430,7 +430,8 @@ static void do_inject_x86_mce(CPUState *cs, run_on_cpu_data data)
> if (need_reset) {
> emit_guest_memory_failure(MEMORY_FAILURE_ACTION_RESET, ar,
> recursive);
> - monitor_puts(params->mon, msg);
> + if (params->mon)
> + monitor_puts(params->mon, msg);
> qemu_log_mask(CPU_LOG_RESET, "%s\n", msg);
> qemu_system_reset_request(SHUTDOWN_CAUSE_GUEST_RESET);
> return;
Could instead revert the broken part of commit bf0c50d4aa85:
- monitor_puts(params->mon, msg);
+ monitor_printf(params->mon, "%s", msg);
qemu_log_mask(CPU_LOG_RESET, "%s\n", msg);
Then the fact that we send the same message to monitor and log is again
more obvious.
Either way:
Reviewed-by: Markus Armbruster <armbru@redhat.com>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] target/i386: Check NULL monitor pointer when injecting MCE
2024-03-20 6:02 ` Markus Armbruster
@ 2024-03-20 7:17 ` Philippe Mathieu-Daudé
2024-03-20 7:53 ` Tao Su
2024-03-20 7:44 ` Tao Su
1 sibling, 1 reply; 5+ messages in thread
From: Philippe Mathieu-Daudé @ 2024-03-20 7:17 UTC (permalink / raw)
To: Markus Armbruster, Tao Su; +Cc: qemu-devel, pbonzini, xiaoyao.li, alex.bennee
Hi Tao,
On 20/3/24 07:02, Markus Armbruster wrote:
> Tao Su <tao1.su@linux.intel.com> writes:
>
>> monitor_puts() doesn't check the monitor pointer, but do_inject_x86_mce()
>> may have a parameter with NULL monitor pointer. Check the monitor pointer
>> before calling monitor_puts().
>>
>> Fixes: bf0c50d4aa85 (monitor: expose monitor_puts to rest of code)
>> Reviwed-by: Xiaoyao Li <xiaoyao.li@intel.com>
>> Signed-off-by: Tao Su <tao1.su@linux.intel.com>
>> ---
>> target/i386/helper.c | 3 ++-
>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/target/i386/helper.c b/target/i386/helper.c
>> index 2070dd0dda..a9ff830a17 100644
>> --- a/target/i386/helper.c
>> +++ b/target/i386/helper.c
>> @@ -430,7 +430,8 @@ static void do_inject_x86_mce(CPUState *cs, run_on_cpu_data data)
>> if (need_reset) {
>> emit_guest_memory_failure(MEMORY_FAILURE_ACTION_RESET, ar,
>> recursive);
>> - monitor_puts(params->mon, msg);
>> + if (params->mon)
Missing braces, see QEMU coding style:
https://www.qemu.org/docs/master/devel/style.html#block-structure
>> + monitor_puts(params->mon, msg);
>> qemu_log_mask(CPU_LOG_RESET, "%s\n", msg);
>> qemu_system_reset_request(SHUTDOWN_CAUSE_GUEST_RESET);
>> return;
>
> Could instead revert the broken part of commit bf0c50d4aa85:
>
> - monitor_puts(params->mon, msg);
> + monitor_printf(params->mon, "%s", msg);
> qemu_log_mask(CPU_LOG_RESET, "%s\n", msg);
>
> Then the fact that we send the same message to monitor and log is again
> more obvious.
>
> Either way:
> Reviewed-by: Markus Armbruster <armbru@redhat.com>
>
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] target/i386: Check NULL monitor pointer when injecting MCE
2024-03-20 6:02 ` Markus Armbruster
2024-03-20 7:17 ` Philippe Mathieu-Daudé
@ 2024-03-20 7:44 ` Tao Su
1 sibling, 0 replies; 5+ messages in thread
From: Tao Su @ 2024-03-20 7:44 UTC (permalink / raw)
To: Markus Armbruster; +Cc: qemu-devel, pbonzini, xiaoyao.li, alex.bennee
On Wed, Mar 20, 2024 at 07:02:46AM +0100, Markus Armbruster wrote:
> Tao Su <tao1.su@linux.intel.com> writes:
>
> > monitor_puts() doesn't check the monitor pointer, but do_inject_x86_mce()
> > may have a parameter with NULL monitor pointer. Check the monitor pointer
> > before calling monitor_puts().
> >
> > Fixes: bf0c50d4aa85 (monitor: expose monitor_puts to rest of code)
> > Reviwed-by: Xiaoyao Li <xiaoyao.li@intel.com>
> > Signed-off-by: Tao Su <tao1.su@linux.intel.com>
> > ---
> > target/i386/helper.c | 3 ++-
> > 1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/target/i386/helper.c b/target/i386/helper.c
> > index 2070dd0dda..a9ff830a17 100644
> > --- a/target/i386/helper.c
> > +++ b/target/i386/helper.c
> > @@ -430,7 +430,8 @@ static void do_inject_x86_mce(CPUState *cs, run_on_cpu_data data)
> > if (need_reset) {
> > emit_guest_memory_failure(MEMORY_FAILURE_ACTION_RESET, ar,
> > recursive);
> > - monitor_puts(params->mon, msg);
> > + if (params->mon)
> > + monitor_puts(params->mon, msg);
> > qemu_log_mask(CPU_LOG_RESET, "%s\n", msg);
> > qemu_system_reset_request(SHUTDOWN_CAUSE_GUEST_RESET);
> > return;
>
> Could instead revert the broken part of commit bf0c50d4aa85:
>
> - monitor_puts(params->mon, msg);
> + monitor_printf(params->mon, "%s", msg);
> qemu_log_mask(CPU_LOG_RESET, "%s\n", msg);
>
> Then the fact that we send the same message to monitor and log is again
> more obvious.
Good suggestion. I will send a v2 with this change.
>
> Either way:
> Reviewed-by: Markus Armbruster <armbru@redhat.com>
Thanks for review!
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] target/i386: Check NULL monitor pointer when injecting MCE
2024-03-20 7:17 ` Philippe Mathieu-Daudé
@ 2024-03-20 7:53 ` Tao Su
0 siblings, 0 replies; 5+ messages in thread
From: Tao Su @ 2024-03-20 7:53 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: Markus Armbruster, qemu-devel, pbonzini, xiaoyao.li, alex.bennee
On Wed, Mar 20, 2024 at 08:17:36AM +0100, Philippe Mathieu-Daudé wrote:
> Hi Tao,
>
> On 20/3/24 07:02, Markus Armbruster wrote:
> > Tao Su <tao1.su@linux.intel.com> writes:
> >
> > > monitor_puts() doesn't check the monitor pointer, but do_inject_x86_mce()
> > > may have a parameter with NULL monitor pointer. Check the monitor pointer
> > > before calling monitor_puts().
> > >
> > > Fixes: bf0c50d4aa85 (monitor: expose monitor_puts to rest of code)
> > > Reviwed-by: Xiaoyao Li <xiaoyao.li@intel.com>
> > > Signed-off-by: Tao Su <tao1.su@linux.intel.com>
> > > ---
> > > target/i386/helper.c | 3 ++-
> > > 1 file changed, 2 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/target/i386/helper.c b/target/i386/helper.c
> > > index 2070dd0dda..a9ff830a17 100644
> > > --- a/target/i386/helper.c
> > > +++ b/target/i386/helper.c
> > > @@ -430,7 +430,8 @@ static void do_inject_x86_mce(CPUState *cs, run_on_cpu_data data)
> > > if (need_reset) {
> > > emit_guest_memory_failure(MEMORY_FAILURE_ACTION_RESET, ar,
> > > recursive);
> > > - monitor_puts(params->mon, msg);
> > > + if (params->mon)
>
> Missing braces, see QEMU coding style:
> https://www.qemu.org/docs/master/devel/style.html#block-structure
Yes, I prefer to revert the broken part.
Anyway, I got this point and will always pay attention to it, thanks for
reminding.
>
> > > + monitor_puts(params->mon, msg);
> > > qemu_log_mask(CPU_LOG_RESET, "%s\n", msg);
> > > qemu_system_reset_request(SHUTDOWN_CAUSE_GUEST_RESET);
> > > return;
> >
> > Could instead revert the broken part of commit bf0c50d4aa85:
> >
> > - monitor_puts(params->mon, msg);
> > + monitor_printf(params->mon, "%s", msg);
> > qemu_log_mask(CPU_LOG_RESET, "%s\n", msg);
> >
> > Then the fact that we send the same message to monitor and log is again
> > more obvious.
> >
> > Either way:
> > Reviewed-by: Markus Armbruster <armbru@redhat.com>
> >
> >
>
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2024-03-20 7:56 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-03-20 5:21 [PATCH] target/i386: Check NULL monitor pointer when injecting MCE Tao Su
2024-03-20 6:02 ` Markus Armbruster
2024-03-20 7:17 ` Philippe Mathieu-Daudé
2024-03-20 7:53 ` Tao Su
2024-03-20 7:44 ` Tao Su
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).