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

[Samiullah Khawaja]

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