linux-kernel - Re: [PATCH v4 3/6] iommu: Introduce mm_get

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20230921190259.GZ13795@ziepe.ca>
Date:   Thu, 21 Sep 2023 16:02:59 -0300
From:   Jason Gunthorpe <jgg@...pe.ca>
To:     Tina Zhang <tina.zhang@...el.com>
Cc:     Kevin Tian <kevin.tian@...el.com>,
        Lu Baolu <baolu.lu@...ux.intel.com>,
        Michael Shavit <mshavit@...gle.com>,
        Vasant Hegde <vasant.hegde@....com>, iommu@...ts.linux.dev,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v4 3/6] iommu: Introduce mm_get_pasid() helper function

On Tue, Sep 12, 2023 at 08:59:33PM +0800, Tina Zhang wrote:
> Use the helper function mm_get_pasid() to get a mm assigned pasid
> value. The motivation is to replace mm->pasid with an iommu private
> data structure that is introduced in a later patch.
> 
> Reviewed-by: Lu Baolu <baolu.lu@...ux.intel.com>
> Signed-off-by: Tina Zhang <tina.zhang@...el.com>
> ---
> 
> Changes in v4:
> - Rebase to v6.6-rc1.
> 
> Changes in v2:
> - Update commit message.
> - Let mm_get_enqcmd_pasid() call mm_get_pasid() to get pasid.
> 
>  drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c | 12 ++++++------
>  drivers/iommu/iommu-sva.c                       | 12 ++++++------
>  include/linux/iommu.h                           | 10 +++++++++-
>  3 files changed, 21 insertions(+), 13 deletions(-)

Michael already has a series to remove this mis-use from the ARM
driver like you did for the Intel driver.

I think for the interm it is fine to just use mm_get_enqcmd_pasid()
and Michael can remove it.

It helps point out this is just wrong anyhow and better discourages
anyone else from using it.

> diff --git a/drivers/iommu/iommu-sva.c b/drivers/iommu/iommu-sva.c
> index b78671a8a914..0f956ecd0c9b 100644
> --- a/drivers/iommu/iommu-sva.c
> +++ b/drivers/iommu/iommu-sva.c
> @@ -23,7 +23,7 @@ static int iommu_sva_alloc_pasid(struct mm_struct *mm, struct device *dev)
>  	mutex_lock(&iommu_sva_lock);
>  	/* Is a PASID already associated with this mm? */
>  	if (mm_valid_pasid(mm)) {
> -		if (mm->pasid >= dev->iommu->max_pasids)
> +		if (mm_get_pasid(mm) >= dev->iommu->max_pasids)
>  			ret = -EOVERFLOW;
>
>  		goto out;
>  	}
>
> @@ -73,7 +73,7 @@ struct iommu_sva *iommu_sva_bind_device(struct device *dev, struct mm_struct *mm
>  
>  	mutex_lock(&iommu_sva_lock);
>  	/* Search for an existing domain. */
> -	domain = iommu_get_domain_for_dev_pasid(dev, mm->pasid,
> +	domain = iommu_get_domain_for_dev_pasid(dev, mm_get_pasid(mm),
>  						IOMMU_DOMAIN_SVA);
>  	if (IS_ERR(domain)) {
>  		ret = PTR_ERR(domain);
> @@ -92,7 +92,7 @@ struct iommu_sva *iommu_sva_bind_device(struct device *dev, struct mm_struct *mm
>  		goto out_unlock;
>  	}
>  
> -	ret = iommu_attach_device_pasid(domain, dev, mm->pasid);
> +	ret = iommu_attach_device_pasid(domain, dev, mm_get_pasid(mm));
>  	if (ret)
>  		goto out_free_domain;
>  	domain->users = 1;

These ones are all manipulating the enqcmd_pasid really..  If it
wasn't for enqcmd then we'd simply make each device have its own PASID
and then we wouldn't have the issue that a valid pasid is incompatible
with a certain device.

Jason