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

[Samiullah Khawaja <skhawaja@google.com>]

On Sun, Jul 12, 2026 at 03:35:39PM +0800, Baolu Lu wrote:
>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;

This sounds great.
>
>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.

+1 to this also.
>
>Thanks,
>baolu

Thanks,
Sami