Re: [PATCH] iommu/vt-d: Fix qi_batch NULL pointer with nested parent domain

[Baolu Lu <baolu.lu@linux.intel.com>]

On 12/9/24 11:07, Yi Liu wrote:
>> Perhaps add a helper to handle the domain->qi_batch allocation? And then
>> call it in the necessary paths? Something like this:
>>
>> --- a/drivers/iommu/intel/cache.c
>> +++ b/drivers/iommu/intel/cache.c
>> @@ -105,12 +105,35 @@ static void cache_tag_unassign(struct 
>> dmar_domain *domain, u16 did,
>>       spin_unlock_irqrestore(&domain->cache_lock, flags);
>>   }
>>
>> +/* domain->qi_batch will be freed in iommu_free_domain() path. */
>> +static int domain_qi_batch_alloc(struct dmar_domain *domain)
>> +{
>> +    unsigned long flags;
>> +    int ret = 0;
>> +
>> +    spin_lock_irqsave(&domain->cache_lock, flags);
> 
> I'm wondering if there is any difference using the domain->lock v.s. using
> the domain->cache_lock. The RID attach path acquires/releases the
> domain->lock first, and then the domain->cache_lock. While the PASID path
> does the reverse order. Although neither of the two paths hold the locks
> for a long duration (no A-B-B-A locking issue), it still means when a
> domain is shared between RID and PASID path, the domain->qi_batch check is
> not in the same phase of the two paths.

You are right. domain->lock is the right one.

> 
>> +    if (domain->qi_batch)
>> +        goto out_unlock;
>> +
>> +    domain->qi_batch = kzalloc(sizeof(*domain->qi_batch), GFP_ATOMIC);
>> +    if (!domain->qi_batch)
>> +        ret = -ENOMEM;
>> +out_unlock:
>> +    spin_unlock_irqrestore(&domain->cache_lock, flags);
>> +
>> +    return ret;
>> +}
>> +
>>   static int __cache_tag_assign_domain(struct dmar_domain *domain, u16 
>> did,
>>                        struct device *dev, ioasid_t pasid)
>>   {
>>       struct device_domain_info *info = dev_iommu_priv_get(dev);
>>       int ret;
>>
>> +    ret = domain_qi_batch_alloc(domain);
>> +    if (ret)
>> +        return ret;
>> +
> 
> also need to call it in __cache_tag_assign_parent_domain(). BTW. Is the
> domain->qi_batch required if there is no ATS?

Yes.

Thanks,
baolu