[RFC] cpus: Initialize current_cpu with the first vCPU created
diff mbox series

Message ID 20200701182100.26930-1-philmd@redhat.com
State New
Headers show
Series
  • [RFC] cpus: Initialize current_cpu with the first vCPU created
Related show

Commit Message

Philippe Mathieu-Daudé July 1, 2020, 6:21 p.m. UTC
We can run I/O access with the 'i' or 'o' HMP commands in the
monitor. These commands are expected to run on a vCPU. The
monitor is not a vCPU thread. To avoid crashing, initialize
the 'current_cpu' variable with the first vCPU created. The
command executed on the monitor will end using it.

This fixes:

  $ cat << EOF| qemu-system-i386 -M q35 -nographic -serial none -monitor stdio
  o/4 0xcf8 0x8400f841
  o/4 0xcfc 0xaa215d6d
  o/4 0x6d30 0x2ef8ffbe
  o/1 0xb2 0x20
  EOF
  Segmentation fault (core dumped)

  Thread 1 "qemu-system-i38" received signal SIGSEGV, Segmentation fault.
  0x00005555558946c7 in tcg_handle_interrupt (cpu=0x0, mask=64) at accel/tcg/tcg-all.c:57
  57          old_mask = cpu->interrupt_request;
  (gdb) bt
  #0  0x00005555558946c7 in tcg_handle_interrupt (cpu=0x0, mask=64) at accel/tcg/tcg-all.c:57
  #1  0x00005555558ed7d2 in cpu_interrupt (cpu=0x0, mask=64) at include/hw/core/cpu.h:877
  #2  0x00005555558ee776 in ich9_apm_ctrl_changed (val=32, arg=0x555556e2ff50) at hw/isa/lpc_ich9.c:442
  #3  0x0000555555b47f96 in apm_ioport_writeb (opaque=0x555556e308c0, addr=0, val=32, size=1) at hw/isa/apm.c:44
  #4  0x0000555555879931 in memory_region_write_accessor (mr=0x555556e308e0, addr=0, value=0x7fffffffb9f8, size=1, shift=0, mask=255, attrs=...) at memory.c:483
  #5  0x0000555555879b5a in access_with_adjusted_size (addr=0, value=0x7fffffffb9f8, size=4, access_size_min=1, access_size_max=1, access_fn=
      0x55555587984e <memory_region_write_accessor>, mr=0x555556e308e0, attrs=...) at memory.c:544
  #6  0x000055555587ca32 in memory_region_dispatch_write (mr=0x555556e308e0, addr=0, data=32, op=MO_32, attrs=...) at memory.c:1465
  #7  0x000055555581b7e9 in flatview_write_continue (fv=0x55555698a790, addr=178, attrs=..., ptr=0x7fffffffbb84, len=4, addr1=0, l=4, mr=0x555556e308e0) at exec.c:3198
  #8  0x000055555581b92e in flatview_write (fv=0x55555698a790, addr=178, attrs=..., buf=0x7fffffffbb84, len=4) at exec.c:3238
  #9  0x000055555581bc81 in address_space_write (as=0x555556441220 <address_space_io>, addr=178, attrs=..., buf=0x7fffffffbb84, len=4) at exec.c:3329
  #10 0x0000555555873f08 in cpu_outl (addr=178, val=32) at ioport.c:80
  #11 0x000055555598a26a in hmp_ioport_write (mon=0x5555567621b0, qdict=0x555557702600) at monitor/misc.c:937
  #12 0x0000555555c9c5a5 in handle_hmp_command (mon=0x5555567621b0, cmdline=0x55555676aae1 "/1 0xb2 0x20") at monitor/hmp.c:1082
  #13 0x0000555555c99e02 in monitor_command_cb (opaque=0x5555567621b0, cmdline=0x55555676aae0 "o/1 0xb2 0x20", readline_opaque=0x0) at monitor/hmp.c:47
                            ^
    HMP command from monitor

Reported-by: Alexander Bulekov <alxndr@bu.edu>
Buglink: https://bugs.launchpad.net/qemu/+bug/1878645
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
Cc: Bug 1878645 <1878645@bugs.launchpad.net>

RFC because I believe the correct fix is to NOT use current_cpu
out of cpus.c, at least use qemu_get_cpu(0) to get the first vCPU.
---
 cpus.c | 3 +++
 1 file changed, 3 insertions(+)

