virtio: console: fixes for bugs and races with unplug
Subsystems (3)
v3 cover virtio: console: fixes for bugs and races with unplug
Hello,
This series fixes a few bugs and races with port unplug and the various file operations: read(), write() and close().
I started coding up an alternative locking mechanism based on the discussion earlier in this series, but some of what we already have has to remain, and the new code is sufficiently different, so I'd rather it bakes for a while, and I ensure there are no regressions wrt the tests I have so far for a while as well. Hopefully this will be in time for the next merge window.
There's one use-after-free I spotted after sending the first two
versions: port_fops_release() calls send_control_msg(), which spins till the host acknowledges receipt of the buffer. While it's spinning, if the device gets unplugged, the vqs go away, and the spinning function never progresses, causing a softlockup. This is difficult to reproduce -- the host usually acknowledges the buffers fast enough. A couple of solutions for this case are possible:
1. Mark the control vq in use, and don't proceed with unplug till it's marked unused,
2. Similar to the various port-specific i and o vqs, don't spin, but queue the buffer and wait for the host to let us know it's done with it.
2nd is easier to implement, but 1st fits with the way I'm thinking of restructuring the locking. I'm not yet decided on which approach to take, will think over it.
Other than that, this series does fix all the bugs I see with the tests I have. Indeed, the patches marked for stable@ fix all the bugs too, and the other ones on top add locking where shared structures are being used.
Please review and apply if appropriate,
v3
* remove patch 5, "update private_data in struct file only on successful open" (Rusty)
* remove patch 6, "fix race in port_fops_poll() and port unplug", (Rusty)
* remove CC: stable from patches without reproducers
v2
* add patch 11: Jason found a use-after-free in port unplug
* patch 7 introduced a regression where the wake_up_interruptible was done before guest_connected and host_connected were set to false
Amit Shah (9):
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: fix raising SIGIO after port unplug
virtio: console: return -ENODEV on all read operations after unplug
virtio: console: add locks around buffer removal in port unplug path
virtio: console: add locking in port unplug path
virtio: console: fix locking around send_sigio_to_port()
virtio: console: prevent use-after-free of port name in port unplug
drivers/char/virtio_console.c | 64 +++++++++++++++++++++++++++---------------- 1 file changed, 40 insertions(+), 24 deletions(-)
Signature · 1 line
1.8.1.4
PATCH v3 1/9 virtio: console: fix race with port unplug and open/close
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.
--- drivers/char/virtio_console.c | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-)
drivers/char/virtio_console.c +6 -7
@@ -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
Signature · 1 line
1.8.1.4
PATCH v3 2/9 virtio: console: fix race in port_fops_open() and port unplug
In reply to Amit ShahBetween 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 bug.
--- drivers/char/virtio_console.c | 4 ++++ 1 file changed, 4 insertions(+)
drivers/char/virtio_console.c +4 -0
@@ -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;/*
Signature · 1 line
1.8.1.4
PATCH v3 3/9 virtio: console: clean up port data immediately at time of unplug
In reply to Amit ShahWe 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.
--- drivers/char/virtio_console.c | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-)
drivers/char/virtio_console.c +8 -8
@@ -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
Signature · 1 line
1.8.1.4
PATCH v3 4/9 virtio: console: fix raising SIGIO after port unplug
In reply to Amit ShahSIGIO should be sent when a port gets unplugged. It should only be sent to prcesses that have the port opened, and have 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 processes.
Fix by setting guest_connected to false after invoking the sigio function.
--- drivers/char/virtio_console.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-)
drivers/char/virtio_console.c +5 -3
@@ -1534,12 +1534,14 @@ static void unplug_port(struct port *port)spin_unlock_irq (&port->portdev->ports_lock);if (port->guest_connected) { +/* Let the app know the port is going down. */ +send_sigio_to_port (port); + +/* Do this after sigio is actually sent */ port->guest_connected = false; port->host_connected = false; -wake_up_interruptible (&port->waitqueue); -/* Let the app know the port is going down. */ -send_sigio_to_port (port); +wake_up_interruptible (&port->waitqueue); }if (is_console_port (port)) {
Signature · 1 line
1.8.1.4
PATCH v3 5/9 virtio: console: return -ENODEV on all read operations after unplug
In reply to Amit ShahIf 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.
--- drivers/char/virtio_console.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)
drivers/char/virtio_console.c +5 -1
@@ -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 ;/*
Signature · 1 line
1.8.1.4
PATCH v3 6/9 virtio: console: add locks around buffer removal in port unplug path
In reply to Amit ShahThe removal functions act on the vqs, and the vq operations need to be locked.
--- drivers/char/virtio_console.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-)
drivers/char/virtio_console.c +6 -2
@@ -1512,18 +1512,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); }/*
Signature · 1 line
1.8.1.4
Rusty Russell
AppliedRe: [PATCH v3 6/9] virtio: console: add locks around buffer removal in port unplug path
Quoted 4 lines · Amit Shah
The removal functions act on the vqs, and the vq operations need to be locked.
Signed-off-by Amit Shah <[email protected]>
Applied,
Rusty.
Quoted 35 lines
--- drivers/char/virtio_console.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-)drivers/char/virtio_console.c +6
-2@@ -1512,18 +1512,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); }/* Signature · 1 line
1.8.1.4
PATCH v3 7/9 virtio: console: add locking in port unplug path
In reply to Amit ShahPort unplug can race with close() in port_fops_release(). port_fops_release() already takes the necessary locks, ensure unplug_port() does that too.
--- drivers/char/virtio_console.c | 2 ++ 1 file changed, 2 insertions(+)
drivers/char/virtio_console.c +2 -0
@@ -1541,6 +1541,7 @@ static void unplug_port(struct port *port)list_del (&port->list);spin_unlock_irq (&port->portdev->ports_lock); +spin_lock_irq (&port->inbuf_lock);if (port->guest_connected) {/* Let the app know the port is going down. */ send_sigio_to_port (port);
@@ -1551,6 +1552,7 @@ static void unplug_port(struct port *port)wake_up_interruptible (&port->waitqueue); } +spin_unlock_irq (&port->inbuf_lock);if (is_console_port (port)) {spin_lock_irq (&pdrvdata_lock);
Signature · 1 line
1.8.1.4
Rusty Russell
AppliedRe: [PATCH v3 7/9] virtio: console: add locking in port unplug path
Quoted 5 lines · Amit Shah
Port unplug can race with
close()inport_fops_release().port_fops_release()already takes the necessary locks, ensureunplug_port()does that too.Signed-off-by Amit Shah <[email protected]>
Applied,
Rusty.
Quoted 26 lines
--- drivers/char/virtio_console.c | 2 ++ 1 file changed, 2 insertions(+)drivers/char/virtio_console.c +2
-0@@ -1541,6 +1541,7 @@ static void unplug_port(struct port *port)list_del (&port->list);spin_unlock_irq (&port->portdev->ports_lock); +spin_lock_irq (&port->inbuf_lock);if (port->guest_connected) {/* Let the app know the port is going down. */ send_sigio_to_port (port);@@ -1551,6 +1552,7 @@ static void unplug_port(struct port *port)wake_up_interruptible (&port->waitqueue); } +spin_unlock_irq (&port->inbuf_lock);if (is_console_port (port)) {spin_lock_irq (&pdrvdata_lock);Signature · 1 line
1.8.1.4
PATCH v3 8/9 virtio: console: fix locking around send_sigio_to_port()
In reply to Amit Shahsend_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.
--- drivers/char/virtio_console.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-)
drivers/char/virtio_console.c +5 -3
@@ -1670,7 +1670,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 :/*
@@ -1790,13 +1792,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 (); }
Signature · 1 line
1.8.1.4
Rusty Russell
AppliedRe: [PATCH v3 8/9] virtio: console: fix locking around send_sigio_to_port()
Quoted 5 lines · Amit Shah
send_sigio_to_port()checks the value ofguest_connected, which we always modify under theinbuf_lock; make sure invocations ofsend_sigio_to_port()have take theinbuf_lockaround the call.Signed-off-by Amit Shah <[email protected]>
Applied,
Rusty.
Quoted 37 lines
--- drivers/char/virtio_console.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-)drivers/char/virtio_console.c +5
-3@@ -1670,7 +1670,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 :/* @@ -1790,13 +1792,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 (); }Signature · 1 line
1.8.1.4
PATCH v3 9/9 virtio: console: prevent use-after-free of port name in port unplug
In reply to Amit ShahRemove the debugfs path before freeing port->name, to prevent a possible use-after-free.
--- drivers/char/virtio_console.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
drivers/char/virtio_console.c +1 -2
@@ -1574,9 +1574,8 @@ static void unplug_port(struct port *port)device_destroy (pdrvdata.class, port->dev->devt);cdev_del (port->cdev); -kfree (port->name); -debugfs_remove (port->debugfs_file); +kfree (port->name);/* * Locks around here are not necessary - a port can't be
Signature · 1 line
1.8.1.4
Rusty Russell
AppliedRe: [PATCH v3 9/9] virtio: console: prevent use-after-free of port name in port unplug
Quoted 5 lines · Amit Shah
Remove the debugfs path before freeing
port->name, to prevent a possible use-after-free.Reported-by Jason Wang <[email protected]>Signed-off-by Amit Shah <[email protected]>
Applied,
Rusty.
Quoted 21 lines
--- drivers/char/virtio_console.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)drivers/char/virtio_console.c +1
-2@@ -1574,9 +1574,8 @@ static void unplug_port(struct port *port)device_destroy (pdrvdata.class, port->dev->devt);cdev_del (port->cdev); -kfree (port->name); -debugfs_remove (port->debugfs_file); +kfree (port->name);/* * Locks around here are not necessary - a port can't be Signature · 1 line
1.8.1.4
Re: [PATCH v3 9/9] virtio: console: prevent use-after-free of port name in port unplug
Quoted 9 lines · Rusty Russell
Quoted 5 lines · Amit Shah
Remove the debugfs path before freeing
port->name, to prevent a possible use-after-free.Reported-by Jason Wang <[email protected]>Signed-off-by Amit Shah <[email protected]>Applied,
Rusty.
Hey Rusty, I don't see this patch in your virtio-next branch:
https://git.kernel.org/cgit/linux/kernel/git/rusty/linux.git/log/?h=virtio-next
The others are present there. Looks like this one got missed?
Amit
Re: [PATCH v3 9/9] virtio: console: prevent use-after-free of port name in port unplug
Quoted 8 lines · Amit Shah
Quoted 7 lines · Rusty Russell
Quoted 5 lines · Amit Shah
Remove the debugfs path before freeing
port->name, to prevent a possible use-after-free.Reported-by Jason Wang <[email protected]>Signed-off-by Amit Shah <[email protected]>
Quoted 8 lines
Quoted 2 lines
Applied,
Rusty.Hey Rusty, I don't see this patch in your virtio-next branch:
https://git.kernel.org/cgit/linux/kernel/git/rusty/linux.git/log/?h=virtio-next
The others are present there. Looks like this one got missed?
Actually, it's in my pending-rebases branch, since it requireds things from the fixes branch. Once the fixes branch has gone to Linus (this week) I will merge it into the virtio-next branch then apply this on top.
It's generally considered bad form to merge into -next branches, but it's allowed for cases like this. But I want to make sure no changes to fixes are required before I merge it, hence the delay for some exposure in linux-next.
Cheers,
Rusty.
Re: [PATCH v3 9/9] virtio: console: prevent use-after-free of port name in port unplug
Quoted 14 lines · Rusty Russell
Quoted 13 lines · Amit Shah
Quoted 9 lines · Rusty Russell
Quoted 5 lines · Amit Shah
Remove the debugfs path before freeing
port->name, to prevent a possible use-after-free.Reported-by Jason Wang <[email protected]>Signed-off-by Amit Shah <[email protected]>Applied,
Rusty.Hey Rusty, I don't see this patch in your virtio-next branch:
Quoted 8 lines
Quoted 3 lines
https://git.kernel.org/cgit/linux/kernel/git/rusty/linux.git/log/?h=virtio-next
The others are present there. Looks like this one got missed?
Actually, it's in my pending-rebases branch, since it requireds things from the fixes branch. Once the fixes branch has gone to Linus (this week) I will merge it into the virtio-next branch then apply this on top.
Ah, thanks!
Quoted 4 lines
It's generally considered bad form to merge into -next branches, but it's allowed for cases like this. But I want to make sure no changes to fixes are required before I merge it, hence the delay for some exposure in linux-next.
Got it, thanks for the explanation!
Amit
A message that is not in the archive (sent off-list or not yet received): 7ca111ad3bca069f921b4234e5b3ccbbfd7a11d8.1374759439.git.amit.shah@redhat.com
Rusty Russell
AppliedRe: [PATCH v3 1/9] virtio: console: fix race with port unplug and open/close
Quoted 5 lines · 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 infind_port_by_devt()itself.Problem reported and analyzed by Mateusz Guzik.
This fix is clearly correct, but what about the other find_port_by_* functions?
Applied,
Rusty.
Quoted 45 lines
CC <[email protected]>Reported-by Mateusz Guzik <[email protected]>Signed-off-by Amit Shah <[email protected]>--- drivers/char/virtio_console.c | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-)drivers/char/virtio_console.c +6
-7@@ -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 Signature · 1 line
1.8.1.4
A message that is not in the archive (sent off-list or not yet received): [email protected]
Re: [PATCH v3 1/9] virtio: console: fix race with port unplug and open/close
Quoted 9 lines · Rusty Russell
Quoted 5 lines · 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 infind_port_by_devt()itself.Problem reported and analyzed by Mateusz Guzik.
This fix is clearly correct, but what about the other find_port_by_* functions?
They don't need a kref -- the kref is only to be bumped when:
1. Initialising / Plugging in the port (add_port)
2. Opening the port (this fix)
Both these cases are now covered. As part of the locking rework, the other find_port_by_* functions may be reworked to set some state, like port_in_use, instead of abusing guest_connected today.
Amit
A message that is not in the archive (sent off-list or not yet received): d8ffeceebfc527db85406850d22fa3da64aabbe3.1374759439.git.amit.shah@redhat.com
Rusty Russell
AppliedRe: [PATCH v3 3/9] virtio: console: clean up port data immediately at time of unplug
Quoted 72 lines · 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 oneThis 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: KVMsysfs: 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:ffff8801147ad500CPU: 0 COMMAND: "cat"
#0 [ffff88011b9d5a90]machine_kexecatffffffff8103232b#1 [ffff88011b9d5af0]crash_kexecatffffffff810b9322#2 [ffff88011b9d5bc0]oops_endatffffffff814f4a50
#3 [ffff88011b9d5bf0] die atffffffff8100f26b
#4 [ffff88011b9d5c20]do_general_protectionatffffffff814f45e2#5 [ffff88011b9d5c50]general_protectionatffffffff814f3db5[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_pathatffffffff8126dc5d#7 [ffff88011b9d5d40]kobject_uevent_envatffffffff8126e551#8 [ffff88011b9d5dd0]kobject_ueventatffffffff8126e9eb#9 [ffff88011b9d5de0]device_delatffffffff813440c7-------------------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.
Quoted 7 lines
CC <[email protected]>Reported-by chayang <[email protected]>Reported-by YOGANANTH SUBRAMANIAN <[email protected]>Reported-by FuXiangChun <[email protected]>Reported-by Qunfang Zhang <[email protected]>Reported-by Sibiao Luo <[email protected]>Signed-off-by Amit Shah <[email protected]>
Applied, thanks.
Cheers,
Rusty.
Quoted 40 lines
--- drivers/char/virtio_console.c | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-)drivers/char/virtio_console.c +8
-8@@ -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 Signature · 1 line
1.8.1.4
A message that is not in the archive (sent off-list or not yet received): a3aca79feb7163e08b940ddefceabf78ab4cd8ce.1374759439.git.amit.shah@redhat.com
Rusty Russell
AppliedRe: [PATCH v3 2/9] virtio: console: fix race in port_fops_open() and port unplug
Quoted 33 lines · 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 bug.
CC <[email protected]>Signed-off-by Amit Shah <[email protected]>--- drivers/char/virtio_console.c | 4 ++++ 1 file changed, 4 insertions(+)drivers/char/virtio_console.c +4
-0@@ -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;/* Signature · 1 line
1.8.1.4
Applied.
Thanks,
Rusty.
A message that is not in the archive (sent off-list or not yet received): e4199103bd85fae76ce7009d7d6abacf28f1f972.1374759439.git.amit.shah@redhat.com
Rusty Russell
AppliedRe: [PATCH v3 4/9] virtio: console: fix raising SIGIO after port unplug
Quoted 11 lines · Amit Shah
SIGIO should be sent when a port gets unplugged. It should only be sent to prcesses that have the port opened, and have asked for SIGIO to be delivered. We were clearing out
guest_connectedbefore callingsend_sigio_to_port(), resulting in a sigio not getting sent to processes.Fix by setting
guest_connectedto false after invoking the sigio function.CC <[email protected]>Signed-off-by Amit Shah <[email protected]>
Applied,
Rusty.
Quoted 28 lines
--- drivers/char/virtio_console.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-)drivers/char/virtio_console.c +5
-3@@ -1534,12 +1534,14 @@ static void unplug_port(struct port *port)spin_unlock_irq (&port->portdev->ports_lock);if (port->guest_connected) { +/* Let the app know the port is going down. */ +send_sigio_to_port (port); + +/* Do this after sigio is actually sent */ port->guest_connected = false; port->host_connected = false; -wake_up_interruptible (&port->waitqueue); -/* Let the app know the port is going down. */ -send_sigio_to_port (port); +wake_up_interruptible (&port->waitqueue); }if (is_console_port (port)) {Signature · 1 line
1.8.1.4
A message that is not in the archive (sent off-list or not yet received): f218052d8d8438fea0d1c3483434e315c7e82db8.1374759439.git.amit.shah@redhat.com
Rusty Russell
AppliedRe: [PATCH v3 5/9] virtio: console: return -ENODEV on all read operations after unplug
Quoted 8 lines · Amit Shah
If a port gets unplugged while a user is blocked on
read(),-ENODEVis 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.
Quoted 8 lines
Fix by returning
-ENODEVon all read()s after the port gets unplugged.write()already behaves this way.CC <[email protected]>Signed-off-by Amit Shah <[email protected]>--- drivers/char/virtio_console.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)
Applied,
Rusty.
Quoted 27 lines
drivers/char/virtio_console.c +5
-1@@ -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 ;/* Signature · 1 line
1.8.1.4