[PATCH 00/10] virtio: console: fixes for races with port unplug

[PATCH 00/10] virtio: console: fixes for races with port unplug

[Amit Shah]

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

[PATCH 01/10] virtio: console: fix race with port unplug and open/close

[Amit Shah]

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@vger.kernel.org>
Reported-by: Mateusz Guzik <mguzik@redhat.com>
Signed-off-by: Amit Shah <amit.shah@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

[PATCH 02/10] virtio: console: fix race in port_fops_open() and port unplug

[Amit Shah]

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

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

[Amit Shah]

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@vger.kernel.org>
Reported-by: chayang <chayang@redhat.com>
Reported-by: YOGANANTH SUBRAMANIAN <anantyog@in.ibm.com>
Reported-by: FuXiangChun <xfu@redhat.com>
Reported-by: Qunfang Zhang <qzhang@redhat.com>
Reported-by: Sibiao Luo <sluo@redhat.com>
Signed-off-by: Amit Shah <amit.shah@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

[PATCH 04/10] virtio: console: return -ENODEV on all read operations after unplug

[Amit Shah]

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

[PATCH 05/10] virtio: console: update private_data in struct file only on successful open

[Amit Shah]

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@vger.kernel.org>
Reported-by: Mateusz Guzik <mguzik@redhat.com>
Signed-off-by: Amit Shah <amit.shah@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

[PATCH 06/10] virtio: console: fix race in port_fops_poll() and port unplug

[Amit Shah]

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@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

[PATCH 07/10] virtio: console: fix raising SIGIO after port unplug

[Amit Shah]

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@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

[PATCH 08/10] virtio: console: add locks around buffer removal in port unplug path

[Amit Shah]

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

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

[PATCH 09/10] virtio: console: add locking in port unplug path

[Amit Shah]

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@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

[PATCH 10/10] virtio: console: fix locking around send_sigio_to_port()

[Amit Shah]

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@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

Re: [PATCH 03/10] virtio: console: clean up port data immediately at time of unplug

[Jason Wang]

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@vger.kernel.org>
> Reported-by: chayang <chayang@redhat.com>
> Reported-by: YOGANANTH SUBRAMANIAN <anantyog@in.ibm.com>
> Reported-by: FuXiangChun <xfu@redhat.com>
> Reported-by: Qunfang Zhang <qzhang@redhat.com>
> Reported-by: Sibiao Luo <sluo@redhat.com>
> Signed-off-by: Amit Shah <amit.shah@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().

Re: [PATCH 03/10] virtio: console: clean up port data immediately at time of unplug

[Amit Shah]

On (Fri) 19 Jul 2013 [11:21:47], Jason Wang wrote:
> On 07/19/2013 04:16 AM, Amit Shah wrote:


> > 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().

It is removed before kfree() -- kfree() is called in remove_port(),
which is called when all the references are dropped.  (Did you confuse
kfree(port->name) with kfree(port)?)

Thanks,

		Amit

Re: [PATCH 00/10] virtio: console: fixes for races with port unplug

[Amit Shah]

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

Re: [PATCH 04/10] virtio: console: return -ENODEV on all read operations after unplug

