On Fri, Jun 02, 2023 at 05:16:43AM -0700, Yi Liu wrote:
> From: Nicolin Chen <nicolinc@xxxxxxxxxx>
>
> Previously, the detach routine is only done by the destroy(). And it was
> called by vfio_iommufd_emulated_unbind() when the device runs close(), so
> all the mappings in iopt were cleaned in that setup, when the call trace
> reaches this detach() routine.
>
> Now, there's a need of a detach uAPI, meaning that it does not only need
> a new iommufd_access_detach() API, but also requires access->ops->unmap()
> call as a cleanup. So add one.
>
> However, leaving that unprotected can introduce some potential of a race
> condition during the pin_/unpin_pages() call, where access->ioas->iopt is
> getting referenced. So, add an ioas_lock to protect the context of iopt
> referencings.
>
> Also, to allow the iommufd_access_unpin_pages() callback to happen via
> this unmap() call, add an ioas_unpin pointer, so the unpin routine won't
> be affected by the "access->ioas = NULL" trick.
>
> Reviewed-by: Kevin Tian <kevin.tian@xxxxxxxxx>
> Tested-by: Terrence Xu <terrence.xu@xxxxxxxxx>
> Tested-by: Yanting Jiang <yanting.jiang@xxxxxxxxx>
> Tested-by: Shameer Kolothum <shameerali.kolothum.thodi@xxxxxxxxxx>
> Signed-off-by: Nicolin Chen <nicolinc@xxxxxxxxxx>
> Signed-off-by: Yi Liu <yi.l.liu@xxxxxxxxx>
> ---
> drivers/iommu/iommufd/device.c | 76 +++++++++++++++++++++++--
> drivers/iommu/iommufd/iommufd_private.h | 2 +
> include/linux/iommufd.h | 1 +
> 3 files changed, 74 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/iommu/iommufd/device.c b/drivers/iommu/iommufd/device.c
> index 96d4281bfa7c..6b4ff635c15e 100644
> --- a/drivers/iommu/iommufd/device.c
> +++ b/drivers/iommu/iommufd/device.c
> @@ -486,6 +486,7 @@ iommufd_access_create(struct iommufd_ctx *ictx,
> iommufd_ctx_get(ictx);
> iommufd_object_finalize(ictx, &access->obj);
> *id = access->obj.id;
> + mutex_init(&access->ioas_lock);
> return access;
> }
> EXPORT_SYMBOL_NS_GPL(iommufd_access_create, IOMMUFD);
> @@ -505,26 +506,66 @@ void iommufd_access_destroy(struct iommufd_access *access)
> }
> EXPORT_SYMBOL_NS_GPL(iommufd_access_destroy, IOMMUFD);
>
> +static void __iommufd_access_detach(struct iommufd_access *access)
> +{
> + struct iommufd_ioas *cur_ioas = access->ioas;
> +
> + lockdep_assert_held(&access->ioas_lock);
> + /*
> + * 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) {
> + mutex_unlock(&access->ioas_lock);
> + access->ops->unmap(access->data, 0, ULONG_MAX);
> + mutex_lock(&access->ioas_lock);
> + }
> + iopt_remove_access(&cur_ioas->iopt, access);
> + refcount_dec(&cur_ioas->obj.users);
> +}
> +
> +void iommufd_access_detach(struct iommufd_access *access)
> +{
> + mutex_lock(&access->ioas_lock);
> + if (WARN_ON(!access->ioas))
> + goto out;
> + __iommufd_access_detach(access);
> +out:
> + access->ioas_unpin = NULL;
> + mutex_unlock(&access->ioas_lock);
> +}
> +EXPORT_SYMBOL_NS_GPL(iommufd_access_detach, IOMMUFD);
There is not really any benefit to make this two functions
> int iommufd_access_attach(struct iommufd_access *access, u32 ioas_id)
> {
[..]
> if (access->ioas) {
if (access->ioas || access->ioas_unpin) {
But I wonder if it should be a WARN_ON? Does VFIO protect against
the userspace racing detach and attach, or do we expect to do it here?
> @@ -579,8 +620,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;
>
> @@ -588,6 +629,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_unpin) {
This should be WARN_ON(), the driver has done something wrong if we
call this after the access has been detached.
Jason
