Virtualization - Jul 2013 - [PATCH 03/10] virtio: console: clean up port data immediately at time of unplug

Hello,

This series fixes a few 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.

Please review and apply if appropriate,

Amit Shah (10):
  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()

 drivers/char/virtio_console.c | 67 ++++++++++++++++++++++++++++---------------
 1 file changed, 44 insertions(+), 23 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.

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.

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

diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index 1d4b748..ce71df1 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) {
-		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);
+
+		/* Do this last so that sigio is actually sent */
+		port->guest_connected = false;
 	}
 
 	if (is_console_port(port)) {
-- 
1.8.1.4

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

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 ce71df1..dd0020d 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.

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 dd0020d..b174945 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) {
 		port->host_connected = false;
 		wake_up_interruptible(&port->waitqueue);
@@ -1555,6 +1556,7 @@ static void unplug_port(struct port *port)
 		/* Do this last so that sigio is actually sent */
 		port->guest_connected = false;
 	}
+	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.

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 b174945..3b9b541 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

On 07/19/2013 04:16 AM, Amit Shah wrote:
> 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
Should we remove debugfs file before kfree()? Otherwise looks like a
use-after-free if user access debugfs after kfree().

On (Fri) 19 Jul 2013 [01:46:07], Amit Shah wrote:
> Hello,
> 
> This series fixes a few 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.
Ah - I forgot CC: stable in patch 6 onwards.  I'll post a v2 with
those included, but will wait for review comments for a couple of
days.

		Amit

On 07/19/2013 04:16 AM, Amit Shah wrote:
> 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;
> +
What if the port is hot-unplugged after this check? Should we serialize
the hot plug with inbuf_lock here?
>  	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;
>  	/*

On 07/19/2013 04:16 AM, Amit Shah wrote:
> Between poll() being called and processed, the port can be unplugged.
> Check if this happened, and bail out.
>
> 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) {
Looks still racy here. Unlike port_fops_read() which check
will_read_block(). If unplug happens after the check but before the
poll_wait(), caller will be blocked forever.

Amit Shah <amit.shah at redhat.com> writes:
> 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.
No, this is a bit weird, but it's fine.

If we fail open, filp will be destroyed; we won't be told about it, and
private_data will never be accessed.

Cheers,
Rusty.
> 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

Amit Shah <amit.shah at redhat.com> writes:
> The removal functions act on the vqs, and the vq operations need to be
> locked.
>
> Signed-off-by: Amit Shah <amit.shah at redhat.com>
How can userspace access the port now?  By the time we are cleaning up
buffers, there should be no possibility of such accesses.

The number of bugfixes here is deeply disturbing.  I wonder if it's be
easier to rewrite it all with a lock per port, and one global to protect
ports_driver_data.

Cheers,
Rusty.
> ---
>  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 ce71df1..dd0020d 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