| Message ID | 20110617101644.5e275e53@doriath |
|---|---|
| State | New |
| Headers | show |
On Fri, 17 Jun 2011 10:16:44 -0300 Luiz Capitulino <lcapitulino@redhat.com> wrote: > On Fri, 17 Jun 2011 12:17:36 +0530 > Amit Shah <amit.shah@redhat.com> wrote: > > > 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. > > What about moving the VIRTIO_CONSOLE_DEVICE_READY handling out of the > switch, like the patch below? This way the function is divided in a way > that related events are handled together. > > I'll implement your first suggestion if you don't like this... > > diff --git a/hw/virtio-serial-bus.c b/hw/virtio-serial-bus.c > index 579f676..5f96245 100644 > --- a/hw/virtio-serial-bus.c > +++ b/hw/virtio-serial-bus.c > @@ -325,19 +325,12 @@ static void handle_control_message(VirtIOSerial *vser, void *buf, size_t len) > return; > } > > - cpkt.event = lduw_p(&gcpkt->event); > cpkt.value = lduw_p(&gcpkt->value); > - > - port = find_port_by_id(vser, ldl_p(&gcpkt->id)); > - if (!port && cpkt.event != VIRTIO_CONSOLE_DEVICE_READY) > - return; > - > - switch(cpkt.event) { > - case VIRTIO_CONSOLE_DEVICE_READY: > + cpkt.event = lduw_p(&gcpkt->event); > + if (cpkt.event == VIRTIO_CONSOLE_DEVICE_READY) { > if (!cpkt.value) { > - error_report("virtio-serial-bus: Guest failure in adding device %s\n", > - vser->bus.qbus.name); > - break; > + error_report("virtio-serial-bus: Guest failure in adding device %s\n", vser->bus.qbus.name); > + return; > } > /* > * The device is up, we can now tell the device about all the > @@ -346,8 +339,13 @@ static void handle_control_message(VirtIOSerial *vser, void *buf, size_t len) > QTAILQ_FOREACH(port, &vser->ports, next) { > send_control_event(port, VIRTIO_CONSOLE_PORT_ADD, 1); > } > - break; > + return; > + } > > + port = find_port_by_id(vser, ldl_p(&gcpkt->id)); > + assert(port != NULL); I did this on top of my first patch and forgot to move the DO_UPCAST() call here... But you got the idea :) > + > + switch(cpkt.event) { > case VIRTIO_CONSOLE_PORT_READY: > if (!cpkt.value) { > error_report("virtio-serial-bus: Guest failure in adding port %u for device %s\n",
On (Fri) 17 Jun 2011 [10:16:44], Luiz Capitulino wrote: > On Fri, 17 Jun 2011 12:17:36 +0530 > Amit Shah <amit.shah@redhat.com> wrote: > > > 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. > > What about moving the VIRTIO_CONSOLE_DEVICE_READY handling out of the > switch, like the patch below? This way the function is divided in a way > that related events are handled together. > > I'll implement your first suggestion if you don't like this... > > diff --git a/hw/virtio-serial-bus.c b/hw/virtio-serial-bus.c > index 579f676..5f96245 100644 > --- a/hw/virtio-serial-bus.c > +++ b/hw/virtio-serial-bus.c > @@ -325,19 +325,12 @@ static void handle_control_message(VirtIOSerial *vser, void *buf, size_t len) > return; > } > > - cpkt.event = lduw_p(&gcpkt->event); > cpkt.value = lduw_p(&gcpkt->value); > - > - port = find_port_by_id(vser, ldl_p(&gcpkt->id)); > - if (!port && cpkt.event != VIRTIO_CONSOLE_DEVICE_READY) > - return; > - > - switch(cpkt.event) { > - case VIRTIO_CONSOLE_DEVICE_READY: > + cpkt.event = lduw_p(&gcpkt->event); > + if (cpkt.event == VIRTIO_CONSOLE_DEVICE_READY) { > if (!cpkt.value) { > - error_report("virtio-serial-bus: Guest failure in adding device %s\n", > - vser->bus.qbus.name); > - break; > + error_report("virtio-serial-bus: Guest failure in adding device %s\n", vser->bus.qbus.name); > + return; The line split should remain -- else it goes beyond 80 chars. > } > /* > * The device is up, we can now tell the device about all the > @@ -346,8 +339,13 @@ static void handle_control_message(VirtIOSerial *vser, void *buf, size_t len) > QTAILQ_FOREACH(port, &vser->ports, next) { > send_control_event(port, VIRTIO_CONSOLE_PORT_ADD, 1); > } > - break; > + return; > + } Makes me think of one case (totally unrelated to what you found)where the guest can fool us: by sending multiple VIRTIO_CONSOLE_DEVICE_READY messages. > + port = find_port_by_id(vser, ldl_p(&gcpkt->id)); > + assert(port != NULL); I doubt if assert is the right thing: if the guest sends bad data, we shouldn't just kill it. It's easier to ignore such data, and perhaps just log it. > + > + switch(cpkt.event) { > case VIRTIO_CONSOLE_PORT_READY: > if (!cpkt.value) { > error_report("virtio-serial-bus: Guest failure in adding port %u for device %s\n", I'm fine with this approach. Amit
On Fri, 17 Jun 2011 21:39:26 +0530 Amit Shah <amit.shah@redhat.com> wrote: > On (Fri) 17 Jun 2011 [10:16:44], Luiz Capitulino wrote: > > On Fri, 17 Jun 2011 12:17:36 +0530 > > Amit Shah <amit.shah@redhat.com> wrote: > > > > > 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. > > > > What about moving the VIRTIO_CONSOLE_DEVICE_READY handling out of the > > switch, like the patch below? This way the function is divided in a way > > that related events are handled together. > > > > I'll implement your first suggestion if you don't like this... > > > > diff --git a/hw/virtio-serial-bus.c b/hw/virtio-serial-bus.c > > index 579f676..5f96245 100644 > > --- a/hw/virtio-serial-bus.c > > +++ b/hw/virtio-serial-bus.c > > @@ -325,19 +325,12 @@ static void handle_control_message(VirtIOSerial *vser, void *buf, size_t len) > > return; > > } > > > > - cpkt.event = lduw_p(&gcpkt->event); > > cpkt.value = lduw_p(&gcpkt->value); > > - > > - port = find_port_by_id(vser, ldl_p(&gcpkt->id)); > > - if (!port && cpkt.event != VIRTIO_CONSOLE_DEVICE_READY) > > - return; > > - > > - switch(cpkt.event) { > > - case VIRTIO_CONSOLE_DEVICE_READY: > > + cpkt.event = lduw_p(&gcpkt->event); > > + if (cpkt.event == VIRTIO_CONSOLE_DEVICE_READY) { > > if (!cpkt.value) { > > - error_report("virtio-serial-bus: Guest failure in adding device %s\n", > > - vser->bus.qbus.name); > > - break; > > + error_report("virtio-serial-bus: Guest failure in adding device %s\n", vser->bus.qbus.name); > > + return; > > The line split should remain -- else it goes beyond 80 chars. It's already beyond 80 chars to me. > > > } > > /* > > * The device is up, we can now tell the device about all the > > @@ -346,8 +339,13 @@ static void handle_control_message(VirtIOSerial *vser, void *buf, size_t len) > > QTAILQ_FOREACH(port, &vser->ports, next) { > > send_control_event(port, VIRTIO_CONSOLE_PORT_ADD, 1); > > } > > - break; > > + return; > > + } > > Makes me think of one case (totally unrelated to what you found)where > the guest can fool us: by sending multiple VIRTIO_CONSOLE_DEVICE_READY > messages. It will be handled just fine, no? > > > + port = find_port_by_id(vser, ldl_p(&gcpkt->id)); > > + assert(port != NULL); > > I doubt if assert is the right thing: if the guest sends bad data, we > shouldn't just kill it. It's easier to ignore such data, and perhaps > just log it. Right. > > > + > > + switch(cpkt.event) { > > case VIRTIO_CONSOLE_PORT_READY: > > if (!cpkt.value) { > > error_report("virtio-serial-bus: Guest failure in adding port %u for device %s\n", > > I'm fine with this approach. > > Amit >
On (Fri) 17 Jun 2011 [15:08:11], Luiz Capitulino wrote: > > > if (!cpkt.value) { > > > - error_report("virtio-serial-bus: Guest failure in adding device %s\n", > > > - vser->bus.qbus.name); > > > - break; > > > + error_report("virtio-serial-bus: Guest failure in adding device %s\n", vser->bus.qbus.name); > > > + return; > > > > The line split should remain -- else it goes beyond 80 chars. > > It's already beyond 80 chars to me. I prefer to not break strings that get printed out -- makes it easier for greppers to find out the source of the string. > > > @@ -346,8 +339,13 @@ static void handle_control_message(VirtIOSerial *vser, void *buf, size_t len) > > > QTAILQ_FOREACH(port, &vser->ports, next) { > > > send_control_event(port, VIRTIO_CONSOLE_PORT_ADD, 1); > > > } > > > - break; > > > + return; > > > + } > > > > Makes me think of one case (totally unrelated to what you found)where > > the guest can fool us: by sending multiple VIRTIO_CONSOLE_DEVICE_READY > > messages. > > It will be handled just fine, no? We'll send out the VIRTIO_CONSOLE_PORT_ADD events for each port (again). That's the case now. No idea how the code might change in the future and we could end up doing something else in addition which might be bad. Anyway, all this is for a buggy or a bad guest. Amit
On Sat, Jun 18, 2011 at 6:42 AM, Amit Shah <amit.shah@redhat.com> wrote: > On (Fri) 17 Jun 2011 [15:08:11], Luiz Capitulino wrote: > >> > > if (!cpkt.value) { >> > > - error_report("virtio-serial-bus: Guest failure in adding device %s\n", >> > > - vser->bus.qbus.name); >> > > - break; >> > > + error_report("virtio-serial-bus: Guest failure in adding device %s\n", vser->bus.qbus.name); >> > > + return; >> > >> > The line split should remain -- else it goes beyond 80 chars. >> >> It's already beyond 80 chars to me. > > I prefer to not break strings that get printed out -- makes it easier > for greppers to find out the source of the string. Please read CODING_STYLE and use scripts/checkpatch.pl before submitting patches. >> > > @@ -346,8 +339,13 @@ static void handle_control_message(VirtIOSerial *vser, void *buf, size_t len) >> > > QTAILQ_FOREACH(port, &vser->ports, next) { >> > > send_control_event(port, VIRTIO_CONSOLE_PORT_ADD, 1); >> > > } >> > > - break; >> > > + return; >> > > + } >> > >> > Makes me think of one case (totally unrelated to what you found)where >> > the guest can fool us: by sending multiple VIRTIO_CONSOLE_DEVICE_READY >> > messages. >> >> It will be handled just fine, no? > > We'll send out the VIRTIO_CONSOLE_PORT_ADD events for each port > (again). That's the case now. No idea how the code might change in > the future and we could end up doing something else in addition which > might be bad. Anyway, all this is for a buggy or a bad guest. > > Amit > >
diff --git a/hw/virtio-serial-bus.c b/hw/virtio-serial-bus.c index 579f676..5f96245 100644 --- a/hw/virtio-serial-bus.c +++ b/hw/virtio-serial-bus.c @@ -325,19 +325,12 @@ static void handle_control_message(VirtIOSerial *vser, void *buf, size_t len) return; } - cpkt.event = lduw_p(&gcpkt->event); cpkt.value = lduw_p(&gcpkt->value); - - port = find_port_by_id(vser, ldl_p(&gcpkt->id)); - if (!port && cpkt.event != VIRTIO_CONSOLE_DEVICE_READY) - return; - - switch(cpkt.event) { - case VIRTIO_CONSOLE_DEVICE_READY: + cpkt.event = lduw_p(&gcpkt->event); + if (cpkt.event == VIRTIO_CONSOLE_DEVICE_READY) { if (!cpkt.value) { - error_report("virtio-serial-bus: Guest failure in adding device %s\n", - vser->bus.qbus.name); - break; + error_report("virtio-serial-bus: Guest failure in adding device %s\n", vser->bus.qbus.name); + return; } /* * The device is up, we can now tell the device about all the @@ -346,8 +339,13 @@ static void handle_control_message(VirtIOSerial *vser, void *buf, size_t len) QTAILQ_FOREACH(port, &vser->ports, next) { send_control_event(port, VIRTIO_CONSOLE_PORT_ADD, 1); } - break; + return; + } + port = find_port_by_id(vser, ldl_p(&gcpkt->id)); + assert(port != NULL); + + switch(cpkt.event) { case VIRTIO_CONSOLE_PORT_READY: if (!cpkt.value) { error_report("virtio-serial-bus: Guest failure in adding port %u for device %s\n",