ACK/Cmnt: [bionic/master-next 1/1] UBUNTU: SAUCE: vfio -- release device lock before userspace requests

[Stefan Bader]

On 12.09.2018 10:50, Andy Whitcroft wrote:
> During a hotplug event vfio_pci_remove() will call
> vfio_del_group_dev() to release the device group.  This may trigger
> a userspace request.  Currently this userspace request is performed
> while holding the device lock.  This leads userspace to deadlock
> against it while trying to perform the requested cleanup.
> 
> Drop the device lock while the userspace request is in flight.
> After it completes reaquire the lock and revalidate the device as
> it may have been successfully removed by a concurrent operation.
> As the remove callback may now drop the lock also check and
> revalidation at the end of that operation.
> 
> BugLink: http://bugs.launchpad.net/bugs/1792099
> Signed-off-by: Andy Whitcroft <apw at canonical.com>
Acked-by: Stefan Bader <stefan.bader at canonical.com>
> ---

Based on successful testing. From the looks and description this sounds like it
should go upstream, too. And at least also into Cosmic (added some cc's).

-Stefan

>  drivers/base/dd.c   |  7 +++++++
>  drivers/vfio/vfio.c | 23 ++++++++++++++++++++++-
>  2 files changed, 29 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/base/dd.c b/drivers/base/dd.c
> index 2c964f56dafe..37c01054521b 100644
> --- a/drivers/base/dd.c
> +++ b/drivers/base/dd.c
> @@ -868,6 +868,13 @@ static void __device_release_driver(struct device *dev, struct device *parent)
>  			dev->bus->remove(dev);
>  		else if (drv->remove)
>  			drv->remove(dev);
> +		/*
> +		 * A concurrent invocation of the same function might
> +		 * have released the driver successfully while this one
> +		 * was waiting, so check for that.
> +		 */
> +		if (dev->driver != drv)
> +			return;
>  
>  		device_links_driver_cleanup(dev);
>  		dma_deconfigure(dev);
> diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c
> index 2bc3705a99bd..6ae4d3f432fe 100644
> --- a/drivers/vfio/vfio.c
> +++ b/drivers/vfio/vfio.c
> @@ -934,6 +934,10 @@ void *vfio_del_group_dev(struct device *dev)
>  	unsigned int i = 0;
>  	long ret;
>  	bool interrupted = false;
> +	bool locked = true;
> +	struct device_driver *drv;
> +
> +	drv = dev->driver;
>  
>  	/*
>  	 * The group exists so long as we have a device reference.  Get
> @@ -974,8 +978,11 @@ void *vfio_del_group_dev(struct device *dev)
>  		if (!device)
>  			break;
>  
> -		if (device->ops->request)
> +		if (device->ops->request) {
> +			device_unlock(dev);
> +			locked = false;
>  			device->ops->request(device_data, i++);
> +		}
>  
>  		vfio_device_put(device);
>  
> @@ -994,6 +1001,20 @@ void *vfio_del_group_dev(struct device *dev)
>  					 current->comm, task_pid_nr(current));
>  			}
>  		}
> +
> +		if (!locked) {
> +			device_lock(dev);
> +			locked = true;
> +			/*
> +			 * A concurrent operation may have released the driver
> +			 * successfully while we had dropped the lock,
> +			 * check for that.
> +			 */
> +			if (dev->driver != drv) {
> +				vfio_group_put(group);
> +				return NULL;
> +			}
> +		}
>  	} while (ret <= 0);
>  
>  	/*
> 


-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: OpenPGP digital signature
URL: <https://lists.ubuntu.com/archives/kernel-team/attachments/20180912/07170a1b/attachment.sig>