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

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

[Amit Shah]

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

[PATCH 1/2] virtio: console: Don't access vqs if device was unplugged

[Amit Shah]

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@redhat.com>
CC: stable@kernel.org
Signed-off-by: Amit Shah <amit.shah@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

[PATCH 2/2] virtio: console: Don't call device_destroy() on port device

[Amit Shah]

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

CC: stable@kernel.org
Signed-off-by: Amit Shah <amit.shah@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

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

[Rusty Russell]

On Wed,  2 Mar 2011 13:53:06 +0530, Amit Shah <amit.shah@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.

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

[Amit Shah]

On (Wed) 02 Mar 2011 [21:38:08], Rusty Russell wrote:
> On Wed,  2 Mar 2011 13:53:06 +0530, Amit Shah <amit.shah@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?

Well what seems to be happening is kref_put() in port_fops_release()
calls remove_port(), which calls device_destroy().  Now this triggers
another fput() on the same file, causing port_fops_release() to be
called again, which leads to device_destroy() being called on the same
region.

Slightly more clueful, but still clueless as to why this happens.

		Amit

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

[Amit Shah]

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

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

[Rusty Russell]

On Fri, 11 Mar 2011 16:46:28 +0530, Amit Shah <amit.shah@redhat.com> wrote:
> 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?

Greg K-H needs to be asked this question, I think.

Cheers,
Rusty.