[Jason Wang]

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@vger.kernel.org>
> Signed-off-by: Amit Shah <amit.shah@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;
>  	/*

Re: [PATCH 03/10] virtio: console: clean up port data immediately at time of unplug

[Jason Wang]

On 07/19/2013 01:02 PM, Amit Shah wrote:
> On (Fri) 19 Jul 2013 [11:21:47], Jason Wang wrote:
>> On 07/19/2013 04:16 AM, Amit Shah wrote:
>
>>> 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().
> It is removed before kfree() -- kfree() is called in remove_port(),
> which is called when all the references are dropped.  (Did you confuse
> kfree(port->name) with kfree(port)?)

Nope. Looks like port->name were accessed in debugfs_read()?
>
> Thanks,
>
> 		Amit

Re: [PATCH 03/10] virtio: console: clean up port data immediately at time of unplug

[Amit Shah]

On (Fri) 19 Jul 2013 [13:11:54], Jason Wang wrote:
> On 07/19/2013 01:02 PM, Amit Shah wrote:
> > On (Fri) 19 Jul 2013 [11:21:47], Jason Wang wrote:
> >> On 07/19/2013 04:16 AM, Amit Shah wrote:
> >
> >>> 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().
> > It is removed before kfree() -- kfree() is called in remove_port(),
> > which is called when all the references are dropped.  (Did you confuse
> > kfree(port->name) with kfree(port)?)
> 
> Nope. Looks like port->name were accessed in debugfs_read()?

Ah, got it.  Since this bug existed before this patch as well, I'll
post an additional patch on top of this.

Thanks!

		Amit

Re: [PATCH 04/10] virtio: console: return -ENODEV on all read operations after unplug

[Amit Shah]

On (Fri) 19 Jul 2013 [13:07:49], Jason Wang wrote:
> 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@vger.kernel.org>
> > Signed-off-by: Amit Shah <amit.shah@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 that happens, port_has_data() returns false, and the later
functions return appropriately, as unplug_port() clears out all the
data.

However, I spotted a couple of things that need fixing:

1. In the condition you describe above, port_has_data will return
false, and host_connected will be false as well, and fops_read() will
return 0 instead of -ENODEV once.  Subsequent reads will return
-ENODEV.

2. get_inbuf(), which is called by port_has_data() accesses the vqs
even if the port is unplugged.  The vqs are still available, and won't
have data in them (since the port is unplugged), but it's best to not
rely on such behaviour.  For the next merge window, I'll add extra
state, port_(un)plugged to track this instead of abusing
guest_connected.

That also simplifies the path for later if we get vq hot-plug as well.

I think the current code will behave satisfactorily for now; what do
you think?

		Amit

Re: [PATCH 04/10] virtio: console: return -ENODEV on all read operations after unplug

[Jason Wang]

On 07/19/2013 01:45 PM, Amit Shah wrote:
> On (Fri) 19 Jul 2013 [13:07:49], Jason Wang wrote:
>> 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@vger.kernel.org>
>>> Signed-off-by: Amit Shah <amit.shah@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 that happens, port_has_data() returns false, and the later
> functions return appropriately, as unplug_port() clears out all the
> data.
>
> However, I spotted a couple of things that need fixing:
>
> 1. In the condition you describe above, port_has_data will return
> false, and host_connected will be false as well, and fops_read() will
> return 0 instead of -ENODEV once.  Subsequent reads will return
> -ENODEV.

Then the check is not needed here.

>
> 2. get_inbuf(), which is called by port_has_data() accesses the vqs
> even if the port is unplugged.  The vqs are still available, and won't
> have data in them (since the port is unplugged), but it's best to not
> rely on such behaviour.  For the next merge window, I'll add extra
> state, port_(un)plugged to track this instead of abusing
> guest_connected.
>
> That also simplifies the path for later if we get vq hot-plug as well.
>
> I think the current code will behave satisfactorily for now; what do
> you think?

Yes, sounds good.
>
> 		Amit

Re: [PATCH 06/10] virtio: console: fix race in port_fops_poll() and port unplug

[Jason Wang]

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@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.

Re: [PATCH 06/10] virtio: console: fix race in port_fops_poll() and port unplug

[Amit Shah]

On (Fri) 19 Jul 2013 [15:03:50], Jason Wang wrote:
> 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@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.

unplug_port() calls wake_up_interruptible on the waitqueue.

(But the wake_up should be done after guest_connected is set to
false -- regression introduced in patch 7.)

		Amit

Re: [PATCH 06/10] virtio: console: fix race in port_fops_poll() and port unplug

[Jason Wang]

On 07/19/2013 03:48 PM, Amit Shah wrote:
> On (Fri) 19 Jul 2013 [15:03:50], Jason Wang wrote:
>> 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@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.
> unplug_port() calls wake_up_interruptible on the waitqueue.

I mean the following cases:

CPU0:                                                CPU1: unplug_port()


if (!port->guest_connected) {
    return POLLHUP;
}
                                                    
wake_up_interruptiable()
poll_wait(filp, &port->waitqueue, wait);

But since it was existed even w/o this series. I agree to keep it as is
and fix on top.

Other looks good.

Thanks
>
> (But the wake_up should be done after guest_connected is set to
> false -- regression introduced in patch 7.)
>
> 		Amit

Re: [PATCH 06/10] virtio: console: fix race in port_fops_poll() and port unplug

[Amit Shah]

On (Fri) 19 Jul 2013 [18:17:32], Jason Wang wrote:
> On 07/19/2013 03:48 PM, Amit Shah wrote:
> > On (Fri) 19 Jul 2013 [15:03:50], Jason Wang wrote:
> >> 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@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.
> > unplug_port() calls wake_up_interruptible on the waitqueue.
> 
> I mean the following cases:

(formatting to fit properly:)

> 
> CPU0:                                CPU1: unplug_port()
> 
> if (!port->guest_connected) {
>     return POLLHUP;
> }
>                                      wake_up_interruptiable()
> 
> poll_wait(filp, &port->waitqueue, wait);

Agreed, this can happen.  I can't think of a way to resolve this.  One
way would be to remove the waitqueue (port->waitqueue = NULL in
unplug_port()), but I'm not sure of the effect on the other parts
yet.  I'll leave this one for later analysis.

> But since it was existed even w/o this series. I agree to keep it as is
> and fix on top.

Yes, I think so too.

> Other looks good.

Thanks for your review!

		Amit

Re: [PATCH 05/10] virtio: console: update private_data in struct file only on successful open

[Rusty Russell]

Amit Shah <amit.shah@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@vger.kernel.org>
> Reported-by: Mateusz Guzik <mguzik@redhat.com>
> Signed-off-by: Amit Shah <amit.shah@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

Re: [PATCH 06/10] virtio: console: fix race in port_fops_poll() and port unplug

[Rusty Russell]

Amit Shah <amit.shah@redhat.com> writes:
> On (Fri) 19 Jul 2013 [18:17:32], Jason Wang wrote:
>> On 07/19/2013 03:48 PM, Amit Shah wrote:
>> > On (Fri) 19 Jul 2013 [15:03:50], Jason Wang wrote:
>> >> 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@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.
>> > unplug_port() calls wake_up_interruptible on the waitqueue.
>> 
>> I mean the following cases:
>
> (formatting to fit properly:)
>
>> 
>> CPU0:                                CPU1: unplug_port()
>> 
>> if (!port->guest_connected) {
>>     return POLLHUP;
>> }
>>                                      wake_up_interruptiable()
>> 
>> poll_wait(filp, &port->waitqueue, wait);
>
> Agreed, this can happen.  I can't think of a way to resolve this.  One
> way would be to remove the waitqueue (port->waitqueue = NULL in
> unplug_port()), but I'm not sure of the effect on the other parts
> yet.  I'll leave this one for later analysis.

No, you are confused by the name, I think,

poll_wait() doesn't actually wait.  It's more like a poll_enqueue().

Cheers,
Rusty.

Re: [PATCH 08/10] virtio: console: add locks around buffer removal in port unplug path

[Rusty Russell]

Amit Shah <amit.shah@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@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

Re: [PATCH 06/10] virtio: console: fix race in port_fops_poll() and port unplug

[Jason Wang]

On 07/22/2013 01:45 PM, Rusty Russell wrote:
> Amit Shah <amit.shah@redhat.com> writes:
>> On (Fri) 19 Jul 2013 [18:17:32], Jason Wang wrote:
>>> On 07/19/2013 03:48 PM, Amit Shah wrote:
>>>> On (Fri) 19 Jul 2013 [15:03:50], Jason Wang wrote:
>>>>> 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@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.
>>>> unplug_port() calls wake_up_interruptible on the waitqueue.
>>> I mean the following cases:
>> (formatting to fit properly:)
>>
>>> CPU0:                                CPU1: unplug_port()
>>>
>>> if (!port->guest_connected) {
>>>     return POLLHUP;
>>> }
>>>                                      wake_up_interruptiable()
>>>
>>> poll_wait(filp, &port->waitqueue, wait);
>> Agreed, this can happen.  I can't think of a way to resolve this.  One
>> way would be to remove the waitqueue (port->waitqueue = NULL in
>> unplug_port()), but I'm not sure of the effect on the other parts
>> yet.  I'll leave this one for later analysis.
> No, you are confused by the name, I think,
>
> poll_wait() doesn't actually wait.  It's more like a poll_enqueue().

Yes, but the caller will wait then and since the wakeup was called
before adding into waitqueue. It may block forever?

>
> Cheers,
> Rusty.

Re: [PATCH 06/10] virtio: console: fix race in port_fops_poll() and port unplug

[Rusty Russell]

Jason Wang <jasowang@redhat.com> writes:
> On 07/22/2013 01:45 PM, Rusty Russell wrote:
>> Amit Shah <amit.shah@redhat.com> writes:
>>> On (Fri) 19 Jul 2013 [18:17:32], Jason Wang wrote:
>>>> On 07/19/2013 03:48 PM, Amit Shah wrote:
>>>>> On (Fri) 19 Jul 2013 [15:03:50], Jason Wang wrote:
>>>>>> 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@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.
>>>>> unplug_port() calls wake_up_interruptible on the waitqueue.
>>>> I mean the following cases:
>>> (formatting to fit properly:)
>>>
>>>> CPU0:                                CPU1: unplug_port()
>>>>
>>>> if (!port->guest_connected) {
>>>>     return POLLHUP;
>>>> }
>>>>                                      wake_up_interruptiable()
>>>>
>>>> poll_wait(filp, &port->waitqueue, wait);
>>> Agreed, this can happen.  I can't think of a way to resolve this.  One
>>> way would be to remove the waitqueue (port->waitqueue = NULL in
>>> unplug_port()), but I'm not sure of the effect on the other parts
>>> yet.  I'll leave this one for later analysis.
>> No, you are confused by the name, I think,
>>
>> poll_wait() doesn't actually wait.  It's more like a poll_enqueue().
>
> Yes, but the caller will wait then and since the wakeup was called
> before adding into waitqueue. It may block forever?

No, we enqueue then check:

	port = filp->private_data;
	poll_wait(filp, &port->waitqueue, wait);

	if (!port->guest_connected) {
		/* Port got unplugged */
		return POLLHUP;
	}
	ret = 0;
	if (!will_read_block(port))
		ret |= POLLIN | POLLRDNORM;
	if (!will_write_block(port))
		ret |= POLLOUT;
	if (!port->host_connected)
		ret |= POLLHUP;

	return ret;

