On Mon, Jul 24, 2023 at 12:47:05PM -0700, Nicolin Chen wrote:
> -int iommufd_access_attach(struct iommufd_access *access, u32 ioas_id)
> +static int iommufd_access_change_pt(struct iommufd_access *access, u32 ioas_id)
> {
> + struct iommufd_ioas *cur_ioas = access->ioas;
> struct iommufd_ioas *new_ioas;
> - int rc = 0;
> + int rc;
>
> - mutex_lock(&access->ioas_lock);
> - if (WARN_ON(access->ioas || access->ioas_unpin)) {
> - mutex_unlock(&access->ioas_lock);
> - return -EINVAL;
> - }
> + lockdep_assert_held(&access->ioas_lock);
>
> new_ioas = iommufd_get_ioas(access->ictx, ioas_id);
> - if (IS_ERR(new_ioas)) {
> - mutex_unlock(&access->ioas_lock);
> + if (IS_ERR(new_ioas))
> return PTR_ERR(new_ioas);
> - }
> +
> + if (cur_ioas)
> + __iommufd_access_detach(access);
The drop of the mutex while this function runs is racey with the rest
of this, we can mitigate it by blocking concurrent change while
detaching which is if access->ioas_unpin is set
> rc = iopt_add_access(&new_ioas->iopt, access);
> if (rc) {
> - mutex_unlock(&access->ioas_lock);
> iommufd_put_object(&new_ioas->obj);
> + if (cur_ioas)
> + WARN_ON(iommufd_access_change_pt(access,
> + cur_ioas->obj.id));
We've already dropped our ref to cur_ioas, so this is also racy with
destroy.
This is what I came up with:
diff --git a/drivers/iommu/iommufd/device.c b/drivers/iommu/iommufd/device.c
index 57c0e81f5073b2..e55d6e902edb98 100644
--- a/drivers/iommu/iommufd/device.c
+++ b/drivers/iommu/iommufd/device.c
@@ -758,64 +758,101 @@ void iommufd_access_destroy(struct iommufd_access *access)
}
EXPORT_SYMBOL_NS_GPL(iommufd_access_destroy, IOMMUFD);
-void iommufd_access_detach(struct iommufd_access *access)
+static int iommufd_access_change_ioas(struct iommufd_access *access,
+ struct iommufd_ioas *new_ioas)
{
struct iommufd_ioas *cur_ioas = access->ioas;
+ int rc;
+
+ lockdep_assert_held(&access->ioas_lock);
+
+ /* We are racing with a concurrent detach, bail */
+ if (access->ioas_unpin)
+ return -EBUSY;
+
+ if (IS_ERR(new_ioas))
+ return PTR_ERR(new_ioas);
+
+ if (cur_ioas == new_ioas)
+ return 0;
- mutex_lock(&access->ioas_lock);
- if (WARN_ON(!access->ioas))
- goto out;
/*
* Set ioas to NULL to block any further iommufd_access_pin_pages().
* iommufd_access_unpin_pages() can continue using access->ioas_unpin.
*/
access->ioas = NULL;
-
- if (access->ops->unmap) {
+ if (cur_ioas && access->ops->unmap) {
mutex_unlock(&access->ioas_lock);
access->ops->unmap(access->data, 0, ULONG_MAX);
mutex_lock(&access->ioas_lock);
}
+
+ if (new_ioas) {
+ rc = iopt_add_access(&new_ioas->iopt, access);
+ if (rc) {
+ iommufd_put_object(&new_ioas->obj);
+ access->ioas = cur_ioas;
+ return rc;
+ }
+ iommufd_ref_to_users(&new_ioas->obj);
+ }
+
+ access->ioas = new_ioas;
+ access->ioas_unpin = new_ioas;
iopt_remove_access(&cur_ioas->iopt, access);
refcount_dec(&cur_ioas->obj.users);
-out:
- access->ioas_unpin = NULL;
+
+ return 0;
+}
+
+void iommufd_access_detach(struct iommufd_access *access)
+{
+ int rc;
+
+ mutex_lock(&access->ioas_lock);
+ if (WARN_ON(!access->ioas)) {
+ mutex_unlock(&access->ioas_lock);
+ return;
+ }
+ rc = iommufd_access_change_ioas(access, NULL);
+ WARN_ON(rc);
mutex_unlock(&access->ioas_lock);
}
EXPORT_SYMBOL_NS_GPL(iommufd_access_detach, IOMMUFD);
int iommufd_access_attach(struct iommufd_access *access, u32 ioas_id)
{
- struct iommufd_ioas *new_ioas;
- int rc = 0;
+ int rc;
mutex_lock(&access->ioas_lock);
- if (WARN_ON(access->ioas || access->ioas_unpin)) {
+ if (WARN_ON(access->ioas)) {
mutex_unlock(&access->ioas_lock);
return -EINVAL;
}
- new_ioas = iommufd_get_ioas(access->ictx, ioas_id);
- if (IS_ERR(new_ioas)) {
- mutex_unlock(&access->ioas_lock);
- return PTR_ERR(new_ioas);
- }
-
- rc = iopt_add_access(&new_ioas->iopt, access);
- if (rc) {
- mutex_unlock(&access->ioas_lock);
- iommufd_put_object(&new_ioas->obj);
- return rc;
- }
- iommufd_ref_to_users(&new_ioas->obj);
-
- access->ioas = new_ioas;
- access->ioas_unpin = new_ioas;
+ rc = iommufd_access_change_ioas(access,
+ iommufd_get_ioas(access->ictx, ioas_id));
mutex_unlock(&access->ioas_lock);
- return 0;
+ return rc;
}
EXPORT_SYMBOL_NS_GPL(iommufd_access_attach, IOMMUFD);
+int iommufd_access_replace(struct iommufd_access *access, u32 ioas_id)
+{
+ int rc;
+
+ mutex_lock(&access->ioas_lock);
+ if (!access->ioas) {
+ mutex_unlock(&access->ioas_lock);
+ return -ENOENT;
+ }
+ rc = iommufd_access_change_ioas(access,
+ iommufd_get_ioas(access->ictx, ioas_id));
+ mutex_unlock(&access->ioas_lock);
+ return rc;
+}
+EXPORT_SYMBOL_NS_GPL(iommufd_access_replace, IOMMUFD);
+
/**
* iommufd_access_notify_unmap - Notify users of an iopt to stop using it
* @iopt: iopt to work on
diff --git a/include/linux/iommufd.h b/include/linux/iommufd.h
index 0ac60256b65929..ffc3a949f8374f 100644
--- a/include/linux/iommufd.h
+++ b/include/linux/iommufd.h
@@ -49,6 +49,7 @@ iommufd_access_create(struct iommufd_ctx *ictx,
const struct iommufd_access_ops *ops, void *data, u32 *id);
void iommufd_access_destroy(struct iommufd_access *access);
int iommufd_access_attach(struct iommufd_access *access, u32 ioas_id);
+int iommufd_access_replace(struct iommufd_access *access, u32 ioas_id);
void iommufd_access_detach(struct iommufd_access *access);
void iommufd_ctx_get(struct iommufd_ctx *ictx);
Jason
