lkml.org 
[lkml]   [2023]   [Jun]   [19]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCH 1/2] iommu: Prevent RESV_DIRECT devices from blocking domains
From
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.

\
 
 \ /
  Last update: 2023-06-19 16:21    [W:0.100 / U:0.708 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site