Skip to contentSkip to conversation

Patch "vfio: Remove unnecessary uses of vfio_container.group_lock" has been added to the 4.12-stable tree

PATCH [email protected] 1 participant 1 message Jul 25, 2017

Subsystems (2)
Stable branch VFIO driver
lore
Share & tools
lore

[email protected]

author maintainer Applied
#0 lore raw

This is a note to let you know that I've just added the patch titled

vfio: Remove unnecessary uses of vfio_container.group_lock

to the 4.12-stable tree which can be found at:
http://www.kernel.org/git/?p=linux/kernel/git/stable/stable-queue.git;a=summary

The filename of the patch is:
vfio-remove-unnecessary-uses-of-vfio_container.group_lock.patch
and it can be found in the queue-4.12 subdirectory.

If you, or anyone else, feels it should not be added to the stable tree, please let <[email protected]> know about it.

Quoted 1 line

From 7f56c30bd0a232822aca38d288da475613bdff9b Mon Sep 17 00:00:00 2001

From: Alex Williamson <[email protected]>
Date: Fri, 7 Jul 2017 15:37:38 -0600
Subject: vfio: Remove unnecessary uses of vfio_container.group_lock

From: Alex Williamson <[email protected]>

commit 7f56c30bd0a232822aca38d288da475613bdff9b upstream.

The original intent of vfio_container.group_lock is to protect vfio_container.group_list, however over time it's become a crutch to prevent changes in container composition any time we call into the iommu driver backend. This introduces problems when we start to have more complex interactions, for example when a user's DMA unmap request triggers a notification to an mdev vendor driver, who responds by attempting to unpin mappings within that request, re-entering the iommu backend. We incorrectly assume that the use of read-locks here allow for this nested locking behavior, but a poorly timed write-lock could in fact trigger a deadlock.

The current use of group_lock seems to fall into the trap of locking code, not data. Correct that by removing uses of group_lock that are not directly related to group_list. Note that the vfio type1 iommu backend has its own mutex, vfio_iommu.lock, which it uses to protect itself for each of these interfaces anyway. The group_lock appears to be a redundancy for these interfaces and type1 even goes so far as to release its mutex to allow for exactly the re-entrant code path above.

Reported-by Chuanxiao Dong <[email protected]>
Signed-off-by Alex Williamson <[email protected]>
Acked-by Alexey Kardashevskiy <[email protected]>
Signed-off-by Greg Kroah-Hartman <[email protected]>
---
 drivers/vfio/vfio.c |   38 --------------------------------------
 1 file changed, 38 deletions(-)
drivers/vfio/vfio.c +0 -38
@@ -1175,15 +1175,11 @@ static long vfio_fops_unl_ioctl(struct f
 		ret = vfio_ioctl_set_iommu(container, arg);
 		break;
 	default:
-		down_read(&container->group_lock);
-
 		driver = container->iommu_driver;
 		data = container->iommu_data;
 
 		if (driver) /* passthrough all unrecognized ioctls */
 			ret = driver->ops->ioctl(data, cmd, arg);
-
-		up_read(&container->group_lock);
 	}
 
 	return ret;

@@ -1237,15 +1233,11 @@ static ssize_t vfio_fops_read(struct fil
 	struct vfio_iommu_driver *driver;
 	ssize_t ret = -EINVAL;
 
-	down_read(&container->group_lock);
-
 	driver = container->iommu_driver;
 	if (likely(driver && driver->ops->read))
 		ret = driver->ops->read(container->iommu_data,
 					buf, count, ppos);
 
-	up_read(&container->group_lock);
-
 	return ret;
 }
 

@@ -1256,15 +1248,11 @@ static ssize_t vfio_fops_write(struct fi
 	struct vfio_iommu_driver *driver;
 	ssize_t ret = -EINVAL;
 
-	down_read(&container->group_lock);
-
 	driver = container->iommu_driver;
 	if (likely(driver && driver->ops->write))
 		ret = driver->ops->write(container->iommu_data,
 					 buf, count, ppos);
 
-	up_read(&container->group_lock);
-
 	return ret;
 }
 

@@ -1274,14 +1262,10 @@ static int vfio_fops_mmap(struct file *f
 	struct vfio_iommu_driver *driver;
 	int ret = -EINVAL;
 
-	down_read(&container->group_lock);
-
 	driver = container->iommu_driver;
 	if (likely(driver && driver->ops->mmap))
 		ret = driver->ops->mmap(container->iommu_data, vma);
 
-	up_read(&container->group_lock);
-
 	return ret;
 }
 

