Amit Shah <amit.shah@xxxxxxxxxx> writes: > 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@xxxxxxxxxxxxxxx> > Reported-by: chayang <chayang@xxxxxxxxxx> > Reported-by: YOGANANTH SUBRAMANIAN <anantyog@xxxxxxxxxx> > Reported-by: FuXiangChun <xfu@xxxxxxxxxx> > Reported-by: Qunfang Zhang <qzhang@xxxxxxxxxx> > Reported-by: Sibiao Luo <sluo@xxxxxxxxxx> > Signed-off-by: Amit Shah <amit.shah@xxxxxxxxxx> Applied, thanks. Cheers, Rusty. > --- > 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 -- To unsubscribe from this list: send the line "unsubscribe stable" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html