Re: [PATCH -next] iommu/dma: Fix missing mutex_init() in iommu_get_msi_cookie()

[Robin Murphy]

On 2022-07-18 07:42, Yang Yingliang wrote:
> Hi,
>
> On 2022/7/15 17:28, Robin Murphy wrote:
> > On 2022-07-15 08:49, Joerg Roedel wrote:
> > > Adding Robin.
> > >
> > > On Mon, Jun 27, 2022 at 04:55:33PM +0800, Yang Yingliang wrote:
> > > > cookie_alloc() is called by iommu_get_dma_cookie() and 
> > > > iommu_get_msi_cookie(),
> > > > but the mutex is only initialized in iommu_get_dma_cookie(), move 
> > > > mutex_init()
> > > > into cookie_alloc() to make sure the mutex will be initialized.
> >
> > The mutex is only used in iommu_dma_init_domain(), which is only 
> > called by iommu_setup_dma_ops() for IOMMU_DOMAIN_DMA domains. How is 
> > there a problem here?
> It's no problem now, but I thinks it's better to initialize the 'mutex' 
> in cookie_alloc() to make code stronger.

Stronger against what, though? The sole reason this mutex exists at all 
is as a temporary measure to protect the IOVA domain from concurrent 
initialisation - I suppose in hindsight it might have made sense to 
define it inside the union, but I don't see much point in churning that 
now. I'd rather spend the time on continuing to get the 
iommu_probe_device() path sorted out so that we don't have the whole 
problematic driver-probe-time-replay mess in the first place and this 
mutex can be reverted ASAP.

> > > > Fixes: ac9a5d522bb8 ("iommu/dma: Fix race condition during 
> > > > iova_domain initialization")
> > > > Reported-by: Hulk Robot <hulkci@xxxxxxxxxx>

Please teach your robot to care about things that actually matter. All 
it's "reporting" here is that it isn't clever enough to follow control 
flow across multiple compilation units, otherwise it should have seen 
the matching iommu_is_dma_domain() checks between __iommu_domain_alloc() 
and iommu_setup_dma_ops(). Having to explain this is not a good use of 
the kernel community's time and effort.

Thanks,
Robin.

> > > > Signed-off-by: Yang Yingliang <yangyingliang@xxxxxxxxxx>
> > > > ---
> > > >   drivers/iommu/dma-iommu.c | 2 +-
> > > >   1 file changed, 1 insertion(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
> > > > index 1910f4f1612b..e29157380c48 100644
> > > > --- a/drivers/iommu/dma-iommu.c
> > > > +++ b/drivers/iommu/dma-iommu.c
> > > > @@ -294,6 +294,7 @@ static struct iommu_dma_cookie 
> > > > *cookie_alloc(enum iommu_dma_cookie_type type)
> > > >       if (cookie) {
> > > >           INIT_LIST_HEAD(&cookie->msi_page_list);
> > > >           cookie->type = type;
> > > > +        mutex_init(&cookie->mutex);
> > > >       }
> > > >       return cookie;
> > > >   }
> > > > @@ -311,7 +312,6 @@ int iommu_get_dma_cookie(struct iommu_domain 
> > > > *domain)
> > > >       if (!domain->iova_cookie)
> > > >           return -ENOMEM;
> > > >   -    mutex_init(&domain->iova_cookie->mutex);
> > > >       return 0;
> > > >   }
> > > >   --
> > > > 2.25.1
> > .