linux-kernel - Re: [PATCH v1 04/10] iommufd/viommu: Allow drivers to control vdev

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] [day] [month] [year] [list]

Message-ID: <20241028125849.GO6956@nvidia.com>
Date: Mon, 28 Oct 2024 09:58:49 -0300
From: Jason Gunthorpe <jgg@...dia.com>
To: Nicolin Chen <nicolinc@...dia.com>
Cc: kevin.tian@...el.com, will@...nel.org, joro@...tes.org,
	suravee.suthikulpanit@....com, robin.murphy@....com,
	dwmw2@...radead.org, baolu.lu@...ux.intel.com, shuah@...nel.org,
	linux-kernel@...r.kernel.org, iommu@...ts.linux.dev,
	linux-arm-kernel@...ts.infradead.org,
	linux-kselftest@...r.kernel.org, eric.auger@...hat.com,
	jean-philippe@...aro.org, mdf@...nel.org, mshavit@...gle.com,
	shameerali.kolothum.thodi@...wei.com, smostafa@...gle.com,
	yi.l.liu@...el.com
Subject: Re: [PATCH v1 04/10] iommufd/viommu: Allow drivers to control
 vdev_id lifecycle

On Wed, Oct 23, 2024 at 11:54:54AM -0700, Nicolin Chen wrote:
> > The iopf detatch function will act as a barrirer to ensure that all
> > the async work has completed, sort of like how RCU works.
> 
> The xa_lock(&group->pasid_array) is released once the handle is
> returned to the iommu_attach_handle_get callers, so it protects
> only for a very short window (T0 below). What if:
>    | detach()                       | isr=>iommu_report_device_fault()
> T0 | Get attach_handle [xa_lock]    | Get attach_handle [xa_lock]
> T1 | Clean deliver Q [fault->mutex] | Waiting for fault->mutex
> T2 | iommufd_eventq_iopf_disable()  | Add new fault to the deliver Q
> T3 | kfree(handle)                  | ?? 

Prior to iommufd_eventq_iopf_disable() the driver has to ensure the
threads calling isr->iommu_report_device_fault() are fenced.

New threads that start running cannot see the attach_handle() because
it is not in the xarray anymore. Old threads are completed because of
the fence.

> > But here, I think it is pretty simple, isn't it?
> > 
> > When you update the master->vsmmu you can query the vsmmu to get the
> > vdev id of that master, then store it in the master struct and forward
> > it to the iommufd_viommu_report_irq(). That could even search the
> > xarray since attach is not a performance path.
> > 
> > Then it is locked under the master->lock
> 
> Yes! I didn't see that coming. vdev->id must be set before the
> attach to a nested domain, and can be cleaned after the device
> detaches. Maybe an attach to vIOMMU-based nested domain should
> just fail if idev->vdev isn't ready?

That would make sense

Jason