Re: [PATCH 1/2] iommu: Prevent RESV_DIRECT devices from blocking domains

[Robin Murphy <robin.murphy@arm.com>]

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.