Which is the correct way to do this.

Cheers,
Rusty.

Re: [PATCH 06/10] virtio: console: fix race in port_fops_poll() and port unplug

[Jason Wang]

On 07/23/2013 01:26 PM, Rusty Russell wrote:
> Jason Wang <jasowang@redhat.com> writes:
>> On 07/22/2013 01:45 PM, Rusty Russell wrote:
>>> Amit Shah <amit.shah@redhat.com> writes:
>>>> On (Fri) 19 Jul 2013 [18:17:32], Jason Wang wrote:
>>>>> On 07/19/2013 03:48 PM, Amit Shah wrote:
>>>>>> On (Fri) 19 Jul 2013 [15:03:50], Jason Wang wrote:
>>>>>>> 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@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.
>>>>>> unplug_port() calls wake_up_interruptible on the waitqueue.
>>>>> I mean the following cases:
>>>> (formatting to fit properly:)
>>>>
>>>>> CPU0:                                CPU1: unplug_port()
>>>>>
>>>>> if (!port->guest_connected) {
>>>>>     return POLLHUP;
>>>>> }
>>>>>                                      wake_up_interruptiable()
>>>>>
>>>>> poll_wait(filp, &port->waitqueue, wait);
>>>> Agreed, this can happen.  I can't think of a way to resolve this.  One
>>>> way would be to remove the waitqueue (port->waitqueue = NULL in
>>>> unplug_port()), but I'm not sure of the effect on the other parts
>>>> yet.  I'll leave this one for later analysis.
>>> No, you are confused by the name, I think,
>>>
>>> poll_wait() doesn't actually wait.  It's more like a poll_enqueue().
>> Yes, but the caller will wait then and since the wakeup was called
>> before adding into waitqueue. It may block forever?
> No, we enqueue then check:
>
> 	port = filp->private_data;
> 	poll_wait(filp, &port->waitqueue, wait);
>
> 	if (!port->guest_connected) {
> 		/* Port got unplugged */
> 		return POLLHUP;
> 	}
> 	ret = 0;
> 	if (!will_read_block(port))
> 		ret |= POLLIN | POLLRDNORM;
> 	if (!will_write_block(port))
> 		ret |= POLLOUT;
> 	if (!port->host_connected)
> 		ret |= POLLHUP;
>
> 	return ret;
>
> Which is the correct way to do this.

