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

[Jason Wang <jasowang@redhat.com>]

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