Re: [PATCH v3 1/2] intel_iommu: Take the bql before registering a new address space

["Michael S. Tsirkin" <mst@redhat.com>]

On Wed, Apr 23, 2025 at 09:15:36AM +0000, CLEMENT MATHIEU--DRIF wrote:
> 
> 
> On 23/04/2025 8:00 am, Michael S. Tsirkin wrote:
> > Caution: External email. Do not open attachments or click links, unless this email comes from a known sender and you know the content is safe.
> >
> >
> > On Wed, Apr 23, 2025 at 05:38:20AM +0000, CLEMENT MATHIEU--DRIF wrote:
> >> Address space creation might end up being called without holding the
> >> bql as it is exposed through the IOMMU ops.
> >>
> >> Signed-off-by: Clement Mathieu--Drif <clement.mathieu--drif@eviden.com>
> >> ---
> >>   hw/i386/intel_iommu.c | 6 ++++++
> >>   1 file changed, 6 insertions(+)
> >>
> >> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
> >> index dffd7ee885..cc8c9857e1 100644
> >> --- a/hw/i386/intel_iommu.c
> >> +++ b/hw/i386/intel_iommu.c
> >> @@ -4238,6 +4238,12 @@ VTDAddressSpace *vtd_find_add_as(IntelIOMMUState *s, PCIBus *bus,
> >>           vtd_dev_as->context_cache_entry.context_cache_gen = 0;
> >>           vtd_dev_as->iova_tree = iova_tree_new();
> >>
> >> +        /*
> >> +         * memory_region_add_subregion_overlap requires the bql,
> >> +         * make sure we own it.
> >> +         */
> >> +        BQL_LOCK_GUARD();
> >> +
> >>           memory_region_init(&vtd_dev_as->root, OBJECT(s), name, UINT64_MAX);
> >>           address_space_init(&vtd_dev_as->as, &vtd_dev_as->root, "vtd-root");
> >
> > Does not look like this addresses all races here:
> > https://lore.kernel.org/all/8062d868-469f-4c1d-a071-099b8e18857c@redhat.com
> >
> >
> > while this can be a separate patch on top, I'd rather we just
> > address everything in a single patchset.
> 
> Hi Michael,
> 
> We only aim to fix the potential crash here.
> I saw Paolo's response and I know the race exists. I will send a patch
> set to fix it soon but are you sure both fixes must be in the same
> series? I think the nature is different.
> 
> cmd

If you have two races in the same function, fixing one can easily
make another one occur more. Let's just fix it all please,
I don't see any rush to apply a partial fix.

-- 
MST