Right, thanks for the explaining.
>
> Cheers,
> Rusty.

Re: [PATCH 06/10] virtio: console: fix race in port_fops_poll() and port unplug

[Amit Shah]

On (Mon) 22 Jul 2013 [15:15:34], Rusty Russell wrote:
> Amit Shah <amit.shah@redhat.com> writes:
> > On (Fri) 19 Jul 2013 [18:17:32], Jason Wang wrote:
> >> On 07/19/2013 03:48 PM, Amit Shah wrote:
> >> > On (Fri) 19 Jul 2013 [15:03:50], Jason Wang wrote:
> >> >> 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@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.
> >> > unplug_port() calls wake_up_interruptible on the waitqueue.
> >> 
> >> I mean the following cases:
> >
> > (formatting to fit properly:)
> >
> >> 
> >> CPU0:                                CPU1: unplug_port()
> >> 
> >> if (!port->guest_connected) {
> >>     return POLLHUP;
> >> }
> >>                                      wake_up_interruptiable()
> >> 
> >> poll_wait(filp, &port->waitqueue, wait);
> >
> > Agreed, this can happen.  I can't think of a way to resolve this.  One
> > way would be to remove the waitqueue (port->waitqueue = NULL in
> > unplug_port()), but I'm not sure of the effect on the other parts
> > yet.  I'll leave this one for later analysis.
> 
> No, you are confused by the name, I think,
> 
> poll_wait() doesn't actually wait.  It's more like a poll_enqueue().

