[patch 0/2] pci_register_device can fail

[patch 0/2] pci_register_device can fail

[Chris Wright]

The pci hotadd patches make it easy to trigger segfaults when adding more
devices than a single PCI bus can handle.  The following 2 patches fix the
pci nic devices and virtio-blk device.  Now the following the following:

  OK bus 0, slot 31, function 0 (devfn 248)
  (qemu) pci_add 0 nic model=virtio
  Segmentation fault

  OK bus 0, slot 31, function 0 (devfn 248)
  (qemu) pci_add 0 storage file=/mnt/disk1,if=virtio
  Segmentation fault

become:

  OK bus 0, slot 31, function 0 (devfn 248)
  (qemu) pci_add 0 nic model=virtio
  qemu: Unable to initialze NIC: virtio
  failed to add model=virtio

  OK bus 0, slot 31, function 0 (devfn 248)
  (qemu) pci_add 0 storage file=/mnt/disk1,if=virtio
  failed to add file=/mnt/disk1,if=virtio

thanks,
-chris
-- 

-------------------------------------------------------------------------
This SF.net email is sponsored by the 2008 JavaOne(SM) Conference 
Don't miss this year's exciting event. There's still time to save $100. 
Use priority code J8TL2D2. 
http://ad.doubleclick.net/clk;198757673;13503038;p?http://java.sun.com/javaone

[patch 1/2] [PATCH] pci nic: pci_register_device can fail

[Chris Wright]

