From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jason Wang Subject: Re: [PATCH 03/10] virtio: console: clean up port data immediately at time of unplug Date: Fri, 19 Jul 2013 11:21:47 +0800 Message-ID: <51E8B0CB.1060703@redhat.com> References: Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: virtualization-bounces@lists.linux-foundation.org Errors-To: virtualization-bounces@lists.linux-foundation.org To: Amit Shah Cc: stable@vger.kernel.org, Virtualization List List-Id: virtualization@lists.linuxfoundation.org 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: > [] ? warn_slowpath_common+0x87/0xc0 > [] ? warn_slowpath_fmt+0x46/0x50 > [] ? sysfs_add_one+0xc9/0x130 > [] ? create_dir+0x68/0xb0 > [] ? sysfs_create_dir+0x39/0x50 > [] ? kobject_add_internal+0xb9/0x260 > [] ? kobject_add_varg+0x38/0x60 > [] ? kobject_add+0x44/0x70 > [] ? get_device_parent+0xf4/0x1d0 > [] ? 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: > Reported-by: chayang > Reported-by: YOGANANTH SUBRAMANIAN > Reported-by: FuXiangChun > Reported-by: Qunfang Zhang > Reported-by: Sibiao Luo > Signed-off-by: Amit Shah > --- > 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().