Ah!  Thanks, yes.

I'll drop this patch.

		Amit

Re: [PATCH 05/10] virtio: console: update private_data in struct file only on successful open

[Amit Shah]

On (Mon) 22 Jul 2013 [15:07:52], Rusty Russell wrote:
> Amit Shah <amit.shah@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.

Thanks!  Will drop this.

		Amit

Re: [PATCH 08/10] virtio: console: add locks around buffer removal in port unplug path

[Amit Shah]

On (Mon) 22 Jul 2013 [15:26:22], Rusty Russell wrote:
> Amit Shah <amit.shah@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@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.

close(), can happen when the port is being unplugged.  We're just
making sure here that port_fops_release() and unplug_port() don't try
to free up the same data at the same time.

> The number of bugfixes here is deeply disturbing.

Yes, the first three fix a bug - close() after unplug.  However, the
others are inadequate locking fixes which I noticed while fixing that
bug.

Port unplug isn't a frequently-used or tested path, so these were
lying unnoticed so far.

>  I wonder if it's be
> easier to rewrite it all with a lock per port, and one global to protect
> ports_driver_data.

Hm, with this series, I don't see anything that might need extra
locking.  Though I'll take a look at this afresh in a while -- and see
if we could simplify something.

Given that this was necessary only for unplug operations, (and they
aren't a 'regular operation', so we could drop the stable@ for the
series?), are you OK with this series for now?

		Amit

