Virtualization - Mar 2011 - [PATCH 0/2] Fix hot-unplug: device removal while port in use

A crash was observed when a device gets removed while a port is in
use.  When the port gets removed, we tried to free vq buffers.  The vq
no longer exists at this stage, just ensure we don't access it.

The second patch fixes a warning where the pci region is already
freed.  I'm not sure what or how the region gets freed, any clues
there will be helpful.

Thanks,
		Amit

Amit Shah (2):
  virtio: console: Don't access vqs if device was unplugged
  virtio: console: Don't call device_destroy() on port device

 drivers/char/virtio_console.c |   10 +++++++++-
 1 files changed, 9 insertions(+), 1 deletions(-)

-- 
1.7.4

If a virtio-console device gets unplugged while a port is open, a
subsequent close() call on the port accesses vqs to free up buffers.
This can lead to a crash.

The buffers are already freed up as a result of the call to
unplug_ports() from virtcons_remove().  The fix is to simply not access
vq information if port->portdev is NULL.

Reported-by: juzhang <juzhang at redhat.com>
CC: stable at kernel.org
Signed-off-by: Amit Shah <amit.shah at redhat.com>
---
 drivers/char/virtio_console.c |    8 ++++++++
 1 files changed, 8 insertions(+), 0 deletions(-)

diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index 4903931..84b164d 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -388,6 +388,10 @@ static void discard_port_data(struct port *port)
 	unsigned int len;
 	int ret;
 
+	if (!port->portdev) {
+		/* Device has been unplugged.  vqs are already gone. */
+		return;
+	}
 	vq = port->in_vq;
 	if (port->inbuf)
 		buf = port->inbuf;
@@ -470,6 +474,10 @@ static void reclaim_consumed_buffers(struct port *port)
 	void *buf;
 	unsigned int len;
 
+	if (!port->portdev) {
+		/* Device has been unplugged.  vqs are already gone. */
+		return;
+	}
 	while ((buf = virtqueue_get_buf(port->out_vq, &len))) {
 		kfree(buf);
 		port->outvq_full = false;
-- 
1.7.4

This results in a warning mentioning the region being removed is already
gone.

CC: stable at kernel.org
Signed-off-by: Amit Shah <amit.shah at redhat.com>
---
 drivers/char/virtio_console.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index 84b164d..5ac9fd9 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -1244,7 +1244,7 @@ static void remove_port(struct kref *kref)
 	port = container_of(kref, struct port, kref);
 
 	sysfs_remove_group(&port->dev->kobj, &port_attribute_group);
-	device_destroy(pdrvdata.class, port->dev->devt);
+
 	cdev_del(port->cdev);
 
 	kfree(port->name);
-- 
1.7.4

On Wed,  2 Mar 2011 13:53:06 +0530, Amit Shah <amit.shah at redhat.com>
wrote:
> A crash was observed when a device gets removed while a port is in
> use.  When the port gets removed, we tried to free vq buffers.  The vq
> no longer exists at this stage, just ensure we don't access it.
> 
> The second patch fixes a warning where the pci region is already
> freed.  I'm not sure what or how the region gets freed, any clues
> there will be helpful.
Put a printk and WARN_ON() in the pci region freeing code, look through
the backtraces?

Rusty.

On (Wed) 02 Mar 2011 [13:53:06], Amit Shah wrote:
> The second patch fixes a warning where the pci region is already
> freed.  I'm not sure what or how the region gets freed, any clues
> there will be helpful.
OK, so in the case where a virtio-console port is in use (opened by a
program) and a virtio-console device is removed, the port is kept
around but all the virtio-related state is assumed to be gone.

When the port is finally released (close() called), we call
device_destroy() on the port's device.  This results in the parent
device's structures to be freed as well.  This includes the PCI
regions for the virtio-console PCI device.

Once this is done, however, virtio_pci_release_dev() kicks in, as the
last ref to the virtio device is now gone, and attempts to do

     pci_iounmap(pci_dev, vp_dev->ioaddr);
     pci_release_regions(pci_dev);
     pci_disable_device(pci_dev);

which results in the double-free warning.

Ideally virtio_pci_release_dev() shouldn't be needed at all; all that
work can be moved to virtio_pci_remove().  virtio_pci_release_dev()
was added in 29f9f12e to curb a warning:

    virtio: add PCI device release() function
    
    Add a release() function for virtio_pci devices so as to avoid:
    
      Device 'virtio0' does not have a release() function, it is
      broken and must be fixed
    

So we could have an empty release() function that does nothing, and
all of the current functionality be moved to virtio_pci_remove(), as
it was earlier.  This should keep everyone happy.

Is that fine?

		Amit