Re: [PATCH v3 07/18] iommu/vt-d: Implement device and iommu preserve/unpreserve ops

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

On 6/23/2026 3:19 AM, Samiullah Khawaja wrote:
> 
>>> +
>>> +static void unpreserve_context_table(struct intel_iommu *iommu,
>>> +                     struct iommu_hw_ser *ser,
>>> +                     u8 bus, u8 devfn)
>>> +{
>>> +    struct context_entry *context;
>>> +
>>> +    spin_lock(&iommu->lock);
>>> +    context = iommu_context_addr(iommu, bus, devfn, 0);
>>> +    spin_unlock(&iommu->lock);
>>
>> The spinlock is dropped immediately after reading the address pointer.
>> If this is guaranteed to be safe, please add a comment to explain why a
>> UAF or race is avoided here. Otherwise, the locking scope needs to be
>> extened to protect both the pointer lookup and use.
> 
> In the Intel VT-d driver, context tables are never freed once they are
> allocated during runtime, as they can be shared across multiple devices.
> So I took the lock here to protect against concurrent allocations inside
> iommu_context_addr(). Once the address is read, it is safe to use
> without holding the lock until the DMAR unit itself is torn down.
> 
> I will add a comment explaining this here.

As part of this series, I think this is acceptable. However, there
is room for further improvement in iommu_context_addr(). We could avoid
concurrent allocations by replacing the code below with try_cmpxchg64():

                 phy_addr = virt_to_phys((void *)context);
                 *entry = phy_addr | 1;

At the same time, we could also replace the spinlock with an rwsem to
eliminate the spinlock critical section. Consequently, the GFP_ATOMIC
allocation in iommu_alloc_pages_node_sz() could then be replaced with a
normal GFP_KERNEL.

Thanks,
baolu