| 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: <20240912082512.ydzmmlwpvkcukbt2@joelS2.panther.com>
Date: Thu, 12 Sep 2024 10:25:12 +0200
From: Joel Granados <j.granados@...sung.com>
To: Baolu Lu <baolu.lu@...ux.intel.com>
CC: David Woodhouse <dwmw2@...radead.org>, Joerg Roedel <joro@...tes.org>,
Will Deacon <will@...nel.org>, Robin Murphy <robin.murphy@....com>, Jason
Gunthorpe <jgg@...pe.ca>, Kevin Tian <kevin.tian@...el.com>, Klaus Jensen
<its@...elevant.dk>, <linux-kernel@...r.kernel.org>, <iommu@...ts.linux.dev>
Subject: Re: [PATCH 5/6] iommu: init pasid array while doing domain_replace
and iopf is active
On Thu, Sep 12, 2024 at 12:21:53PM +0800, Baolu Lu wrote:
> On 9/11/24 6:56 PM, Joel Granados wrote:
> > On Thu, Sep 05, 2024 at 11:30:05AM +0800, Baolu Lu wrote:
> >> On 9/4/24 9:17 PM, Joel Granados via B4 Relay wrote:
> >>> From: Joel Granados<j.granados@...sung.com>
> >>>
> >>> iommu_report_device_fault expects a pasid array to have an
> >>> iommu_attach_handle when a fault is detected.
> >> The iommu_attach_handle is expected only when an iopf-capable domain is
> >> attached to the device or PASID. The iommu_report_device_fault() treats
> >> it as a fault when a fault occurs, but no iopf-capable domain is
> >> attached.
> >>
> >>> Add this handle when the
> >>> replacing hwpt has a valid iommufd fault object. Remove it when we
> >>> release ownership of the group.
> >> The iommu_attach_handle is managed by the caller (iommufd here for
> >> example). Therefore, before iommu_attach_handle tries to attach a domain
> >> to an iopf-capable device or pasid, it should allocate the handle and
> >> pass it to the domain attachment interfaces.
> > What do you mean here?
> > 1. Do you want to move the iommufd_init_pasid_array call up to
> > iommufd_hwpt_replace_device?
> > 2. Do you want to move it further up to iommufd_device_do_replace?
>
> I'm having trouble understanding the need for iommu_init_pasid_array()
> in the core.
The iommu_init_pasid_array function is there because I needed to access
struct iommu_group->pasid_array. The struct declaration for iommu_group
is in the core (drivers/iommu/iommu.c), so I thought that the function
to modify a member of that struct would go in core also.
To move all the logic into iommufd_init_pasid_array in iommufd, we would
need to move the iommu_group struct (include/linux/iommu.h?). Here is a
proposed patch that would make that happen (and removes
iommu_init_pasid_array). What do you think?
diff --git i/drivers/iommu/iommu-priv.h w/drivers/iommu/iommu-priv.h
index 493e501badc7..de5b54eaa8bf 100644
--- i/drivers/iommu/iommu-priv.h
+++ w/drivers/iommu/iommu-priv.h
@@ -38,9 +38,6 @@ void iommu_device_unregister_bus(struct iommu_device *iommu,
struct iommu_attach_handle *iommu_attach_handle_get(struct iommu_group *group,
ioasid_t pasid,
unsigned int type);
-int iommu_init_pasid_array(struct iommu_domain *domain,
- struct iommu_group *group,
- struct iommu_attach_handle *handle);
int iommu_attach_group_handle(struct iommu_domain *domain,
struct iommu_group *group,
struct iommu_attach_handle *handle);
diff --git i/drivers/iommu/iommu.c w/drivers/iommu/iommu.c
index 64ae1cf571aa..83bc0bfd7bb0 100644
--- i/drivers/iommu/iommu.c
+++ w/drivers/iommu/iommu.c
@@ -44,24 +44,6 @@ static unsigned int iommu_def_domain_type __read_mostly;
static bool iommu_dma_strict __read_mostly = IS_ENABLED(CONFIG_IOMMU_DEFAULT_DMA_STRICT);
static u32 iommu_cmd_line __read_mostly;
-struct iommu_group {
- struct kobject kobj;
- struct kobject *devices_kobj;
- struct list_head devices;
- struct xarray pasid_array;
- struct mutex mutex;
- void *iommu_data;
- void (*iommu_data_release)(void *iommu_data);
- char *name;
- int id;
- struct iommu_domain *default_domain;
- struct iommu_domain *blocking_domain;
- struct iommu_domain *domain;
- struct list_head entry;
- unsigned int owner_cnt;
- void *owner;
-};
-
struct group_device {
struct list_head list;
struct device *dev;
@@ -3498,33 +3480,6 @@ iommu_attach_handle_get(struct iommu_group *group, ioasid_t pasid, unsigned int
}
EXPORT_SYMBOL_NS_GPL(iommu_attach_handle_get, IOMMUFD_INTERNAL);
-/**
- * iommu_init_pasid_array - Initialize pasid array in the domain group
- *
- * Returns 0 on success. Error code on failure
- *
- * An IOMMU_NO_PASID element is *NOT* replaced if there is one already there.
- */
-int iommu_init_pasid_array(struct iommu_domain *domain,
- struct iommu_group *group,
- struct iommu_attach_handle *handle)
-{
- int ret;
-
- if (handle)
- handle->domain = domain;
-
- mutex_lock(&group->mutex);
- ret = xa_insert(&group->pasid_array, IOMMU_NO_PASID, handle, GFP_KERNEL);
- mutex_unlock(&group->mutex);
-
- if (ret == -EBUSY)
- ret = 0;
-
- return ret;
-}
-EXPORT_SYMBOL_NS_GPL(iommu_init_pasid_array, IOMMUFD_INTERNAL);
-
/**
* iommu_attach_group_handle - Attach an IOMMU domain to an IOMMU group
* @domain: IOMMU domain to attach
diff --git i/drivers/iommu/iommufd/fault.c w/drivers/iommu/iommufd/fault.c
index ea7f1bf64892..51cb70465b87 100644
--- i/drivers/iommu/iommufd/fault.c
+++ w/drivers/iommu/iommufd/fault.c
@@ -189,8 +189,15 @@ static int iommufd_init_pasid_array(struct iommu_domain *domain,
if (!handle)
return -ENOMEM;
handle->idev = idev;
+ handle->handle.domain = domain;
+
+ mutex_lock(&group->mutex);
+ ret = xa_insert(&group->pasid_array, IOMMU_NO_PASID, handle, GFP_KERNEL);
+ mutex_unlock(&group->mutex);
+
+ if (ret == -EBUSY)
+ ret = 0;
- ret = iommu_init_pasid_array(domain, group, &handle->handle);
if (ret)
kfree(handle);
diff --git i/include/linux/iommu.h w/include/linux/iommu.h
index bd722f473635..c34aa737aeb2 100644
--- i/include/linux/iommu.h
+++ w/include/linux/iommu.h
@@ -31,8 +31,25 @@
*/
#define IOMMU_PRIV (1 << 5)
+struct iommu_group {
+ struct kobject kobj;
+ struct kobject *devices_kobj;
+ struct list_head devices;
+ struct xarray pasid_array;
+ struct mutex mutex;
+ void *iommu_data;
+ void (*iommu_data_release)(void *iommu_data);
+ char *name;
+ int id;
+ struct iommu_domain *default_domain;
+ struct iommu_domain *blocking_domain;
+ struct iommu_domain *domain;
+ struct list_head entry;
+ unsigned int owner_cnt;
+ void *owner;
+};
+
struct iommu_ops;
-struct iommu_group;
struct bus_type;
struct device;
struct iommu_domain;
--
Joel Granados
Powered by blists - more mailing lists