Patchwork virtio-serial: Fix segfault on guest boot

login
register
mail settings
Submitter Luiz Capitulino
Date June 16, 2011, 4:38 p.m.
Message ID <20110616133849.02fb4bea@doriath>
Download mbox | patch
Permalink /patch/100689/
State New
Headers show

Comments

Luiz Capitulino - June 16, 2011, 4:38 p.m.
If I start qemu with:

 # qemu -hda disks/test.img -enable-kvm -m 1G -snapshot \
        -device virtio-serial \
        -chardev socket,host=localhost,port=1234,server,nowait,id=foo \
        -device virtserialport,chardev=foo,name=org.qemu.guest_agent

I get a segfault when booting a Fedora 14 guest. The backtrace says:

  Program terminated with signal 11, Segmentation fault.
  #0  0x0000000000420850 in handle_control_message (vser=0x3732bd0, buf=0x2c173e0, len=8) at /home/lcapitulino/src/qmp-unstable/hw/virtio-serial-bus.c:335
  335     info = DO_UPCAST(VirtIOSerialPortInfo, qdev, port->dev.info);

I've also bisected this and git points out to commit:

  commit a15bb0d6a981de749452a5180fc8084d625671da
  Author: Markus Armbruster <armbru@redhat.com>
  Date:   Wed May 25 14:21:13 2011 +0200

      virtio-serial: Drop redundant VirtIOSerialPort member info

I think what's happening is that the device is not initialized on a
VIRTIO_CONSOLE_DEVICE_READY event. Moving the DO_UPCAST() call to
the other events fixes the problem to me.

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 hw/virtio-serial-bus.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)
Amit Shah - June 17, 2011, 6:47 a.m.
On (Thu) 16 Jun 2011 [13:38:49], Luiz Capitulino wrote:
> If I start qemu with:
> 
>  # qemu -hda disks/test.img -enable-kvm -m 1G -snapshot \
>         -device virtio-serial \
>         -chardev socket,host=localhost,port=1234,server,nowait,id=foo \
>         -device virtserialport,chardev=foo,name=org.qemu.guest_agent
> 
> I get a segfault when booting a Fedora 14 guest. The backtrace says:
>
>   Program terminated with signal 11, Segmentation fault.
>   #0  0x0000000000420850 in handle_control_message (vser=0x3732bd0, buf=0x2c173e0, len=8) at /home/lcapitulino/src/qmp-unstable/hw/virtio-serial-bus.c:335
>   335     info = DO_UPCAST(VirtIOSerialPortInfo, qdev, port->dev.info);

Strange, I've not seen it so far in my testing (neither in the daily
test runs of the virtio-serial testsuite).

> I've also bisected this and git points out to commit:
> 
>   commit a15bb0d6a981de749452a5180fc8084d625671da
>   Author: Markus Armbruster <armbru@redhat.com>
>   Date:   Wed May 25 14:21:13 2011 +0200
> 
>       virtio-serial: Drop redundant VirtIOSerialPort member info
> 
> I think what's happening is that the device is not initialized on a
> VIRTIO_CONSOLE_DEVICE_READY event. Moving the DO_UPCAST() call to
> the other events fixes the problem to me.
> 
> Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
> ---
>  hw/virtio-serial-bus.c |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/virtio-serial-bus.c b/hw/virtio-serial-bus.c
> index 9a12104..579f676 100644
> --- a/hw/virtio-serial-bus.c
> +++ b/hw/virtio-serial-bus.c
> @@ -332,8 +332,6 @@ static void handle_control_message(VirtIOSerial *vser, void *buf, size_t len)
>      if (!port && cpkt.event != VIRTIO_CONSOLE_DEVICE_READY)
>          return;
>  
> -    info = DO_UPCAST(VirtIOSerialPortInfo, qdev, port->dev.info);
> -

Ah - this missed the !port check.  It should be possible to do this in
a 'if (port)' block instead of replicating in the individual case
statements.

Thanks for the debugging and patch; please update with the above and
I'll apply it to the virtio-serial tree.

		Amit