Re: [PATCH 08/10] virtio: console: add locks around buffer removal in port unplug path

[Rusty Russell]

Amit Shah <amit.shah@redhat.com> writes:
> On (Mon) 22 Jul 2013 [15:26:22], Rusty Russell wrote:
>> Amit Shah <amit.shah@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@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.
>
> close(), can happen when the port is being unplugged.  We're just
> making sure here that port_fops_release() and unplug_port() don't try
> to free up the same data at the same time.

Why doesn't reference counting help us here?  Surely the last one should
clean up?

>> The number of bugfixes here is deeply disturbing.
>
> Yes, the first three fix a bug - close() after unplug.  However, the
> others are inadequate locking fixes which I noticed while fixing that
> bug.
>
> Port unplug isn't a frequently-used or tested path, so these were
> lying unnoticed so far.
>
>>  I wonder if it's be
>> easier to rewrite it all with a lock per port, and one global to protect
>> ports_driver_data.
>
> Hm, with this series, I don't see anything that might need extra
> locking.  Though I'll take a look at this afresh in a while -- and see
> if we could simplify something.
>
> Given that this was necessary only for unplug operations, (and they
> aren't a 'regular operation', so we could drop the stable@ for the
> series?), are you OK with this series for now?

Let's skip stable@ for theoretical bugs.  Please repost.

Cheers,
Rusty.

Re: [PATCH 08/10] virtio: console: add locks around buffer removal in port unplug path

[Amit Shah]

On (Wed) 24 Jul 2013 [11:19:24], Rusty Russell wrote:
> Amit Shah <amit.shah@redhat.com> writes:
> > On (Mon) 22 Jul 2013 [15:26:22], Rusty Russell wrote:
> >> Amit Shah <amit.shah@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@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.
> >
> > close(), can happen when the port is being unplugged.  We're just
> > making sure here that port_fops_release() and unplug_port() don't try
> > to free up the same data at the same time.
> 
> Why doesn't reference counting help us here?  Surely the last one should
> clean up?

That's what I had originally thought.  But that's not how it should
happen, as detailed in patch 3.

Cleaning up has to happen at the time of port unplug itself, for two
reasons:

1. If the device itself is removed (not just the port on which it
   sits), unplug should do the cleanup, as all vqs will be lost by the
   time close() is called

2. A new port could be plugged in which uses the same vqs as the older
   one, causing the close() to delete data which it should not.

For the 2nd problem, some new state to track the port's status wrt the
host can be introduced, as both of us mentioned in this thread.
However, I'd like to tackle that properly later.

> >> The number of bugfixes here is deeply disturbing.
> >
> > Yes, the first three fix a bug - close() after unplug.  However, the
> > others are inadequate locking fixes which I noticed while fixing that
> > bug.
> >
> > Port unplug isn't a frequently-used or tested path, so these were
> > lying unnoticed so far.
> >
> >>  I wonder if it's be
> >> easier to rewrite it all with a lock per port, and one global to protect
> >> ports_driver_data.
> >
> > Hm, with this series, I don't see anything that might need extra
> > locking.  Though I'll take a look at this afresh in a while -- and see
> > if we could simplify something.
> >
> > Given that this was necessary only for unplug operations, (and they
> > aren't a 'regular operation', so we could drop the stable@ for the
> > series?), are you OK with this series for now?
> 
> Let's skip stable@ for theoretical bugs.  Please repost.

They're not observed in practice yet, right.  Will repost.

Thanks,
		Amit