| 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: <2872aa9f-c325-ca28-fb64-f86857ad3e91@linux.intel.com>
Date: Tue, 7 Dec 2021 10:07:16 +0800
From: Lu Baolu <baolu.lu@...ux.intel.com>
To: Christoph Hellwig <hch@...radead.org>
Cc: baolu.lu@...ux.intel.com,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
Joerg Roedel <joro@...tes.org>,
Alex Williamson <alex.williamson@...hat.com>,
Bjorn Helgaas <bhelgaas@...gle.com>,
Jason Gunthorpe <jgg@...dia.com>,
Kevin Tian <kevin.tian@...el.com>,
Ashok Raj <ashok.raj@...el.com>, Will Deacon <will@...nel.org>,
Robin Murphy <robin.murphy@....com>,
Dan Williams <dan.j.williams@...el.com>, rafael@...nel.org,
Diana Craciun <diana.craciun@....nxp.com>,
Cornelia Huck <cohuck@...hat.com>,
Eric Auger <eric.auger@...hat.com>,
Liu Yi L <yi.l.liu@...el.com>,
Jacob jun Pan <jacob.jun.pan@...el.com>,
Chaitanya Kulkarni <kch@...dia.com>,
Stuart Yoder <stuyoder@...il.com>,
Laurentiu Tudor <laurentiu.tudor@....com>,
Thierry Reding <thierry.reding@...il.com>,
David Airlie <airlied@...ux.ie>,
Daniel Vetter <daniel@...ll.ch>,
Jonathan Hunter <jonathanh@...dia.com>,
Li Yang <leoyang.li@....com>,
Dmitry Osipenko <digetx@...il.com>,
iommu@...ts.linux-foundation.org, linux-pci@...r.kernel.org,
kvm@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 01/18] iommu: Add device dma ownership set/release
interfaces
On 12/6/21 10:42 PM, Christoph Hellwig wrote:
> On Mon, Dec 06, 2021 at 09:58:46AM +0800, Lu Baolu wrote:
>> >From the perspective of who is initiating the device to do DMA, device
>> DMA could be divided into the following types:
>>
>> DMA_OWNER_DMA_API: Device DMAs are initiated by a kernel driver
>> through the kernel DMA API.
>> DMA_OWNER_PRIVATE_DOMAIN: Device DMAs are initiated by a kernel
>> driver with its own PRIVATE domain.
>> DMA_OWNER_PRIVATE_DOMAIN_USER: Device DMAs are initiated by
>> userspace.
>>
>> Different DMA ownerships are exclusive for all devices in the same iommu
>> group as an iommu group is the smallest granularity of device isolation
>> and protection that the IOMMU subsystem can guarantee. This extends the
>> iommu core to enforce this exclusion.
>>
>> Basically two new interfaces are provided:
>>
>> int iommu_device_set_dma_owner(struct device *dev,
>> enum iommu_dma_owner type, void *owner_cookie);
>> void iommu_device_release_dma_owner(struct device *dev,
>> enum iommu_dma_owner type);
>>
>> Although above interfaces are per-device, DMA owner is tracked per group
>> under the hood. An iommu group cannot have different dma ownership set
>> at the same time. Violation of this assumption fails
>> iommu_device_set_dma_owner().
>>
>> Kernel driver which does DMA have DMA_OWNER_DMA_API automatically set/
>> released in the driver binding/unbinding process (see next patch).
>>
>> Kernel driver which doesn't do DMA could avoid setting the owner type.
>> Device bound to such driver is considered same as a driver-less device
>> which is compatible to all owner types.
>>
>> Userspace driver framework (e.g. vfio) should set
>> DMA_OWNER_PRIVATE_DOMAIN_USER for a device before the userspace is allowed
>> to access it, plus a owner cookie pointer to mark the user identity so a
>> single group cannot be operated by multiple users simultaneously. Vice
>> versa, the owner type should be released after the user access permission
>> is withdrawn.
>>
>> Signed-off-by: Jason Gunthorpe <jgg@...dia.com>
>> Signed-off-by: Kevin Tian <kevin.tian@...el.com>
>> Signed-off-by: Lu Baolu <baolu.lu@...ux.intel.com>
>> ---
>> include/linux/iommu.h | 36 +++++++++++++++++
>> drivers/iommu/iommu.c | 93 +++++++++++++++++++++++++++++++++++++++++++
>> 2 files changed, 129 insertions(+)
>>
>> diff --git a/include/linux/iommu.h b/include/linux/iommu.h
>> index d2f3435e7d17..24676b498f38 100644
>> --- a/include/linux/iommu.h
>> +++ b/include/linux/iommu.h
>> @@ -162,6 +162,23 @@ enum iommu_dev_features {
>> IOMMU_DEV_FEAT_IOPF,
>> };
>>
>> +/**
>> + * enum iommu_dma_owner - IOMMU DMA ownership
>> + * @DMA_OWNER_NONE: No DMA ownership.
>> + * @DMA_OWNER_DMA_API: Device DMAs are initiated by a kernel driver through
>> + * the kernel DMA API.
>> + * @DMA_OWNER_PRIVATE_DOMAIN: Device DMAs are initiated by a kernel driver
>> + * which provides an UNMANAGED domain.
>> + * @DMA_OWNER_PRIVATE_DOMAIN_USER: Device DMAs are initiated by userspace,
>> + * kernel ensures that DMAs never go to kernel memory.
>> + */
>> +enum iommu_dma_owner {
>> + DMA_OWNER_NONE,
>> + DMA_OWNER_DMA_API,
>> + DMA_OWNER_PRIVATE_DOMAIN,
>> + DMA_OWNER_PRIVATE_DOMAIN_USER,
>> +};
>> +
>> #define IOMMU_PASID_INVALID (-1U)
>>
>> #ifdef CONFIG_IOMMU_API
>> @@ -681,6 +698,10 @@ struct iommu_sva *iommu_sva_bind_device(struct device *dev,
>> void iommu_sva_unbind_device(struct iommu_sva *handle);
>> u32 iommu_sva_get_pasid(struct iommu_sva *handle);
>>
>> +int iommu_device_set_dma_owner(struct device *dev, enum iommu_dma_owner owner,
>> + void *owner_cookie);
>> +void iommu_device_release_dma_owner(struct device *dev, enum iommu_dma_owner owner);
>> +
>> #else /* CONFIG_IOMMU_API */
>>
>> struct iommu_ops {};
>> @@ -1081,6 +1102,21 @@ static inline struct iommu_fwspec *dev_iommu_fwspec_get(struct device *dev)
>> {
>> return NULL;
>> }
>> +
>> +static inline int iommu_device_set_dma_owner(struct device *dev,
>> + enum iommu_dma_owner owner,
>> + void *owner_cookie)
>> +{
>> + if (owner != DMA_OWNER_DMA_API)
>> + return -EINVAL;
>> +
>> + return 0;
>> +}
>> +
>> +static inline void iommu_device_release_dma_owner(struct device *dev,
>> + enum iommu_dma_owner owner)
>> +{
>> +}
>> #endif /* CONFIG_IOMMU_API */
>>
>> /**
>> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
>> index 8b86406b7162..1de520a07518 100644
>> --- a/drivers/iommu/iommu.c
>> +++ b/drivers/iommu/iommu.c
>> @@ -48,6 +48,9 @@ struct iommu_group {
>> struct iommu_domain *default_domain;
>> struct iommu_domain *domain;
>> struct list_head entry;
>> + enum iommu_dma_owner dma_owner;
>> + refcount_t owner_cnt;
>
> owner_cnt is only manipulated under group->mutex, not need for a
> refcount_t here, a plain unsigned int while do it and will also
> simplify a fair bit of code as it avoid the need for atomic add/sub
> and test operations.
Fair enough.
>
>> +static int __iommu_group_set_dma_owner(struct iommu_group *group,
>> + enum iommu_dma_owner owner,
>> + void *owner_cookie)
>> +{
>
> As pointed out last time, please move the group->mutex locking into
> this helper, which makes it identical to the later added public
> function.
I didn't mean to ignore your comment. :-) As I replied, by placing the
lock out of the function, the helper could easily handle the error paths
(return directly without something like "goto out_unlock").
As the implementation of iommu_group_set_dma_owner() has been greatly
simplified, I agree with you now, we should move the group->mutex
locking into the helper and make it identical to the latter public
interface.
I will work towards this.
>
>> +static void __iommu_group_release_dma_owner(struct iommu_group *group,
>> + enum iommu_dma_owner owner)
>> +{
>
> Same here.
>
Ditto.
Best regards,
baolu
Powered by blists - more mailing lists