| 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
| ||
|
Message-ID: <d1023a09ab3d4c69bc45095a733ad15ef000f373.1677288789.git.nicolinc@nvidia.com>
Date: Fri, 24 Feb 2023 17:51:39 -0800
From: Nicolin Chen <nicolinc@...dia.com>
To: <jgg@...dia.com>, <kevin.tian@...el.com>, <joro@...tes.org>,
<will@...nel.org>, <robin.murphy@....com>,
<alex.williamson@...hat.com>, <shuah@...nel.org>
CC: <yi.l.liu@...el.com>, <linux-kernel@...r.kernel.org>,
<iommu@...ts.linux.dev>, <kvm@...r.kernel.org>,
<linux-kselftest@...r.kernel.org>, <mjrosato@...ux.ibm.com>,
<farman@...ux.ibm.com>
Subject: [PATCH v3 2/5] iommufd: Create access in vfio_iommufd_emulated_bind()
To prepare for an access->ioas replacement, move iommufd_access_create()
call into vfio_iommufd_emulated_bind(), making it symmetric with the
__vfio_iommufd_access_destroy() call in vfio_iommufd_emulated_unbind().
This means an access is created/destroyed by the bind()/unbind(), and the
vfio_iommufd_emulated_attach_ioas() only updates the access->ioas pointer.
Since there's no longer an ioas_id input for iommufd_access_create(), add
a new helper iommufd_access_set_ioas() to set access->ioas. We can later
add a "replace" feature simply to the new iommufd_access_set_ioas() too.
Leaving the access->ioas in vfio_iommufd_emulated_attach_ioas(), however,
can introduce some potential of a race condition during pin_/unpin_pages()
call where access->ioas->iopt is getting referenced. So, add an ioas_lock
to protect it.
Reviewed-by: Kevin Tian <kevin.tian@...el.com>
Signed-off-by: Nicolin Chen <nicolinc@...dia.com>
---
drivers/iommu/iommufd/device.c | 103 ++++++++++++++++++------
drivers/iommu/iommufd/iommufd_private.h | 1 +
drivers/iommu/iommufd/selftest.c | 5 +-
drivers/vfio/iommufd.c | 23 +++---
include/linux/iommufd.h | 3 +-
5 files changed, 100 insertions(+), 35 deletions(-)
diff --git a/drivers/iommu/iommufd/device.c b/drivers/iommu/iommufd/device.c
index 6ba8633b699d..a955ebd4bd5d 100644
--- a/drivers/iommu/iommufd/device.c
+++ b/drivers/iommu/iommufd/device.c
@@ -618,9 +618,12 @@ void iommufd_access_destroy_object(struct iommufd_object *obj)
struct iommufd_access *access =
container_of(obj, struct iommufd_access, obj);
- iopt_remove_access(&access->ioas->iopt, access);
+ if (access->ioas) {
+ iopt_remove_access(&access->ioas->iopt, access);
+ refcount_dec(&access->ioas->obj.users);
+ }
iommufd_ctx_put(access->ictx);
- refcount_dec(&access->ioas->obj.users);
+ mutex_destroy(&access->ioas_lock);
}
/**
@@ -637,12 +640,10 @@ void iommufd_access_destroy_object(struct iommufd_object *obj)
* The provided ops are required to use iommufd_access_pin_pages().
*/
struct iommufd_access *
-iommufd_access_create(struct iommufd_ctx *ictx, u32 ioas_id,
+iommufd_access_create(struct iommufd_ctx *ictx,
const struct iommufd_access_ops *ops, void *data)
{
struct iommufd_access *access;
- struct iommufd_object *obj;
- int rc;
/*
* There is no uAPI for the access object, but to keep things symmetric
@@ -655,33 +656,18 @@ iommufd_access_create(struct iommufd_ctx *ictx, u32 ioas_id,
access->data = data;
access->ops = ops;
- obj = iommufd_get_object(ictx, ioas_id, IOMMUFD_OBJ_IOAS);
- if (IS_ERR(obj)) {
- rc = PTR_ERR(obj);
- goto out_abort;
- }
- access->ioas = container_of(obj, struct iommufd_ioas, obj);
- iommufd_ref_to_users(obj);
-
if (ops->needs_pin_pages)
access->iova_alignment = PAGE_SIZE;
else
access->iova_alignment = 1;
- rc = iopt_add_access(&access->ioas->iopt, access);
- if (rc)
- goto out_put_ioas;
/* The calling driver is a user until iommufd_access_destroy() */
refcount_inc(&access->obj.users);
+ mutex_init(&access->ioas_lock);
access->ictx = ictx;
iommufd_ctx_get(ictx);
iommufd_object_finalize(ictx, &access->obj);
return access;
-out_put_ioas:
- refcount_dec(&access->ioas->obj.users);
-out_abort:
- iommufd_object_abort(ictx, &access->obj);
- return ERR_PTR(rc);
}
EXPORT_SYMBOL_NS_GPL(iommufd_access_create, IOMMUFD);
@@ -700,6 +686,50 @@ void iommufd_access_destroy(struct iommufd_access *access)
}
EXPORT_SYMBOL_NS_GPL(iommufd_access_destroy, IOMMUFD);
+int iommufd_access_set_ioas(struct iommufd_access *access, u32 ioas_id)
+{
+ struct iommufd_ioas *new_ioas = NULL, *cur_ioas;
+ struct iommufd_ctx *ictx = access->ictx;
+ struct iommufd_object *obj;
+ int rc = 0;
+
+ if (ioas_id) {
+ obj = iommufd_get_object(ictx, ioas_id, IOMMUFD_OBJ_IOAS);
+ if (IS_ERR(obj))
+ return PTR_ERR(obj);
+ new_ioas = container_of(obj, struct iommufd_ioas, obj);
+ }
+
+ mutex_lock(&access->ioas_lock);
+ cur_ioas = access->ioas;
+ if (cur_ioas == new_ioas)
+ goto out_unlock;
+
+ if (new_ioas) {
+ rc = iopt_add_access(&new_ioas->iopt, access);
+ if (rc)
+ goto out_unlock;
+ iommufd_ref_to_users(obj);
+ }
+
+ if (cur_ioas) {
+ iopt_remove_access(&cur_ioas->iopt, access);
+ refcount_dec(&cur_ioas->obj.users);
+ }
+
+ access->ioas = new_ioas;
+ mutex_unlock(&access->ioas_lock);
+
+ return 0;
+
+out_unlock:
+ mutex_unlock(&access->ioas_lock);
+ if (new_ioas)
+ iommufd_put_object(obj);
+ return rc;
+}
+EXPORT_SYMBOL_NS_GPL(iommufd_access_set_ioas, IOMMUFD);
+
/**
* iommufd_access_notify_unmap - Notify users of an iopt to stop using it
* @iopt: iopt to work on
@@ -750,8 +780,8 @@ void iommufd_access_notify_unmap(struct io_pagetable *iopt, unsigned long iova,
void iommufd_access_unpin_pages(struct iommufd_access *access,
unsigned long iova, unsigned long length)
{
- struct io_pagetable *iopt = &access->ioas->iopt;
struct iopt_area_contig_iter iter;
+ struct io_pagetable *iopt;
unsigned long last_iova;
struct iopt_area *area;
@@ -759,6 +789,13 @@ void iommufd_access_unpin_pages(struct iommufd_access *access,
WARN_ON(check_add_overflow(iova, length - 1, &last_iova)))
return;
+ mutex_lock(&access->ioas_lock);
+ if (!access->ioas) {
+ mutex_unlock(&access->ioas_lock);
+ return;
+ }
+ iopt = &access->ioas->iopt;
+
down_read(&iopt->iova_rwsem);
iopt_for_each_contig_area(&iter, area, iopt, iova, last_iova)
iopt_area_remove_access(
@@ -768,6 +805,7 @@ void iommufd_access_unpin_pages(struct iommufd_access *access,
min(last_iova, iopt_area_last_iova(area))));
up_read(&iopt->iova_rwsem);
WARN_ON(!iopt_area_contig_done(&iter));
+ mutex_unlock(&access->ioas_lock);
}
EXPORT_SYMBOL_NS_GPL(iommufd_access_unpin_pages, IOMMUFD);
@@ -813,8 +851,8 @@ int iommufd_access_pin_pages(struct iommufd_access *access, unsigned long iova,
unsigned long length, struct page **out_pages,
unsigned int flags)
{
- struct io_pagetable *iopt = &access->ioas->iopt;
struct iopt_area_contig_iter iter;
+ struct io_pagetable *iopt;
unsigned long last_iova;
struct iopt_area *area;
int rc;
@@ -829,6 +867,13 @@ int iommufd_access_pin_pages(struct iommufd_access *access, unsigned long iova,
if (check_add_overflow(iova, length - 1, &last_iova))
return -EOVERFLOW;
+ mutex_lock(&access->ioas_lock);
+ if (!access->ioas) {
+ mutex_unlock(&access->ioas_lock);
+ return -ENOENT;
+ }
+ iopt = &access->ioas->iopt;
+
down_read(&iopt->iova_rwsem);
iopt_for_each_contig_area(&iter, area, iopt, iova, last_iova) {
unsigned long last = min(last_iova, iopt_area_last_iova(area));
@@ -859,6 +904,7 @@ int iommufd_access_pin_pages(struct iommufd_access *access, unsigned long iova,
}
up_read(&iopt->iova_rwsem);
+ mutex_unlock(&access->ioas_lock);
return 0;
err_remove:
@@ -873,6 +919,7 @@ int iommufd_access_pin_pages(struct iommufd_access *access, unsigned long iova,
iopt_area_last_iova(area))));
}
up_read(&iopt->iova_rwsem);
+ mutex_unlock(&access->ioas_lock);
return rc;
}
EXPORT_SYMBOL_NS_GPL(iommufd_access_pin_pages, IOMMUFD);
@@ -892,8 +939,8 @@ EXPORT_SYMBOL_NS_GPL(iommufd_access_pin_pages, IOMMUFD);
int iommufd_access_rw(struct iommufd_access *access, unsigned long iova,
void *data, size_t length, unsigned int flags)
{
- struct io_pagetable *iopt = &access->ioas->iopt;
struct iopt_area_contig_iter iter;
+ struct io_pagetable *iopt;
struct iopt_area *area;
unsigned long last_iova;
int rc;
@@ -903,6 +950,13 @@ int iommufd_access_rw(struct iommufd_access *access, unsigned long iova,
if (check_add_overflow(iova, length - 1, &last_iova))
return -EOVERFLOW;
+ mutex_lock(&access->ioas_lock);
+ if (!access->ioas) {
+ mutex_unlock(&access->ioas_lock);
+ return -ENOENT;
+ }
+ iopt = &access->ioas->iopt;
+
down_read(&iopt->iova_rwsem);
iopt_for_each_contig_area(&iter, area, iopt, iova, last_iova) {
unsigned long last = min(last_iova, iopt_area_last_iova(area));
@@ -929,6 +983,7 @@ int iommufd_access_rw(struct iommufd_access *access, unsigned long iova,
rc = -ENOENT;
err_out:
up_read(&iopt->iova_rwsem);
+ mutex_unlock(&access->ioas_lock);
return rc;
}
EXPORT_SYMBOL_NS_GPL(iommufd_access_rw, IOMMUFD);
diff --git a/drivers/iommu/iommufd/iommufd_private.h b/drivers/iommu/iommufd/iommufd_private.h
index c9acc70d84f7..94e88377a7fc 100644
--- a/drivers/iommu/iommufd/iommufd_private.h
+++ b/drivers/iommu/iommufd/iommufd_private.h
@@ -311,6 +311,7 @@ struct iommufd_access {
struct iommufd_object obj;
struct iommufd_ctx *ictx;
struct iommufd_ioas *ioas;
+ struct mutex ioas_lock;
const struct iommufd_access_ops *ops;
void *data;
unsigned long iova_alignment;
diff --git a/drivers/iommu/iommufd/selftest.c b/drivers/iommu/iommufd/selftest.c
index 51ea09af5778..dacd1f67957d 100644
--- a/drivers/iommu/iommufd/selftest.c
+++ b/drivers/iommu/iommufd/selftest.c
@@ -754,7 +754,7 @@ static int iommufd_test_create_access(struct iommufd_ucmd *ucmd,
}
access = iommufd_access_create(
- ucmd->ictx, ioas_id,
+ ucmd->ictx,
(flags & MOCK_FLAGS_ACCESS_CREATE_NEEDS_PIN_PAGES) ?
&selftest_access_ops_pin :
&selftest_access_ops,
@@ -763,6 +763,9 @@ static int iommufd_test_create_access(struct iommufd_ucmd *ucmd,
rc = PTR_ERR(access);
goto out_put_fdno;
}
+ rc = iommufd_access_set_ioas(access, ioas_id);
+ if (rc)
+ goto out_destroy;
cmd->create_access.out_access_fd = fdno;
rc = iommufd_ucmd_respond(ucmd, sizeof(*cmd));
if (rc)
diff --git a/drivers/vfio/iommufd.c b/drivers/vfio/iommufd.c
index db4efbd56042..1f87294c1931 100644
--- a/drivers/vfio/iommufd.c
+++ b/drivers/vfio/iommufd.c
@@ -138,10 +138,18 @@ static const struct iommufd_access_ops vfio_user_ops = {
int vfio_iommufd_emulated_bind(struct vfio_device *vdev,
struct iommufd_ctx *ictx, u32 *out_device_id)
{
+ struct iommufd_access *user;
+
lockdep_assert_held(&vdev->dev_set->lock);
- vdev->iommufd_ictx = ictx;
iommufd_ctx_get(ictx);
+ user = iommufd_access_create(ictx, &vfio_user_ops, vdev);
+ if (IS_ERR(user)) {
+ iommufd_ctx_put(ictx);
+ return PTR_ERR(user);
+ }
+ vdev->iommufd_access = user;
+ vdev->iommufd_ictx = ictx;
return 0;
}
EXPORT_SYMBOL_GPL(vfio_iommufd_emulated_bind);
@@ -161,15 +169,12 @@ EXPORT_SYMBOL_GPL(vfio_iommufd_emulated_unbind);
int vfio_iommufd_emulated_attach_ioas(struct vfio_device *vdev, u32 *pt_id)
{
- struct iommufd_access *user;
-
lockdep_assert_held(&vdev->dev_set->lock);
- user = iommufd_access_create(vdev->iommufd_ictx, *pt_id, &vfio_user_ops,
- vdev);
- if (IS_ERR(user))
- return PTR_ERR(user);
- vdev->iommufd_access = user;
- return 0;
+ if (!vdev->iommufd_ictx)
+ return -EINVAL;
+ if (!vdev->iommufd_access)
+ return -ENOENT;
+ return iommufd_access_set_ioas(vdev->iommufd_access, *pt_id);
}
EXPORT_SYMBOL_GPL(vfio_iommufd_emulated_attach_ioas);
diff --git a/include/linux/iommufd.h b/include/linux/iommufd.h
index 3044a432a83e..f20f63e0b066 100644
--- a/include/linux/iommufd.h
+++ b/include/linux/iommufd.h
@@ -41,9 +41,10 @@ enum {
};
struct iommufd_access *
-iommufd_access_create(struct iommufd_ctx *ictx, u32 ioas_id,
+iommufd_access_create(struct iommufd_ctx *ictx,
const struct iommufd_access_ops *ops, void *data);
void iommufd_access_destroy(struct iommufd_access *access);
+int iommufd_access_set_ioas(struct iommufd_access *access, u32 ioas_id);
void iommufd_ctx_get(struct iommufd_ctx *ictx);
--
2.39.2
Powered by blists - more mailing lists