On 19/06/2023 2:41 pm, Jason Gunthorpe wrote:
On Mon, Jun 19, 2023 at 02:33:18PM +0100, Robin Murphy wrote:
@@ -2121,6 +2125,21 @@ static int __iommu_device_set_domain(struct iommu_group *group,
{
int ret;
+ /*
+ * If the driver has requested IOMMU_RESV_DIRECT then we cannot allow
+ * the blocking domain to be attached as it does not contain the
+ * required 1:1 mapping. This test effectively exclusive the device from
+ * being used with iommu_group_claim_dma_owner() which will block vfio
+ * and iommufd as well.
+ */
+ if (dev->iommu->requires_direct &&
+ (new_domain->type == IOMMU_DOMAIN_BLOCKED ||
Given the notion elsewhere that we want to use the blocking domain as a last
resort to handle an attach failure,
We shouldn't do that for cases where requires_direct is true, the last
resort will have to be the static identity domain.
at face value it looks suspect that failing to attach to a blocking
domain could also be a thing. I guess technically this is failing at
a slightly different level so maybe it does work out OK, but it's
still smelly.
It basically says that this driver doesn't support blocking domains on
this device. What we don't want is for the driver to fail blocking or
identity attaches.
Is that really the relevant semantic though? I thought the point was to
prevent userspace (or anyone else for that matter) taking ownership of a
device with reserved regions which we can't trust them to honour. Not
least because the series is entitled "Prevent RESV_DIRECT devices from
user assignment", not anything about attaching to blocking domains. Plus
the existing intel-iommu behaviour being generalised is specific to
IOMMU_DOMAIN_UNMANAGED.
The main thing, though, is that not everything implements the
IOMMU_DOMAIN_BLOCKED optimisation, so a nominal blocking domain could be
IOMMU_DOMAIN_UNMANAGED as well.
Yes, it should check new_domain == group->blocking_domain as well.
FWIW I'd prefer to make the RESV_DIRECT check explicit in
__iommu_take_dma_ownership() rather than hide it in an
implementation detail; that's going to be a lot clearer to reason
about as time goes on.
We want to completely forbid blocking domains at all on these devices
because they are not supported (by FW request). I don't really like
the idea that we go and assume the only users of blocking domains are
also using take_dma_ownership() - that feels like a future bug waiting
to happen.
On reflection, I don't think that aspect actually matters anyway -
nobody can explicitly request attachment to a blocking domain, so if the
only time they're used is when the IOMMU driver has already had a
catastrophic internal failure such that we decide to declare the device
toasted and deliberately put it into an unusable state, blocking its
reserved regions doesn't seem like a big deal. In fact if anything it
kind of feels like the right thing to do for that situation. We're
saying that we want the device to stop accessing memory because things
might be in an inconsistent state which we can't trust; who says that
mappings of RESV_DIRECT regions haven't also gone wonky? Having BLOCKED
mean that the device truly cannot access - and thus potentially corrupt
- *any* memory anywhere seems like the most robust and useful behaviour.
Thanks,
Robin.
