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

[Baolu Lu <baolu.lu@linux.intel.com>]

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@nvidia.com/
>>> https://lore.kernel.org/kvm/YyyTxx0HnA3maxEk@nvidia.com/
>>>
>>> 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