Re: [PATCH] iommu/s390: Fix race with release_device ops

[Matthew Rosato <mjrosato@linux.ibm.com>]

On 8/24/22 4:37 AM, Pierre Morel wrote:
> 
> 
> On 8/23/22 22:30, Matthew Rosato wrote:
>> With commit fa7e9ecc5e1c ("iommu/s390: Tolerate repeat attach_dev
>> calls") s390-iommu is supposed to handle dynamic switching between IOMMU
>> domains and the DMA API handling.  However, this commit does not
>> sufficiently handle the case where the device is released via a call
>> to the release_device op as it may occur at the same time as an opposing
>> attach_dev or detach_dev since the group mutex is not held over
>> release_device.  This was observed if the device is deconfigured during a
>> small window during vfio-pci initialization and can result in WARNs and
>> potential kernel panics.
>>
>> Handle this by tracking when the device is probed/released via
>> dev_iommu_priv_set/get().  Ensure that once the device is released only
>> release_device handles the re-init of the device DMA.
>>
>> Fixes: fa7e9ecc5e1c ("iommu/s390: Tolerate repeat attach_dev calls")
>> Signed-off-by: Matthew Rosato <mjrosato@linux.ibm.com>
>> ---
>>   arch/s390/include/asm/pci.h |  1 +
>>   arch/s390/pci/pci.c         |  1 +
>>   drivers/iommu/s390-iommu.c  | 68 ++++++++++++++++++++++++++++---------
>>   3 files changed, 54 insertions(+), 16 deletions(-)
>>
>> diff --git a/arch/s390/include/asm/pci.h b/arch/s390/include/asm/pci.h
>> index 7b4cdadbc023..1295b6900e4b 100644
>> --- a/arch/s390/include/asm/pci.h
>> +++ b/arch/s390/include/asm/pci.h
>> @@ -157,6 +157,7 @@ struct zpci_dev {
>>       /* DMA stuff */
>>       unsigned long    *dma_table;
>>       spinlock_t    dma_table_lock;
>> +    spinlock_t    dma_domain_lock;
>>       int        tlb_refresh;
>>         spinlock_t    iommu_bitmap_lock;
>> diff --git a/arch/s390/pci/pci.c b/arch/s390/pci/pci.c
>> index 73cdc5539384..61901c1be3cc 100644
>> --- a/arch/s390/pci/pci.c
>> +++ b/arch/s390/pci/pci.c
>> @@ -832,6 +832,7 @@ struct zpci_dev *zpci_create_device(u32 fid, u32 fh, enum zpci_state state)
>>       kref_init(&zdev->kref);
>>       mutex_init(&zdev->lock);
>>       mutex_init(&zdev->kzdev_lock);
>> +    spin_lock_init(&zdev->dma_domain_lock);
>>         rc = zpci_init_iommu(zdev);
>>       if (rc)
>> diff --git a/drivers/iommu/s390-iommu.c b/drivers/iommu/s390-iommu.c
>> index c898bcbbce11..513a7ebd23b3 100644
>> --- a/drivers/iommu/s390-iommu.c
>> +++ b/drivers/iommu/s390-iommu.c
>> @@ -90,15 +90,39 @@ static int s390_iommu_attach_device(struct iommu_domain *domain,
>>       struct zpci_dev *zdev = to_zpci_dev(dev);
>>       struct s390_domain_device *domain_device;
>>       unsigned long flags;
>> -    int cc, rc;
>> +    int cc, rc = 0;
>>         if (!zdev)
>>           return -ENODEV;
>>   +    /* First check compatibility */
>> +    spin_lock_irqsave(&s390_domain->list_lock, flags);
>> +    /* First device defines the DMA range limits */
>> +    if (list_empty(&s390_domain->devices)) {
>> +        domain->geometry.aperture_start = zdev->start_dma;
>> +        domain->geometry.aperture_end = zdev->end_dma;
>> +        domain->geometry.force_aperture = true;
>> +    /* Allow only devices with identical DMA range limits */
>> +    } else if (domain->geometry.aperture_start != zdev->start_dma ||
>> +           domain->geometry.aperture_end != zdev->end_dma) {
>> +        rc = -EINVAL;
>> +    }
>> +    spin_unlock_irqrestore(&s390_domain->list_lock, flags);
>> +    if (rc)
>> +        return rc;
>> +
>>       domain_device = kzalloc(sizeof(*domain_device), GFP_KERNEL);
>>       if (!domain_device)
>>           return -ENOMEM;
>>   +    /* Leave now if the device has already been released */
>> +    spin_lock_irqsave(&zdev->dma_domain_lock, flags);
>> +    if (!dev_iommu_priv_get(dev)) {
>> +        spin_unlock_irqrestore(&zdev->dma_domain_lock, flags);
>> +        kfree(domain_device);
>> +        return 0;
>> +    }
>> +
>>       if (zdev->dma_table && !zdev->s390_domain) {
>>           cc = zpci_dma_exit_device(zdev);
>>           if (cc) {
> 
> Am I wrong? It seems to me that zpci_dma_exit_device here is called with the spin_lock locked but this function zpci_dma_exit_device calls vfree which may sleep.
> 

Oh, good point, I just enabled lockdep to verify that.

I think we could just replace this with a mutex instead, it's not a performance path.  I've been running tests successfully today with this patch modified to instead use a mutex for dma_domain_lock.