[PATCH 8/8] iommu: Improve map/unmap sanity checks
Robin Murphy
robin.murphy at arm.com
Wed Sep 13 11:46:45 PDT 2023
On 2023-09-13 15:39, Jason Gunthorpe wrote:
> On Tue, Sep 12, 2023 at 05:18:44PM +0100, Robin Murphy wrote:
>> The current checks for the __IOMMU_DOMAIN_PAGING capability seem a
>> bit stifled, since it is quite likely now that a non-paging domain
>> won't have a pgsize_bitmap and/or mapping ops, and thus get caught
>> by the earlier condition anyway. Swap them around to test the more
>> fundamental condition first, then we can reasonably also upgrade
>> the other to a WARN_ON, since if a driver does ever expose a paging
>> domain without the means to actually page, it's clearly very broken.
>
>> Signed-off-by: Robin Murphy <robin.murphy at arm.com>
>> ---
>> drivers/iommu/iommu.c | 10 +++++-----
>> 1 file changed, 5 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
>> index c83f2e4c56f5..391bcae4d02d 100644
>> --- a/drivers/iommu/iommu.c
>> +++ b/drivers/iommu/iommu.c
>> @@ -2427,12 +2427,12 @@ static int __iommu_map(struct iommu_domain *domain, unsigned long iova,
>> phys_addr_t orig_paddr = paddr;
>> int ret = 0;
>>
>> - if (unlikely(!ops->map_pages || domain->pgsize_bitmap == 0UL))
>> - return -ENODEV;
>> -
>> if (unlikely(!(domain->type & __IOMMU_DOMAIN_PAGING)))
>> return -EINVAL;
>
> Why isn't this a WARN_ON? The caller is clearly lost its mind if it
> is calling map on a non paging domain..
Sure it's a dumb thing to do, however I don't think we can reasonably
say without question that an external caller being dumb represents an
unexpected and serious loss of internal consistency.
>> + if (WARN_ON(!ops->map_pages || domain->pgsize_bitmap == 0UL))
>> + return -ENODEV;
>> +
>
> And these could be moved to after attach so they are not in any fast
> path, and eventually to after alloc..
Perhaps, although TBH I can't imagine it making any appreciable
difference - they're both values we need to load and use soon anyway, so
on a sensible CPU, the additional overhead should only really be a
couple of not-taken branch-if-zero instructions, which is nothing at all
compared to how much goes on in the main loop and the driver op call itself.
Thanks,
Robin.
More information about the linux-arm-kernel
mailing list