Markus Armbruster - June 17, 2011, 7:47 a.m.
Amit Shah <amit.shah@redhat.com> writes:

> On (Thu) 16 Jun 2011 [13:38:49], Luiz Capitulino wrote:
>> If I start qemu with:
>> 
>>  # qemu -hda disks/test.img -enable-kvm -m 1G -snapshot \
>>         -device virtio-serial \
>>         -chardev socket,host=localhost,port=1234,server,nowait,id=foo \
>>         -device virtserialport,chardev=foo,name=org.qemu.guest_agent
>> 
>> I get a segfault when booting a Fedora 14 guest. The backtrace says:
>>
>>   Program terminated with signal 11, Segmentation fault.
>>   #0  0x0000000000420850 in handle_control_message (vser=0x3732bd0, buf=0x2c173e0, len=8) at /home/lcapitulino/src/qmp-unstable/hw/virtio-serial-bus.c:335
>>   335     info = DO_UPCAST(VirtIOSerialPortInfo, qdev, port->dev.info);
>
> Strange, I've not seen it so far in my testing (neither in the daily
> test runs of the virtio-serial testsuite).
>
>> I've also bisected this and git points out to commit:
>> 
>>   commit a15bb0d6a981de749452a5180fc8084d625671da
>>   Author: Markus Armbruster <armbru@redhat.com>
>>   Date:   Wed May 25 14:21:13 2011 +0200
>> 
>>       virtio-serial: Drop redundant VirtIOSerialPort member info
>> 
>> I think what's happening is that the device is not initialized on a
>> VIRTIO_CONSOLE_DEVICE_READY event.

Really?

I believe the device is initialized just fine, but the message refers to
a nonexistant port.  The check after find_port_by_id() suggests that's
expected for VIRTIO_CONSOLE_DEVICE_READY.

>>                                    Moving the DO_UPCAST() call to
>> the other events fixes the problem to me.
>> 
>> Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
>> ---
>>  hw/virtio-serial-bus.c |    4 ++--
>>  1 files changed, 2 insertions(+), 2 deletions(-)
>> 
>> diff --git a/hw/virtio-serial-bus.c b/hw/virtio-serial-bus.c
>> index 9a12104..579f676 100644
>> --- a/hw/virtio-serial-bus.c
>> +++ b/hw/virtio-serial-bus.c
>> @@ -332,8 +332,6 @@ static void handle_control_message(VirtIOSerial *vser, void *buf, size_t len)
>>      if (!port && cpkt.event != VIRTIO_CONSOLE_DEVICE_READY)
>>          return;
>>  
>> -    info = DO_UPCAST(VirtIOSerialPortInfo, qdev, port->dev.info);
>> -
>
> Ah - this missed the !port check.  It should be possible to do this in
> a 'if (port)' block instead of replicating in the individual case
> statements.

You might have to do something like info = port ? DO_UPCAST(...) : NULL
to avoid warnings about info used uninitialized.

[...]
Amit Shah - June 17, 2011, 8:44 a.m.
On (Fri) 17 Jun 2011 [09:47:31], Markus Armbruster wrote:
> Amit Shah <amit.shah@redhat.com> writes:
> 
> > On (Thu) 16 Jun 2011 [13:38:49], Luiz Capitulino wrote:
> >> If I start qemu with:
> >> 
> >>  # qemu -hda disks/test.img -enable-kvm -m 1G -snapshot \
> >>         -device virtio-serial \
> >>         -chardev socket,host=localhost,port=1234,server,nowait,id=foo \
> >>         -device virtserialport,chardev=foo,name=org.qemu.guest_agent
> >> 
> >> I get a segfault when booting a Fedora 14 guest. The backtrace says:
> >>
> >>   Program terminated with signal 11, Segmentation fault.
> >>   #0  0x0000000000420850 in handle_control_message (vser=0x3732bd0, buf=0x2c173e0, len=8) at /home/lcapitulino/src/qmp-unstable/hw/virtio-serial-bus.c:335
> >>   335     info = DO_UPCAST(VirtIOSerialPortInfo, qdev, port->dev.info);
> >
> > Strange, I've not seen it so far in my testing (neither in the daily
> > test runs of the virtio-serial testsuite).
> >
> >> I've also bisected this and git points out to commit:
> >> 
> >>   commit a15bb0d6a981de749452a5180fc8084d625671da
> >>   Author: Markus Armbruster <armbru@redhat.com>
> >>   Date:   Wed May 25 14:21:13 2011 +0200
> >> 
> >>       virtio-serial: Drop redundant VirtIOSerialPort member info
> >> 
> >> I think what's happening is that the device is not initialized on a
> >> VIRTIO_CONSOLE_DEVICE_READY event.
> 
> Really?
> 
> I believe the device is initialized just fine, but the message refers to
> a nonexistant port.  The check after find_port_by_id() suggests that's
> expected for VIRTIO_CONSOLE_DEVICE_READY.

