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

[Yi Liu <yi.l.liu@intel.com>]

On 2024/12/9 09:39, Baolu Lu wrote:
> On 12/7/24 20:03, Yi Liu wrote:
>> The qi_batch is allocated when assigning cache tag for a domain. While
>> for nested parent domain, it is missed. Hence, when trying to map pages
>> to the nested parent, NULL dereference occurred.
> 
> Yes. Good catch!
> 
>> To solve it, allocate qi_batch for the nested parent domain when assigning
>> cache tag for the nested domain is enough. However, it seems not quite
>> reliable to allocate qi_batch in cache tag as a domain may be used by
>> multiple devices, and there is no lock around the domain->qi_batch check.
>> As all the domains (except blocking domain and identity domain) are supposed
>> to have qi_batch, this fix just allocates the qi_batch in the domain
>> allocation.
> 
> So there appears to be two problems, the domain->qi_batch is not
> allocated for all paths and it's allocated without lock protection that
> creates a race case and possibly result in memory leak.

yes, it is although we haven't encountered it for now.

> But I don't think there is a need to move the domain->qi_batch
> allocation to the domain allocation path. That's a kind of refactoring
> and there is no need to mess it with a fix patch like this.

aha, yes. I felt the qi_batch eventually would be allocating it in domain
allocation, hence make the fix like this.

> 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.

> +    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?

-- 
Regards,
Yi Liu