Linux Virtualization - Jul 2013 - [PATCH v2 00/11] virtio: console: fixes for port unplug

Hello,

This series fixes a few bugs and races with port unplug and the
various file operations: read(), write(), close() and poll().

There still might be more races lurking, but testing this series looks
good to at least solve the easily-triggerable ones.  I've run the
virtio-serial testsuite and a few open/close/unplug tests, and haven't
seen any badness.

I've marked these patches for stable@ as well.  I went over the list
twice to check if really each one should go to stable, and to me it
looks like all are stable candidates.

v2
 * add patch 11: Jason found a use-after-free in port unplug
 * patch 7 introduced a regression where the wake_up_interruptible was
   done before guest_connected and host_connected were set to false

Please review and apply,

Amit Shah (11):
  virtio: console: fix race with port unplug and open/close
  virtio: console: fix race in port_fops_open() and port unplug
  virtio: console: clean up port data immediately at time of unplug
  virtio: console: return -ENODEV on all read operations after unplug
  virtio: console: update private_data in struct file only on successful
    open
  virtio: console: fix race in port_fops_poll() and port unplug
  virtio: console: fix raising SIGIO after port unplug
  virtio: console: add locks around buffer removal in port unplug path
  virtio: console: add locking in port unplug path
  virtio: console: fix locking around send_sigio_to_port()
  virtio: console: prevent use-after-free of port name in port unplug

 drivers/char/virtio_console.c | 70 +++++++++++++++++++++++++++----------------
 1 file changed, 45 insertions(+), 25 deletions(-)

-- 
1.8.1.4

There's a window between find_port_by_devt() returning a port and us
taking a kref on the port, where the port could get unplugged.  Fix it
by taking the reference in find_port_by_devt() itself.

Problem reported and analyzed by Mateusz Guzik.

