linux-kernel - Re: [PATCH v3 4/7] iommu: Switch __iommu_domain

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

Message-ID: <5c2746a3-ef3a-2747-c600-395159f668ba@arm.com>
Date:   Mon, 18 Sep 2023 11:36:18 +0100
From:   Robin Murphy <robin.murphy@....com>
To:     Baolu Lu <baolu.lu@...ux.intel.com>, joro@...tes.org,
        will@...nel.org
Cc:     iommu@...ts.linux.dev, linux-kernel@...r.kernel.org,
        linux-arm-kernel@...ts.infradead.org, jgg@...dia.com
Subject: Re: [PATCH v3 4/7] iommu: Switch __iommu_domain_alloc() to device ops

On 2023-09-18 07:10, Baolu Lu wrote:
> On 9/16/23 12:58 AM, Robin Murphy wrote:
>> @@ -1997,16 +1995,13 @@ void iommu_set_fault_handler(struct 
>> iommu_domain *domain,
>>   }
>>   EXPORT_SYMBOL_GPL(iommu_set_fault_handler);
>> -static struct iommu_domain *__iommu_domain_alloc(const struct 
>> bus_type *bus,
>> +static struct iommu_domain *__iommu_domain_alloc(struct device *dev,
>>                            unsigned type)
>>   {
>> -    const struct iommu_ops *ops = bus ? bus->iommu_ops : NULL;
>> +    const struct iommu_ops *ops = dev_iommu_ops(dev);
>>       struct iommu_domain *domain;
>>       unsigned int alloc_type = type & IOMMU_DOMAIN_ALLOC_FLAGS;
>> -    if (!ops)
>> -        return NULL;
>> -
>>       domain = ops->domain_alloc(alloc_type);
>>       if (!domain)
>>           return NULL;
>> @@ -2030,9 +2025,28 @@ static struct iommu_domain 
>> *__iommu_domain_alloc(const struct bus_type *bus,
>>       return domain;
>>   }
>> +static int __iommu_domain_alloc_dev(struct device *dev, void *data)
>> +{
>> +    struct device **alloc_dev = data;
>> +
>> +    if (!dev_has_iommu(dev))
>> +        return 0;
>> +
>> +    WARN_ONCE(*alloc_dev && dev_iommu_ops(dev) != 
>> dev_iommu_ops(*alloc_dev),
>> +          "Multiple IOMMU drivers present, which the public IOMMU API 
>> can't fully support yet. You may still need to disable one or more to 
>> get the expected result here, sorry!\n");
>> +
>> +    *alloc_dev = dev;
>> +    return 0;
>> +}
>> +
>>   struct iommu_domain *iommu_domain_alloc(const struct bus_type *bus)
>>   {
>> -    return __iommu_domain_alloc(bus, IOMMU_DOMAIN_UNMANAGED);
>> +    struct device *dev = NULL;
>> +
>> +    if (bus_for_each_dev(bus, NULL, &dev, __iommu_domain_alloc_dev))
>> +        return NULL;
> 
> __iommu_domain_alloc_dev() always returns 0. Hence above if condition
> will never be true. Perhaps, in __iommu_domain_alloc_dev(),

Oh bugger, seems I screwed up the unnecessarily overcomplicated rebase 
that I made for myself, and managed to put this back to the v1 code, so 
it's just wrong (bus_for_each_dev() can return an error itself if the 
bus isn't properly initialised, but it also returns success if the bus 
has no devices, which was handled properly in v2 that you actually R-b'd).

> 
>      if (WARN_ON(*alloc_dev && dev_iommu_ops(dev) !=
>              dev_iommu_ops(*alloc_dev))
>          return -EPERM;

I went back and forth on this initially, but in the end I figured since 
the other patches are meant to be making the rest of the public API 
sufficiently robust, then if someone does try it with multiple drivers 
before full support can be finished, they can at least have some chance 
of getting the desired result, rather than a guarantee of not. I am 
still open to being convinced otherwise, though.

Thanks,
Robin.

> 
> ?
> 
>> +
>> +    return __iommu_domain_alloc(dev, IOMMU_DOMAIN_UNMANAGED);
> 
> Is it possible that all devices on this bus have dev_has_iommu() to be
> false? If so, we probably need something like below:
> 
>      if (!dev_has_iommu(dev))
>          return -ENODEV;
> 
> ?
> 
>>   }
>>   EXPORT_SYMBOL_GPL(iommu_domain_alloc);
>> @@ -3228,13 +3242,17 @@ static int 
>> __iommu_group_alloc_blocking_domain(struct iommu_group *group)
>>       if (group->blocking_domain)
>>           return 0;
>> -    group->blocking_domain = __iommu_domain_alloc(dev->bus, 
>> IOMMU_DOMAIN_BLOCKED);
>> +    /* noiommu groups should never be here */
>> +    if (WARN_ON(!dev_has_iommu(dev)))
>> +        return -ENODEV;
>> +
>> +    group->blocking_domain = __iommu_domain_alloc(dev, 
>> IOMMU_DOMAIN_BLOCKED);
>>       if (!group->blocking_domain) {
>>           /*
>>            * For drivers that do not yet understand IOMMU_DOMAIN_BLOCKED
>>            * create an empty domain instead.
>>            */
>> -        group->blocking_domain = __iommu_domain_alloc(dev->bus, 
>> IOMMU_DOMAIN_UNMANAGED);
>> +        group->blocking_domain = __iommu_domain_alloc(dev, 
>> IOMMU_DOMAIN_UNMANAGED);
>>           if (!group->blocking_domain)
>>               return -EINVAL;
>>       }
> 
> Best regards,
> baolu