linux-kernel - Re: [PATCHv6 3/3] vfio/mdev: Synchronize device create/remove with parent removal

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20190604074820.71853cbb.cohuck@redhat.com>
Date:   Tue, 4 Jun 2019 07:48:20 +0200
From:   Cornelia Huck <cohuck@...hat.com>
To:     Parav Pandit <parav@...lanox.com>
Cc:     kvm@...r.kernel.org, linux-kernel@...r.kernel.org,
        kwankhede@...dia.com, alex.williamson@...hat.com, cjia@...dia.com
Subject: Re: [PATCHv6 3/3] vfio/mdev: Synchronize device create/remove with
 parent removal

On Mon,  3 Jun 2019 13:56:58 -0500
Parav Pandit <parav@...lanox.com> wrote:

> In following sequences, child devices created while removing mdev parent
> device can be left out, or it may lead to race of removing half
> initialized child mdev devices.
> 
> issue-1:
> --------
>        cpu-0                         cpu-1
>        -----                         -----
>                                   mdev_unregister_device()
>                                     device_for_each_child()
>                                       mdev_device_remove_cb()
>                                         mdev_device_remove()
> create_store()
>   mdev_device_create()                   [...]
>     device_add()
>                                   parent_remove_sysfs_files()
> 
> /* BUG: device added by cpu-0
>  * whose parent is getting removed
>  * and it won't process this mdev.
>  */
> 
> issue-2:
> --------
> Below crash is observed when user initiated remove is in progress
> and mdev_unregister_driver() completes parent unregistration.
> 
>        cpu-0                         cpu-1
>        -----                         -----
> remove_store()
>    mdev_device_remove()
>    active = false;
>                                   mdev_unregister_device()
>                                   parent device removed.
>    [...]
>    parents->ops->remove()
>  /*
>   * BUG: Accessing invalid parent.
>   */
> 
> This is similar race like create() racing with mdev_unregister_device().
> 
> BUG: unable to handle kernel paging request at ffffffffc0585668
> PGD e8f618067 P4D e8f618067 PUD e8f61a067 PMD 85adca067 PTE 0
> Oops: 0000 [#1] SMP PTI
> CPU: 41 PID: 37403 Comm: bash Kdump: loaded Not tainted 5.1.0-rc6-vdevbus+ #6
> Hardware name: Supermicro SYS-6028U-TR4+/X10DRU-i+, BIOS 2.0b 08/09/2016
> RIP: 0010:mdev_device_remove+0xfa/0x140 [mdev]
> Call Trace:
>  remove_store+0x71/0x90 [mdev]
>  kernfs_fop_write+0x113/0x1a0
>  vfs_write+0xad/0x1b0
>  ksys_write+0x5a/0xe0
>  do_syscall_64+0x5a/0x210
>  entry_SYSCALL_64_after_hwframe+0x49/0xbe
> 
> Therefore, mdev core is improved as below to overcome above issues.
> 
> Wait for any ongoing mdev create() and remove() to finish before
> unregistering parent device.
> This continues to allow multiple create and remove to progress in
> parallel for different mdev devices as most common case.
> At the same time guard parent removal while parent is being accessed by
> create() and remove() callbacks.
> create()/remove() and unregister_device() are synchronized by the rwsem.
> 
> Refactor device removal code to mdev_device_remove_common() to avoid
> acquiring unreg_sem of the parent.
> 
> Fixes: 7b96953bc640 ("vfio: Mediated device Core driver")
> Signed-off-by: Parav Pandit <parav@...lanox.com>
> ---
>  drivers/vfio/mdev/mdev_core.c    | 71 ++++++++++++++++++++++++--------
>  drivers/vfio/mdev/mdev_private.h |  2 +
>  2 files changed, 55 insertions(+), 18 deletions(-)
> 

> @@ -265,6 +299,12 @@ int mdev_device_create(struct kobject *kobj,
>  
>  	mdev->parent = parent;
>  

Adding

/* Check if parent unregistration has started */

here as well might be nice, but no need to resend the patch for that.

> +	if (!down_read_trylock(&parent->unreg_sem)) {
> +		mdev_device_free(mdev);
> +		ret = -ENODEV;
> +		goto mdev_fail;
> +	}
> +
>  	device_initialize(&mdev->dev);
>  	mdev->dev.parent  = dev;
>  	mdev->dev.bus     = &mdev_bus_type;

Reviewed-by: Cornelia Huck <cohuck@...hat.com>