[-- Attachment #1: pci-pci_nic_init-can-fail.patch --]
[-- Type: text/plain, Size: 3858 bytes --]

The pci_register_device() call in PCI nic initialization routines can
fail.  Handle this failure and propagate a meaningful error message to
the user instead of generating a SEGV.

Cc: Marcelo Tosatti <mtosatti@redhat.com>
Signed-off-by: Chris Wright <chrisw@sous-sol.org>
---
 qemu/hw/e1000.c      |    3 +++
 qemu/hw/eepro100.c   |    2 ++
 qemu/hw/ne2000.c     |    3 +++
 qemu/hw/pci.c        |    6 ++++++
 qemu/hw/pcnet.c      |    2 ++
 qemu/hw/rtl8139.c    |    3 +++
 qemu/hw/virtio-net.c |    2 ++
 qemu/hw/virtio.c     |    3 +++
 8 files changed, 24 insertions(+)

--- a/qemu/hw/e1000.c
+++ b/qemu/hw/e1000.c
@@ -963,6 +963,9 @@ pci_e1000_init(PCIBus *bus, NICInfo *nd,
     d = (E1000State *)pci_register_device(bus, "e1000",
                 sizeof(E1000State), devfn, NULL, NULL);
 
+    if (!d)
+    	return NULL;
+
     pci_conf = d->dev.config;
     memset(pci_conf, 0, 256);
 
--- a/qemu/hw/eepro100.c
+++ b/qemu/hw/eepro100.c
@@ -1753,6 +1753,8 @@ static PCIDevice *nic_init(PCIBus * bus,
     d = (PCIEEPRO100State *) pci_register_device(bus, name,
                                                  sizeof(PCIEEPRO100State), -1,
                                                  NULL, NULL);
+    if (!d)
+        return NULL;
 
     s = &d->eepro100;
     s->device = device;
--- a/qemu/hw/ne2000.c
+++ b/qemu/hw/ne2000.c
@@ -796,6 +796,9 @@ PCIDevice *pci_ne2000_init(PCIBus *bus, 
                                               "NE2000", sizeof(PCINE2000State),
                                               devfn,
                                               NULL, NULL);
+    if (!d)
+       return NULL;
+
     pci_conf = d->dev.config;
     pci_conf[0x00] = 0xec; // Realtek 8029
     pci_conf[0x01] = 0x10;
--- a/qemu/hw/pci.c
+++ b/qemu/hw/pci.c
@@ -696,6 +696,12 @@ PCIDevice *pci_nic_init(PCIBus *bus, NIC
         fprintf(stderr, "qemu: Unsupported NIC: %s\n", nd->model);
         return NULL;
     }
+
+    if (!pci_dev) {
+        fprintf(stderr, "qemu: Unable to initialze NIC: %s\n", nd->model);
+        return NULL;
+    }
+
     nd->devfn = pci_dev->devfn;
     return pci_dev;
 }
--- a/qemu/hw/pcnet.c
+++ b/qemu/hw/pcnet.c
@@ -1970,6 +1970,8 @@ PCIDevice *pci_pcnet_init(PCIBus *bus, N
 
     d = (PCNetState *)pci_register_device(bus, "PCNet", sizeof(PCNetState),
                                           devfn, NULL, NULL);
+    if (!d)
+    	return NULL;
 
     pci_conf = d->dev.config;
 
--- a/qemu/hw/rtl8139.c
+++ b/qemu/hw/rtl8139.c
@@ -3411,6 +3411,9 @@ PCIDevice *pci_rtl8139_init(PCIBus *bus,
                                               "RTL8139", sizeof(PCIRTL8139State),
                                               devfn,
                                               NULL, NULL);
+    if (!d)
+    	return NULL;
+
     pci_conf = d->dev.config;
     pci_conf[0x00] = 0xec; /* Realtek 8139 */
     pci_conf[0x01] = 0x10;
--- a/qemu/hw/virtio-net.c
+++ b/qemu/hw/virtio-net.c
@@ -292,6 +292,8 @@ PCIDevice *virtio_net_init(PCIBus *bus, 
 				     0, VIRTIO_ID_NET,
 				     0x02, 0x00, 0x00,
 				     6, sizeof(VirtIONet));
+    if (!n)
+    	return NULL;
 
     n->vdev.update_config = virtio_net_update_config;
     n->vdev.get_features = virtio_net_get_features;
--- a/qemu/hw/virtio.c
+++ b/qemu/hw/virtio.c
@@ -408,6 +408,9 @@ VirtIODevice *virtio_init_pci(PCIBus *bu
 
     pci_dev = pci_register_device(bus, name, struct_size,
 				  -1, NULL, NULL);
+    if (!pci_dev)
+    	return NULL;
+
     vdev = to_virtio_device(pci_dev);
 
     vdev->status = 0;

-- 

-------------------------------------------------------------------------
This SF.net email is sponsored by the 2008 JavaOne(SM) Conference 
Don't miss this year's exciting event. There's still time to save $100. 
Use priority code J8TL2D2. 
http://ad.doubleclick.net/clk;198757673;13503038;p?http://java.sun.com/javaone

[patch 2/2] [PATCH] virtio-blk: virtio_pci_init can fail

[Chris Wright]

[-- Attachment #1: virtio-blk-virtio_init_pci-can-fail.patch --]
[-- Type: text/plain, Size: 997 bytes --]

The pci_device_register() call in virtio_pci_init() can fail.
Handle this error condition instead of generating a SEGV.

Cc: Marcelo Tosatti <mtosatti@redhat.com>
Signed-off-by: Chris Wright <chrisw@sous-sol.org>
---
 qemu/hw/virtio-blk.c |    2 ++
 1 file changed, 2 insertions(+)

--- a/qemu/hw/virtio-blk.c
+++ b/qemu/hw/virtio-blk.c
@@ -163,6 +163,8 @@ void *virtio_blk_init(PCIBus *bus, uint1
 				       0, VIRTIO_ID_BLOCK,
 				       0x01, 0x80, 0x00,
 				       sizeof(struct virtio_blk_config), sizeof(VirtIOBlock));
+    if (!s)
+    	return NULL;
 
     s->vdev.update_config = virtio_blk_update_config;
     s->vdev.get_features = virtio_blk_get_features;

-- 

-------------------------------------------------------------------------
This SF.net email is sponsored by the 2008 JavaOne(SM) Conference 
Don't miss this year's exciting event. There's still time to save $100. 
Use priority code J8TL2D2. 
http://ad.doubleclick.net/clk;198757673;13503038;p?http://java.sun.com/javaone

Re: [patch 2/2] [PATCH] virtio-blk: virtio_pci_init can fail

[Marcelo Tosatti]

Looks good. Does SCSI handle pci_register_device() failure too?

Acked-by: Marcelo Tosatti <mtosatti@redhat.com>


On Mon, Apr 21, 2008 at 04:02:49PM -0700, Chris Wright wrote:
> The pci_device_register() call in virtio_pci_init() can fail.
> Handle this error condition instead of generating a SEGV.
> 
> Cc: Marcelo Tosatti <mtosatti@redhat.com>
> Signed-off-by: Chris Wright <chrisw@sous-sol.org>
> ---
>  qemu/hw/virtio-blk.c |    2 ++
>  1 file changed, 2 insertions(+)
> 
> --- a/qemu/hw/virtio-blk.c
> +++ b/qemu/hw/virtio-blk.c
> @@ -163,6 +163,8 @@ void *virtio_blk_init(PCIBus *bus, uint1
>  				       0, VIRTIO_ID_BLOCK,
>  				       0x01, 0x80, 0x00,
>  				       sizeof(struct virtio_blk_config), sizeof(VirtIOBlock));
> +    if (!s)
> +    	return NULL;
>  
>      s->vdev.update_config = virtio_blk_update_config;
>      s->vdev.get_features = virtio_blk_get_features;
> 
> -- 

-------------------------------------------------------------------------
This SF.net email is sponsored by the 2008 JavaOne(SM) Conference 
Don't miss this year's exciting event. There's still time to save $100. 
Use priority code J8TL2D2. 
http://ad.doubleclick.net/clk;198757673;13503038;p?http://java.sun.com/javaone

Re: [patch 2/2] [PATCH] virtio-blk: virtio_pci_init can fail

[Chris Wright]

* Marcelo Tosatti (mtosatti@redhat.com) wrote:
> 
> Looks good. Does SCSI handle pci_register_device() failure too?

Yeah, but it missed actually checking the return value from
lsi_scsi_init.  Patch to follow.

thanks,
-chris

-------------------------------------------------------------------------
This SF.net email is sponsored by the 2008 JavaOne(SM) Conference 
Don't miss this year's exciting event. There's still time to save $100. 
Use priority code J8TL2D2. 
http://ad.doubleclick.net/clk;198757673;13503038;p?http://java.sun.com/javaone

[patch 3/2] hotadd: lsi_scsi_init can fail

[Chris Wright]

During hotadd of SCSI devices lsi_scsi_init() handles failed
pci_device_register(), but qemu_system_hot_add_storage() will try and
attach a drive any way.  Handle this error case rather the generating
SEGV.

Cc: Marcelo Tosatti <mtosatti@redhat.com>
Signed-off-by: Chris Wright <chrisw@sous-sol.org>
---
 qemu/hw/device-hotplug.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

--- a/qemu/hw/device-hotplug.c
+++ b/qemu/hw/device-hotplug.c
@@ -125,7 +125,7 @@ static PCIDevice *qemu_system_hot_add_st
     switch (type) {
     case IF_SCSI:
         opaque = lsi_scsi_init (pci_bus, -1);
-        if (drive_idx >= 0)
+        if (opaque && drive_idx >= 0)
             lsi_scsi_attach (opaque, drives_table[drive_idx].bdrv,
                              drives_table[drive_idx].unit);
         break;

-------------------------------------------------------------------------
This SF.net email is sponsored by the 2008 JavaOne(SM) Conference 
Don't miss this year's exciting event. There's still time to save $100. 
Use priority code J8TL2D2. 
http://ad.doubleclick.net/clk;198757673;13503038;p?http://java.sun.com/javaone

Re: [patch 3/2] hotadd: lsi_scsi_init can fail

[Avi Kivity]

Chris Wright wrote:
> During hotadd of SCSI devices lsi_scsi_init() handles failed
> pci_device_register(), but qemu_system_hot_add_storage() will try and
> attach a drive any way.  Handle this error case rather the generating
> SEGV.
>
> Cc: Marcelo Tosatti <mtosatti@redhat.com>
> Signed-off-by: Chris Wright <chrisw@sous-sol.org>
> ---
>  qemu/hw/device-hotplug.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> --- a/qemu/hw/device-hotplug.c
> +++ b/qemu/hw/device-hotplug.c
> @@ -125,7 +125,7 @@ static PCIDevice *qemu_system_hot_add_st
>      switch (type) {
>      case IF_SCSI:
>          opaque = lsi_scsi_init (pci_bus, -1);
> -        if (drive_idx >= 0)
> +        if (opaque && drive_idx >= 0)
>              lsi_scsi_attach (opaque, drives_table[drive_idx].bdrv,
>                               drives_table[drive_idx].unit);
>          break;
>   

It's not so opaque if you're testing it against NULL...

long term we want better error reporting here.

-- 
Any sufficiently difficult bug is indistinguishable from a feature.


-------------------------------------------------------------------------
This SF.net email is sponsored by the 2008 JavaOne(SM) Conference 
Don't miss this year's exciting event. There's still time to save $100. 
Use priority code J8TL2D2. 
http://ad.doubleclick.net/clk;198757673;13503038;p?http://java.sun.com/javaone

Re: [patch 0/2] pci_register_device can fail

[Avi Kivity]

Chris Wright wrote:
> The pci hotadd patches make it easy to trigger segfaults when adding more
> devices than a single PCI bus can handle.  The following 2 patches fix the
> pci nic devices and virtio-blk device.  Now the following the following:
>
>   OK bus 0, slot 31, function 0 (devfn 248)
>   (qemu) pci_add 0 nic model=virtio
>   Segmentation fault
>
>   OK bus 0, slot 31, function 0 (devfn 248)
>   (qemu) pci_add 0 storage file=/mnt/disk1,if=virtio
>   Segmentation fault
>
> become:
>
>   OK bus 0, slot 31, function 0 (devfn 248)
>   (qemu) pci_add 0 nic model=virtio
>   qemu: Unable to initialze NIC: virtio
>   failed to add model=virtio
>
>   OK bus 0, slot 31, function 0 (devfn 248)
>   (qemu) pci_add 0 storage file=/mnt/disk1,if=virtio
>   failed to add file=/mnt/disk1,if=virtio
>   

Applied all three, thanks.

-- 
error compiling committee.c: too many arguments to function


-------------------------------------------------------------------------
This SF.net email is sponsored by the 2008 JavaOne(SM) Conference 
Don't miss this year's exciting event. There's still time to save $100. 
Use priority code J8TL2D2. 
http://ad.doubleclick.net/clk;198757673;13503038;p?http://java.sun.com/javaone