CC: <stable at vger.kernel.org>
Reported-by: Mateusz Guzik <mguzik at redhat.com>
Signed-off-by: Amit Shah <amit.shah at redhat.com>
---
 drivers/char/virtio_console.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index 1b456fe..291f437 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -272,9 +272,12 @@ static struct port *find_port_by_devt_in_portdev(struct
ports_device *portdev,
 	unsigned long flags;
 
 	spin_lock_irqsave(&portdev->ports_lock, flags);
-	list_for_each_entry(port, &portdev->ports, list)
-		if (port->cdev->dev == dev)
+	list_for_each_entry(port, &portdev->ports, list) {
+		if (port->cdev->dev == dev) {
+			kref_get(&port->kref);
 			goto out;
+		}
+	}
 	port = NULL;
 out:
 	spin_unlock_irqrestore(&portdev->ports_lock, flags);
@@ -1019,14 +1022,10 @@ static int port_fops_open(struct inode *inode, struct
file *filp)
 	struct port *port;
 	int ret;
 
+	/* We get the port with a kref here */
 	port = find_port_by_devt(cdev->dev);
 	filp->private_data = port;
 
-	/* Prevent against a port getting hot-unplugged at the same time */
-	spin_lock_irq(&port->portdev->ports_lock);
-	kref_get(&port->kref);
-	spin_unlock_irq(&port->portdev->ports_lock);
-
 	/*
 	 * Don't allow opening of console port devices -- that's done
 	 * via /dev/hvc
-- 
1.8.1.4

Between open() being called and processed, the port can be unplugged.
Check if this happened, and bail out.

A simple test script to reproduce this is:

while true; do for i in $(seq 1 100); do echo $i > /dev/vport0p3; done; done;

This opens and closes the port a lot of times; unplugging the port while
this is happening triggers the race condition.

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

diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index 291f437..b04ec95 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -1024,6 +1024,10 @@ static int port_fops_open(struct inode *inode, struct
file *filp)
 
 	/* We get the port with a kref here */
 	port = find_port_by_devt(cdev->dev);
+	if (!port) {
+		/* Port was unplugged before we could proceed */
+		return -ENXIO;
+	}
 	filp->private_data = port;
 
 	/*
-- 
1.8.1.4

We used to keep the port's char device structs and the /sys entries
around till the last reference to the port was dropped.  This is
actually unnecessary, and resulted in buggy behaviour:

1. Open port in guest
2. Hot-unplug port
3. Hot-plug a port with the same 'name' property as the unplugged one

This resulted in hot-plug being unsuccessful, as a port with the same
name already exists (even though it was unplugged).

This behaviour resulted in a warning message like this one:

-------------------8<---------------------------------------
WARNING: at fs/sysfs/dir.c:512 sysfs_add_one+0xc9/0x130() (Not tainted)
Hardware name: KVM
sysfs: cannot create duplicate filename
'/devices/pci0000:00/0000:00:04.0/virtio0/virtio-ports/vport0p1'

Call Trace:
 [<ffffffff8106b607>] ? warn_slowpath_common+0x87/0xc0
 [<ffffffff8106b6f6>] ? warn_slowpath_fmt+0x46/0x50
 [<ffffffff811f2319>] ? sysfs_add_one+0xc9/0x130
 [<ffffffff811f23e8>] ? create_dir+0x68/0xb0
 [<ffffffff811f2469>] ? sysfs_create_dir+0x39/0x50
 [<ffffffff81273129>] ? kobject_add_internal+0xb9/0x260
 [<ffffffff812733d8>] ? kobject_add_varg+0x38/0x60
 [<ffffffff812734b4>] ? kobject_add+0x44/0x70
 [<ffffffff81349de4>] ? get_device_parent+0xf4/0x1d0
 [<ffffffff8134b389>] ? device_add+0xc9/0x650

-------------------8<---------------------------------------

Instead of relying on guest applications to release all references to
the ports, we should go ahead and unregister the port from all the core
layers.  Any open/read calls on the port will then just return errors,
and an unplug/plug operation on the host will succeed as expected.

This also caused buggy behaviour in case of the device removal (not just
a port): when the device was removed (which means all ports on that
device are removed automatically as well), the ports with active
users would clean up only when the last references were dropped -- and
it would be too late then to be referencing char device pointers,
resulting in oopses:

-------------------8<---------------------------------------
PID: 6162   TASK: ffff8801147ad500  CPU: 0   COMMAND: "cat"
 #0 [ffff88011b9d5a90] machine_kexec at ffffffff8103232b
 #1 [ffff88011b9d5af0] crash_kexec at ffffffff810b9322
 #2 [ffff88011b9d5bc0] oops_end at ffffffff814f4a50
 #3 [ffff88011b9d5bf0] die at ffffffff8100f26b
 #4 [ffff88011b9d5c20] do_general_protection at ffffffff814f45e2
 #5 [ffff88011b9d5c50] general_protection at ffffffff814f3db5
    [exception RIP: strlen+2]
    RIP: ffffffff81272ae2  RSP: ffff88011b9d5d00  RFLAGS: 00010246
    RAX: 0000000000000000  RBX: ffff880118901c18  RCX: 0000000000000000
    RDX: ffff88011799982c  RSI: 00000000000000d0  RDI: 3a303030302f3030
    RBP: ffff88011b9d5d38   R8: 0000000000000006   R9: ffffffffa0134500
    R10: 0000000000001000  R11: 0000000000001000  R12: ffff880117a1cc10
    R13: 00000000000000d0  R14: 0000000000000017  R15: ffffffff81aff700
    ORIG_RAX: ffffffffffffffff  CS: 0010  SS: 0018
 #6 [ffff88011b9d5d00] kobject_get_path at ffffffff8126dc5d
 #7 [ffff88011b9d5d40] kobject_uevent_env at ffffffff8126e551
 #8 [ffff88011b9d5dd0] kobject_uevent at ffffffff8126e9eb
 #9 [ffff88011b9d5de0] device_del at ffffffff813440c7

-------------------8<---------------------------------------

So clean up when we have all the context, and all that's left to do when
the references to the port have dropped is to free up the port struct
itself.

CC: <stable at vger.kernel.org>
Reported-by: chayang <chayang at redhat.com>
Reported-by: YOGANANTH SUBRAMANIAN <anantyog at in.ibm.com>
Reported-by: FuXiangChun <xfu at redhat.com>
Reported-by: Qunfang Zhang <qzhang at redhat.com>
Reported-by: Sibiao Luo <sluo at redhat.com>
Signed-off-by: Amit Shah <amit.shah at redhat.com>
---
 drivers/char/virtio_console.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index b04ec95..6bf0df3 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -1501,14 +1501,6 @@ 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);
-
-	debugfs_remove(port->debugfs_file);
-
 	kfree(port);
 }
 
@@ -1566,6 +1558,14 @@ static void unplug_port(struct port *port)
 	 */
 	port->portdev = NULL;
 
+	sysfs_remove_group(&port->dev->kobj, &port_attribute_group);
+	device_destroy(pdrvdata.class, port->dev->devt);
+	cdev_del(port->cdev);
+
+	kfree(port->name);
+
+	debugfs_remove(port->debugfs_file);
+
 	/*
 	 * Locks around here are not necessary - a port can't be
 	 * opened after we removed the port struct from ports_list
-- 
1.8.1.4

If a port gets unplugged while a user is blocked on read(), -ENODEV is
returned.  However, subsequent read()s returned 0, indicating there's no
host-side connection (but not indicating the device went away).

This also happened when a port was unplugged and the user didn't have
any blocking operation pending.  If the user didn't monitor the SIGIO
signal, they won't have a chance to find out if the port went away.

Fix by returning -ENODEV on all read()s after the port gets unplugged.
write() already behaves this way.

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

diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index 6bf0df3..a39702a 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -749,6 +749,10 @@ static ssize_t port_fops_read(struct file *filp, char
__user *ubuf,
 
 	port = filp->private_data;
 
+	/* Port is hot-unplugged. */
+	if (!port->guest_connected)
+		return -ENODEV;
+
 	if (!port_has_data(port)) {
 		/*
 		 * If nothing's connected on the host just return 0 in
@@ -765,7 +769,7 @@ static ssize_t port_fops_read(struct file *filp, char __user
*ubuf,
 		if (ret < 0)
 			return ret;
 	}
-	/* Port got hot-unplugged. */
+	/* Port got hot-unplugged while we were waiting above. */
 	if (!port->guest_connected)
 		return -ENODEV;
 	/*
-- 
1.8.1.4

Mateusz Guzik points out that we update the 'file' struct's
private_data
field before we've successfully done all our checks.  This means we can
return an error with the private_data field updated.  This could lead to
problems.

Fix by moving the assignment after all checks are done.

CC: <stable at vger.kernel.org>
Reported-by: Mateusz Guzik <mguzik at redhat.com>
Signed-off-by: Amit Shah <amit.shah at redhat.com>
---
 drivers/char/virtio_console.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index a39702a..7728af9 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -1032,7 +1032,6 @@ static int port_fops_open(struct inode *inode, struct file
*filp)
 		/* Port was unplugged before we could proceed */
 		return -ENXIO;
 	}
-	filp->private_data = port;
 
 	/*
 	 * Don't allow opening of console port devices -- that's done
@@ -1051,6 +1050,7 @@ static int port_fops_open(struct inode *inode, struct file
*filp)
 		goto out;
 	}
 
+	filp->private_data = port;
 	port->guest_connected = true;
 	spin_unlock_irq(&port->inbuf_lock);
 
-- 
1.8.1.4

Between poll() being called and processed, the port can be unplugged.
Check if this happened, and bail out.

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

diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index 7728af9..1d4b748 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -967,6 +967,10 @@ static unsigned int port_fops_poll(struct file *filp,
poll_table *wait)
 	unsigned int ret;
 
 	port = filp->private_data;
+	if (!port->guest_connected) {
+		/* Port was unplugged before we could proceed */
+		return POLLHUP;
+	}
 	poll_wait(filp, &port->waitqueue, wait);
 
 	if (!port->guest_connected) {
-- 
1.8.1.4

SIGIO should be sent when a port gets unplugged.  However, the SIGIO is
sent only when there's a userspace application that has the port opened,
and has asked for SIGIO to be delivered.  We were clearing out
guest_connected before calling send_sigio_to_port(), resulting in a
sigio not getting sent to users.

Fix by setting guest_connected to false after invoking the sigio
function.

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

diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index 1d4b748..dd21b2f 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -1542,12 +1542,14 @@ static void unplug_port(struct port *port)
 	spin_unlock_irq(&port->portdev->ports_lock);
 
 	if (port->guest_connected) {
+		/* Let the app know the port is going down. */
+		send_sigio_to_port(port);
+
+		/* Do this after sigio is actually sent */
 		port->guest_connected = false;
 		port->host_connected = false;
-		wake_up_interruptible(&port->waitqueue);
 
-		/* Let the app know the port is going down. */
-		send_sigio_to_port(port);
+		wake_up_interruptible(&port->waitqueue);
 	}
 
 	if (is_console_port(port)) {
-- 
1.8.1.4

The removal functions act on the vqs, and the vq operations need to be
locked.

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

diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index dd21b2f..c9faea1 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -1516,18 +1516,22 @@ static void remove_port_data(struct port *port)
 {
 	struct port_buffer *buf;
 
+	spin_lock_irq(&port->inbuf_lock);
 	/* Remove unused data this port might have received. */
 	discard_port_data(port);
 
-	reclaim_consumed_buffers(port);
-
 	/* Remove buffers we queued up for the Host to send us data in. */
 	while ((buf = virtqueue_detach_unused_buf(port->in_vq)))
 		free_buf(buf, true);
+	spin_unlock_irq(&port->inbuf_lock);
+
+	spin_lock_irq(&port->outvq_lock);
+	reclaim_consumed_buffers(port);
 
 	/* Free pending buffers from the out-queue. */
 	while ((buf = virtqueue_detach_unused_buf(port->out_vq)))
 		free_buf(buf, true);
+	spin_unlock_irq(&port->outvq_lock);
 }
 
 /*
-- 
1.8.1.4

Port unplug can race with close() in port_fops_release().
port_fops_release() already takes the necessary locks, ensure
unplug_port() does that too.

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

diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index c9faea1..bdfb814 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -1545,6 +1545,7 @@ static void unplug_port(struct port *port)
 	list_del(&port->list);
 	spin_unlock_irq(&port->portdev->ports_lock);
 
+	spin_lock_irq(&port->inbuf_lock);
 	if (port->guest_connected) {
 		/* Let the app know the port is going down. */
 		send_sigio_to_port(port);
@@ -1555,6 +1556,7 @@ static void unplug_port(struct port *port)
 
 		wake_up_interruptible(&port->waitqueue);
 	}
+	spin_unlock_irq(&port->inbuf_lock);
 
 	if (is_console_port(port)) {
 		spin_lock_irq(&pdrvdata_lock);
-- 
1.8.1.4

send_sigio_to_port() checks the value of guest_connected, which we
always modify under the inbuf_lock; make sure invocations of
send_sigio_to_port() have take the inbuf_lock around the call.

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

diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index bdfb814..89ef748 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -1674,7 +1674,9 @@ static void handle_control_message(struct ports_device
*portdev,
 		 * If the guest is connected, it'll be interested in
 		 * knowing the host connection state changed.
 		 */
+		spin_lock_irq(&port->inbuf_lock);
 		send_sigio_to_port(port);
+		spin_unlock_irq(&port->inbuf_lock);
 		break;
 	case VIRTIO_CONSOLE_PORT_NAME:
 		/*
@@ -1794,13 +1796,13 @@ static void in_intr(struct virtqueue *vq)
 	if (!port->guest_connected &&
!is_rproc_serial(port->portdev->vdev))
 		discard_port_data(port);
 
+	/* Send a SIGIO indicating new data in case the process asked for it */
+	send_sigio_to_port(port);
+
 	spin_unlock_irqrestore(&port->inbuf_lock, flags);
 
 	wake_up_interruptible(&port->waitqueue);
 
-	/* Send a SIGIO indicating new data in case the process asked for it */
-	send_sigio_to_port(port);
-
 	if (is_console_port(port) && hvc_poll(port->cons.hvc))
 		hvc_kick();
 }
-- 
1.8.1.4

Remove the debugfs path before freeing port->name, to prevent a possible
use-after-free.

CC: <stable at vger.kernel.org>
Reported-by: Jason Wang <jasowang at redhat.com>
Signed-off-by: Amit Shah <amit.shah at redhat.com>
---
 drivers/char/virtio_console.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index 89ef748..f18e960 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -1578,9 +1578,8 @@ static void unplug_port(struct port *port)
 	device_destroy(pdrvdata.class, port->dev->devt);
 	cdev_del(port->cdev);
 
-	kfree(port->name);
-
 	debugfs_remove(port->debugfs_file);
+	kfree(port->name);
 
 	/*
 	 * Locks around here are not necessary - a port can't be
-- 
1.8.1.4

On 07/19/2013 07:21 PM, Amit Shah wrote:
> Hello,
>
> This series fixes a few bugs and races with port unplug and the
> various file operations: read(), write(), close() and poll().
>
> There still might be more races lurking, but testing this series looks
> good to at least solve the easily-triggerable ones.  I've run the
> virtio-serial testsuite and a few open/close/unplug tests, and haven't
> seen any badness.
>
> I've marked these patches for stable@ as well.  I went over the list
> twice to check if really each one should go to stable, and to me it
> looks like all are stable candidates.
>
> v2
>  * add patch 11: Jason found a use-after-free in port unplug
>  * patch 7 introduced a regression where the wake_up_interruptible was
>    done before guest_connected and host_connected were set to false
>
> Please review and apply,
For the series.

Reviewed-by: Jason Wang <jasowang at redhat.com>

Thanks
> Amit Shah (11):
>   virtio: console: fix race with port unplug and open/close
>   virtio: console: fix race in port_fops_open() and port unplug
>   virtio: console: clean up port data immediately at time of unplug
>   virtio: console: return -ENODEV on all read operations after unplug
>   virtio: console: update private_data in struct file only on successful
>     open
>   virtio: console: fix race in port_fops_poll() and port unplug
>   virtio: console: fix raising SIGIO after port unplug
>   virtio: console: add locks around buffer removal in port unplug path
>   virtio: console: add locking in port unplug path
>   virtio: console: fix locking around send_sigio_to_port()
>   virtio: console: prevent use-after-free of port name in port unplug
>
>  drivers/char/virtio_console.c | 70
+++++++++++++++++++++++++++----------------
>  1 file changed, 45 insertions(+), 25 deletions(-)
>