@@ -1993,8 +1977,6 @@ int vfio_pin_pages(struct device *dev, u
 		goto err_pin_pages;
 
 	container = group->container;
-	down_read(&container->group_lock);
-
 	driver = container->iommu_driver;
 	if (likely(driver && driver->ops->pin_pages))
 		ret = driver->ops->pin_pages(container->iommu_data, user_pfn,

@@ -2002,7 +1984,6 @@ int vfio_pin_pages(struct device *dev, u
 	else
 		ret = -ENOTTY;
 
-	up_read(&container->group_lock);
 	vfio_group_try_dissolve_container(group);
 
 err_pin_pages:

@@ -2042,8 +2023,6 @@ int vfio_unpin_pages(struct device *dev,
 		goto err_unpin_pages;
 
 	container = group->container;
-	down_read(&container->group_lock);
-
 	driver = container->iommu_driver;
 	if (likely(driver && driver->ops->unpin_pages))
 		ret = driver->ops->unpin_pages(container->iommu_data, user_pfn,

@@ -2051,7 +2030,6 @@ int vfio_unpin_pages(struct device *dev,
 	else
 		ret = -ENOTTY;
 
-	up_read(&container->group_lock);
 	vfio_group_try_dissolve_container(group);
 
 err_unpin_pages:

@@ -2073,8 +2051,6 @@ static int vfio_register_iommu_notifier(
 		return -EINVAL;
 
 	container = group->container;
-	down_read(&container->group_lock);
-
 	driver = container->iommu_driver;
 	if (likely(driver && driver->ops->register_notifier))
 		ret = driver->ops->register_notifier(container->iommu_data,

@@ -2082,7 +2058,6 @@ static int vfio_register_iommu_notifier(
 	else
 		ret = -ENOTTY;
 
-	up_read(&container->group_lock);
 	vfio_group_try_dissolve_container(group);
 
 	return ret;

@@ -2100,8 +2075,6 @@ static int vfio_unregister_iommu_notifie
 		return -EINVAL;
 
 	container = group->container;
-	down_read(&container->group_lock);
-
 	driver = container->iommu_driver;
 	if (likely(driver && driver->ops->unregister_notifier))
 		ret = driver->ops->unregister_notifier(container->iommu_data,

@@ -2109,7 +2082,6 @@ static int vfio_unregister_iommu_notifie
 	else
 		ret = -ENOTTY;
 
-	up_read(&container->group_lock);
 	vfio_group_try_dissolve_container(group);
 
 	return ret;

@@ -2127,7 +2099,6 @@ static int vfio_register_group_notifier(
 					unsigned long *events,
 					struct notifier_block *nb)
 {
-	struct vfio_container *container;
 	int ret;
 	bool set_kvm = false;
 

@@ -2145,9 +2116,6 @@ static int vfio_register_group_notifier(
 	if (ret)
 		return -EINVAL;
 
-	container = group->container;
-	down_read(&container->group_lock);
-
 	ret = blocking_notifier_chain_register(&group->notifier, nb);
 
 	/*

@@ -2158,7 +2126,6 @@ static int vfio_register_group_notifier(
 		blocking_notifier_call_chain(&group->notifier,
 					VFIO_GROUP_NOTIFY_SET_KVM, group->kvm);
 
-	up_read(&container->group_lock);
 	vfio_group_try_dissolve_container(group);
 
 	return ret;

@@ -2167,19 +2134,14 @@ static int vfio_register_group_notifier(
 static int vfio_unregister_group_notifier(struct vfio_group *group,
 					 struct notifier_block *nb)
 {
-	struct vfio_container *container;
 	int ret;
 
 	ret = vfio_group_add_container_user(group);
 	if (ret)
 		return -EINVAL;
 
-	container = group->container;
-	down_read(&container->group_lock);
-
 	ret = blocking_notifier_chain_unregister(&group->notifier, nb);
 
-	up_read(&container->group_lock);
 	vfio_group_try_dissolve_container(group);
 
 	return ret;

Patches currently in stable-queue which might be from [email protected] are

queue-4.12/vfio-new-external-user-group-file-match.patch queue-4.12/vfio-remove-unnecessary-uses-of-vfio_container.group_lock.patch queue-4.12/vfio-fix-group-release-deadlock.patch