Comments

Alexander Bulekov July 1, 2020, 6:54 p.m. UTC | #1
On 200701 2021, Philippe Mathieu-Daudé wrote:
> We can run I/O access with the 'i' or 'o' HMP commands in the
> monitor. These commands are expected to run on a vCPU. The
> monitor is not a vCPU thread. To avoid crashing, initialize
> the 'current_cpu' variable with the first vCPU created. The
> command executed on the monitor will end using it.
> 
> This fixes:
> 
>   $ cat << EOF| qemu-system-i386 -M q35 -nographic -serial none -monitor stdio
>   o/4 0xcf8 0x8400f841
>   o/4 0xcfc 0xaa215d6d
>   o/4 0x6d30 0x2ef8ffbe
>   o/1 0xb2 0x20
>   EOF
>   Segmentation fault (core dumped)
> 
>   Thread 1 "qemu-system-i38" received signal SIGSEGV, Segmentation fault.
>   0x00005555558946c7 in tcg_handle_interrupt (cpu=0x0, mask=64) at accel/tcg/tcg-all.c:57
>   57          old_mask = cpu->interrupt_request;
>   (gdb) bt
>   #0  0x00005555558946c7 in tcg_handle_interrupt (cpu=0x0, mask=64) at accel/tcg/tcg-all.c:57
>   #1  0x00005555558ed7d2 in cpu_interrupt (cpu=0x0, mask=64) at include/hw/core/cpu.h:877
>   #2  0x00005555558ee776 in ich9_apm_ctrl_changed (val=32, arg=0x555556e2ff50) at hw/isa/lpc_ich9.c:442
>   #3  0x0000555555b47f96 in apm_ioport_writeb (opaque=0x555556e308c0, addr=0, val=32, size=1) at hw/isa/apm.c:44
>   #4  0x0000555555879931 in memory_region_write_accessor (mr=0x555556e308e0, addr=0, value=0x7fffffffb9f8, size=1, shift=0, mask=255, attrs=...) at memory.c:483
>   #5  0x0000555555879b5a in access_with_adjusted_size (addr=0, value=0x7fffffffb9f8, size=4, access_size_min=1, access_size_max=1, access_fn=
>       0x55555587984e <memory_region_write_accessor>, mr=0x555556e308e0, attrs=...) at memory.c:544
>   #6  0x000055555587ca32 in memory_region_dispatch_write (mr=0x555556e308e0, addr=0, data=32, op=MO_32, attrs=...) at memory.c:1465
>   #7  0x000055555581b7e9 in flatview_write_continue (fv=0x55555698a790, addr=178, attrs=..., ptr=0x7fffffffbb84, len=4, addr1=0, l=4, mr=0x555556e308e0) at exec.c:3198
>   #8  0x000055555581b92e in flatview_write (fv=0x55555698a790, addr=178, attrs=..., buf=0x7fffffffbb84, len=4) at exec.c:3238
>   #9  0x000055555581bc81 in address_space_write (as=0x555556441220 <address_space_io>, addr=178, attrs=..., buf=0x7fffffffbb84, len=4) at exec.c:3329
>   #10 0x0000555555873f08 in cpu_outl (addr=178, val=32) at ioport.c:80
>   #11 0x000055555598a26a in hmp_ioport_write (mon=0x5555567621b0, qdict=0x555557702600) at monitor/misc.c:937
>   #12 0x0000555555c9c5a5 in handle_hmp_command (mon=0x5555567621b0, cmdline=0x55555676aae1 "/1 0xb2 0x20") at monitor/hmp.c:1082
>   #13 0x0000555555c99e02 in monitor_command_cb (opaque=0x5555567621b0, cmdline=0x55555676aae0 "o/1 0xb2 0x20", readline_opaque=0x0) at monitor/hmp.c:47
>                             ^
>     HMP command from monitor
> 
> Reported-by: Alexander Bulekov <alxndr@bu.edu>
> Buglink: https://bugs.launchpad.net/qemu/+bug/1878645
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
> Cc: Bug 1878645 <1878645@bugs.launchpad.net>
> 
> RFC because I believe the correct fix is to NOT use current_cpu
> out of cpus.c, at least use qemu_get_cpu(0) to get the first vCPU.
> ---
>  cpus.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/cpus.c b/cpus.c
> index 41d1c5099f..1f6f43d221 100644
> --- a/cpus.c
> +++ b/cpus.c
> @@ -2106,6 +2106,9 @@ void qemu_init_vcpu(CPUState *cpu)
>  {
>      MachineState *ms = MACHINE(qdev_get_machine());
>  
> +    if (!current_cpu) {
> +        current_cpu = cpu;
> +    }

Seems like a neat solution.
is it fair to assume that qemu_init_vcpu is called before any threads
that can do I/O are created? I confirmed that the qtest and hmp crashes
are avoided.
-Alex

>      cpu->nr_cores = ms->smp.cores;
>      cpu->nr_threads =  ms->smp.threads;
>      cpu->stopped = true;
> -- 
> 2.21.3
>
Peter Maydell July 1, 2020, 8:35 p.m. UTC | #2
On Wed, 1 Jul 2020 at 19:21, Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
>
> We can run I/O access with the 'i' or 'o' HMP commands in the
> monitor. These commands are expected to run on a vCPU. The
> monitor is not a vCPU thread. To avoid crashing, initialize
> the 'current_cpu' variable with the first vCPU created. The
> command executed on the monitor will end using it.

>
> RFC because I believe the correct fix is to NOT use current_cpu
> out of cpus.c, at least use qemu_get_cpu(0) to get the first vCPU.

Yes, I agree -- I don't think this is the correct fix.
current_cpu is documented as "only valid inside cpu_exec()",
ie if you're actually on a vcpu thread you can use it, but if
you're not on a CPU thread, like the monitor, you need to
say which vCPU you want to affect. For the monitor, that
would be the current "default cpu" as set by the "cpu"
command (cf monitor_set_cpu()). The bug here will be that
somewhere along the line we are probably missing plumbing
sufficient to pass down "which CPU do we want".

https://bugs.launchpad.net/qemu/+bug/1602247 is a bug of
a similar nature -- if the gdbstub does a memory access
we know which CPU we're trying to do that memory access as,
but it doesn't get plumbed through and so in the Arm GIC
register read/write function that looks at current_cpu
we get a segfault.

One way to fix this would be to do something akin to how
real hardware works -- encode into the MemTxAttrs an
indication of what the transaction master was (eg the
CPU number for CPUs); then the handful of devices that
care about which CPU was doing the transaction can use
that, rather than directly looking at current_cpu, and
when gdbstub or monitor perform an access that should
act like it's from a particular CPU they can indicate
that by supplying appropriate transaction attributes.
That would potentially be quite a bit of work though
(but I think it would be a neat design if we want to
try to fix this kind of "segfault if the user prods
a device via the monitor" bug).

+    if (!current_cpu) {
+        current_cpu = cpu;
+    }

Some more specific issues with this -- current_cpu is
a thread-local variable, so this will set that for
whatever thread happens to make this call, which
might or might not be the one that ends up handling
the monitor command. Also some code assumes that
current_cpu is non-NULL only if this is a vCPU thread,
eg sigbus_handler().

thanks
-- PMM
Philippe Mathieu-Daudé July 2, 2020, 7:55 a.m. UTC | #3
On 7/1/20 10:35 PM, Peter Maydell wrote:
> On Wed, 1 Jul 2020 at 19:21, Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
>>
>> We can run I/O access with the 'i' or 'o' HMP commands in the
>> monitor. These commands are expected to run on a vCPU. The
>> monitor is not a vCPU thread. To avoid crashing, initialize
>> the 'current_cpu' variable with the first vCPU created. The
>> command executed on the monitor will end using it.
> 
>>
>> RFC because I believe the correct fix is to NOT use current_cpu
>> out of cpus.c, at least use qemu_get_cpu(0) to get the first vCPU.
> 
> Yes, I agree -- I don't think this is the correct fix.
> current_cpu is documented as "only valid inside cpu_exec()",
> ie if you're actually on a vcpu thread you can use it, but if
> you're not on a CPU thread, like the monitor, you need to
> say which vCPU you want to affect. For the monitor, that
> would be the current "default cpu" as set by the "cpu"
> command (cf monitor_set_cpu()). The bug here will be that
> somewhere along the line we are probably missing plumbing
> sufficient to pass down "which CPU do we want".

TIL mon_get_cpu() :)

This is a bit different here, the monitor is not doing an
access on a CPU address space, but directly on the I/O
address space. The APM port is registered by the ICH9
south bridge, and this triggers a SMI exception on a
CPU core, but I have no idea which one, a random one?
Then this particular core will switch to SMM mode.

Another way of seeing this problem, is imagining we
start a PIT timer from the monitor. When the timer
expires, the PIT will interrupt the CPU. Which CPU
to interrupt? We are not in the context of the monitor.

> https://bugs.launchpad.net/qemu/+bug/1602247 is a bug of
> a similar nature -- if the gdbstub does a memory access
> we know which CPU we're trying to do that memory access as,
> but it doesn't get plumbed through and so in the Arm GIC
> register read/write function that looks at current_cpu
> we get a segfault.
> 
> One way to fix this would be to do something akin to how
> real hardware works -- encode into the MemTxAttrs an
> indication of what the transaction master was (eg the
> CPU number for CPUs); then the handful of devices that
> care about which CPU was doing the transaction can use
> that, rather than directly looking at current_cpu, and
> when gdbstub or monitor perform an access that should
> act like it's from a particular CPU they can indicate
> that by supplying appropriate transaction attributes.
> That would potentially be quite a bit of work though
> (but I think it would be a neat design if we want to
> try to fix this kind of "segfault if the user prods
> a device via the monitor" bug).

This is complex stuff. Too early for me to digest, I am
keeping this for later (I am not ignoring your comment).

> 
> +    if (!current_cpu) {
> +        current_cpu = cpu;
> +    }
> 
> Some more specific issues with this -- current_cpu is
> a thread-local variable, so this will set that for
> whatever thread happens to make this call, which
> might or might not be the one that ends up handling
> the monitor command. Also some code assumes that
> current_cpu is non-NULL only if this is a vCPU thread,
> eg sigbus_handler().

Yes, I agree.

> 
> thanks
> -- PMM
>
Paolo Bonzini July 2, 2020, 9:52 a.m. UTC | #4
On 01/07/20 22:35, Peter Maydell wrote:
> For the monitor, that
> would be the current "default cpu" as set by the "cpu"
> command (cf monitor_set_cpu()). The bug here will be that
> somewhere along the line we are probably missing plumbing
> sufficient to pass down "which CPU do we want".

Yeah, the fix is probably to add a functions that returns either
current_cpu or the monitor CPU, and use it in device emulation if
applicable.

The problem with current_cpu is that it affects stuff like run_on_cpu,
and that is guaranteed to cause havoc to code that expects to run on a
given CPU and therefore doesn't use locks.

Paolo
Alex Bennée July 2, 2020, 10:49 a.m. UTC | #5
Paolo Bonzini <pbonzini@redhat.com> writes:

> On 01/07/20 22:35, Peter Maydell wrote:
>> For the monitor, that
>> would be the current "default cpu" as set by the "cpu"
>> command (cf monitor_set_cpu()). The bug here will be that
>> somewhere along the line we are probably missing plumbing
>> sufficient to pass down "which CPU do we want".
>
> Yeah, the fix is probably to add a functions that returns either
> current_cpu or the monitor CPU, and use it in device emulation if
> applicable.
>
> The problem with current_cpu is that it affects stuff like run_on_cpu,
> and that is guaranteed to cause havoc to code that expects to run on a
> given CPU and therefore doesn't use locks.

IIRC the original reported bug was in a APM callback which was triggered
by an MMIO operation. Currently we don't expose the current cpu via the
memory dispatch routines. Should we to ensure there is always something
valid there?

>
> Paolo

Patch
diff mbox series

diff --git a/cpus.c b/cpus.c
index 41d1c5099f..1f6f43d221 100644
--- a/cpus.c
+++ b/cpus.c
@@ -2106,6 +2106,9 @@  void qemu_init_vcpu(CPUState *cpu)
 {
     MachineState *ms = MACHINE(qdev_get_machine());
 
+    if (!current_cpu) {
+        current_cpu = cpu;
+    }
     cpu->nr_cores = ms->smp.cores;
     cpu->nr_threads =  ms->smp.threads;
     cpu->stopped = true;