Re: [PATCH 1/4] iommu: Add dev_iommu->ops_rwsem

[Baolu Lu]

On 2/15/23 7:24 PM, Robin Murphy wrote:
> On 2023-02-15 05:34, Baolu Lu wrote:
> > On 2/13/23 10:16 PM, Jason Gunthorpe wrote:
> > > On Mon, Feb 13, 2023 at 03:49:38PM +0800, Lu Baolu wrote:
> > >
> > > > +static int iommu_group_freeze_dev_ops(struct iommu_group *group)
> > > > +{
> > > > +    struct group_device *device;
> > > > +    struct device *dev;
> > > > +
> > > > +    mutex_lock(&group->mutex);
> > > > +    list_for_each_entry(device, &group->devices, list) {
> > > > +        dev = device->dev;
> > > > +        down_read(&dev->iommu->ops_rwsem);
> > >
> > > This isn't allowed, you can't obtain locks in a loop like this, it
> > > will deadlock.
> > >
> > > You don't need more locks, we already have the group mutex, the
> > > release path should be fixed to use it properly as I was trying to do 
> > > here:
> > >
> > > https://lore.kernel.org/kvm/4-v1-ef00ffecea52+2cb-iommu_group_lifetime_jgg@xxxxxxxxxx/
> > > https://lore.kernel.org/kvm/YyyTxx0HnA3maxEk@xxxxxxxxxx/
> > >
> > > Then what you'd do in a path like this is:
> > >
> > >    mutex_lock(&group->mutex);
> > >    dev = iommu_group_first_device(group)
> > >    if (!dev)
> > >       /* Racing with group cleanup */
> > >       return -EINVAL;
> > >    /* Now dev->ops is valid and must remain valid so long as
> > >       group->mutex is held */
> > >
> > > The only reason this doesn't work already is because of the above
> > > stuff about release not holding the group mutex when manipulating the
> > > devices in the group. This is simply mis-locked.
> > >
> > > Robin explained it was done like this because
> > > arm_iommu_detach_device() re-enters the iommu core during release_dev,
> > > so I suggest fixing that by simply not doing that (see above)
> > >
> > > Below is the lastest attempt I had tried, I didn't have time to get back
> > > to it and we fixed the bug another way. It needs a bit of adjusting to
> > > also remove the device from the group after release is called within
> > > the same mutex critical region.
> >
> > Yes. If we can make remove device from list and device release in the
> > same mutex critical region, we don't need any other lock mechanism
> > anymore.
> >
> > The ipmmu driver supports default domain.
>
> It supports default domains *on arm64*. Nothing on 32-bit ARM uses 
> default domains, they won't even exist since iommu-dma is not enabled, 
> and either way the ARM DMA ops don't understand groups. I don't see an 
> obvious satisfactory solution while the arm_iommu_* APIs still exist, 
> but if we need bodges in the interim could we please try to concentrate 
> them in ARM-specific code?

Yes, sure. Thanks for the guide.

Best regards,
baolu