What's happening is VIRTIO_CONSOLE_DEVICE_READY is a message for the
whole device, not for an individual port.  So port is NULL.  All other
messages (currently) are port-specific, so they have 'port' set to
something.  The commit message needs to be tweaked for this.

> 
> >>                                    Moving the DO_UPCAST() call to
> >> the other events fixes the problem to me.
> >> 
> >> Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
> >> ---
> >>  hw/virtio-serial-bus.c |    4 ++--
> >>  1 files changed, 2 insertions(+), 2 deletions(-)
> >> 
> >> diff --git a/hw/virtio-serial-bus.c b/hw/virtio-serial-bus.c
> >> index 9a12104..579f676 100644
> >> --- a/hw/virtio-serial-bus.c
> >> +++ b/hw/virtio-serial-bus.c
> >> @@ -332,8 +332,6 @@ static void handle_control_message(VirtIOSerial *vser, void *buf, size_t len)
> >>      if (!port && cpkt.event != VIRTIO_CONSOLE_DEVICE_READY)
> >>          return;
> >>  
> >> -    info = DO_UPCAST(VirtIOSerialPortInfo, qdev, port->dev.info);
> >> -
> >
> > Ah - this missed the !port check.  It should be possible to do this in
> > a 'if (port)' block instead of replicating in the individual case
> > statements.
> 
> You might have to do something like info = port ? DO_UPCAST(...) : NULL
> to avoid warnings about info used uninitialized.

That won't happen -- in the case where port is NULL, we shouldn't try
to use info.  If we do get such a warning, it means there's a bug :-)


		Amit

Patch

diff --git a/hw/virtio-serial-bus.c b/hw/virtio-serial-bus.c
index 9a12104..579f676 100644
--- a/hw/virtio-serial-bus.c
+++ b/hw/virtio-serial-bus.c
@@ -332,8 +332,6 @@  static void handle_control_message(VirtIOSerial *vser, void *buf, size_t len)
     if (!port && cpkt.event != VIRTIO_CONSOLE_DEVICE_READY)
         return;
 
-    info = DO_UPCAST(VirtIOSerialPortInfo, qdev, port->dev.info);
-
     switch(cpkt.event) {
     case VIRTIO_CONSOLE_DEVICE_READY:
         if (!cpkt.value) {
@@ -363,6 +361,7 @@  static void handle_control_message(VirtIOSerial *vser, void *buf, size_t len)
          * this port is a console port so that the guest can hook it
          * up to hvc.
          */
+        info = DO_UPCAST(VirtIOSerialPortInfo, qdev, port->dev.info);
         if (info->is_console) {
             send_control_event(port, VIRTIO_CONSOLE_CONSOLE_PORT, 1);
         }
@@ -398,6 +397,7 @@  static void handle_control_message(VirtIOSerial *vser, void *buf, size_t len)
         break;
 
     case VIRTIO_CONSOLE_PORT_OPEN:
+        info = DO_UPCAST(VirtIOSerialPortInfo, qdev, port->dev.info);
         port->guest_connected = cpkt.value;
         if (cpkt.value && info->guest_open) {
             /* Send the guest opened notification if an app is interested */