Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <ebe5fa1e-812b-d705-1dff-41fa390c8596@linux.ibm.com>
Date:   Wed, 31 Aug 2022 16:10:34 -0400
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.2.0
Subject: Re: [PATCH v3] iommu/s390: Fix race with release_device ops
Content-Language: en-US
From:   Matthew Rosato <mjrosato@linux.ibm.com>
To:     Pierre Morel <pmorel@linux.ibm.com>, iommu@lists.linux.dev
Cc:     linux-s390@vger.kernel.org, schnelle@linux.ibm.com,
        borntraeger@linux.ibm.com, hca@linux.ibm.com, gor@linux.ibm.com,
        gerald.schaefer@linux.ibm.com, agordeev@linux.ibm.com,
        svens@linux.ibm.com, joro@8bytes.org, will@kernel.org,
        robin.murphy@arm.com, jgg@nvidia.com, linux-kernel@vger.kernel.org
References: <20220830201546.18871-1-mjrosato@linux.ibm.com>
 <12a49c8d-cb38-e22a-0040-88350b6210aa@linux.ibm.com>
 <89bd532b-33ac-5f9d-a06e-eb002168778b@linux.ibm.com>
In-Reply-To: <89bd532b-33ac-5f9d-a06e-eb002168778b@linux.ibm.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Precedence: bulk

On 8/31/22 10:12 AM, Matthew Rosato wrote:
> On 8/31/22 5:05 AM, Pierre Morel wrote:
>>
>>
>> On 8/30/22 22:15, 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>
>>> ---
>>> Changes since v2:
>>> - Relocate the list_empty and aperture chekcs in attach_dev to the
>>>    their original locations so they are all done under a single
>>>    acquisition of the spinlock (Heiko)
>>> ---
>>>   arch/s390/include/asm/pci.h |  1 +
>>>   arch/s390/pci/pci.c         |  1 +
>>>   drivers/iommu/s390-iommu.c  | 39 ++++++++++++++++++++++++++++++++++---
>>>   3 files changed, 38 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/arch/s390/include/asm/pci.h b/arch/s390/include/asm/pci.h
>>> index 7b4cdadbc023..080251e7b275 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;
>>> +    struct mutex    dma_domain_lock; /* protects s390_domain value */
>>>       int        tlb_refresh;
>>>         spinlock_t    iommu_bitmap_lock;
>>> diff --git a/arch/s390/pci/pci.c b/arch/s390/pci/pci.c
>>> index 73cdc5539384..973edd32ecc9 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);
>>> +    mutex_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..1137d669e849 100644
>>> --- a/drivers/iommu/s390-iommu.c
>>> +++ b/drivers/iommu/s390-iommu.c
>>> @@ -99,6 +99,14 @@ static int s390_iommu_attach_device(struct iommu_domain *domain,
>>>       if (!domain_device)
>>>           return -ENOMEM;
>>>   +    /* Leave now if the device has already been released */
>>> +    mutex_lock(&zdev->dma_domain_lock);
>>> +    if (!dev_iommu_priv_get(dev)) {
>>> +        mutex_unlock(&zdev->dma_domain_lock);
>>> +        kfree(domain_device);
>>> +        return 0;
>>> +    }
>>> +
>>>       if (zdev->dma_table && !zdev->s390_domain) {
>>>           cc = zpci_dma_exit_device(zdev);
>>>           if (cc) {
>>> @@ -132,9 +140,10 @@ static int s390_iommu_attach_device(struct iommu_domain *domain,
>>>           goto out_restore;
>>>       }
>>>       domain_device->zdev = zdev;
>>> -    zdev->s390_domain = s390_domain;
>>>       list_add(&domain_device->list, &s390_domain->devices);
>>>       spin_unlock_irqrestore(&s390_domain->list_lock, flags);
>>> +    zdev->s390_domain = s390_domain;
>>> +    mutex_unlock(&zdev->dma_domain_lock);
>>>         return 0;
>>>   @@ -147,6 +156,7 @@ static int s390_iommu_attach_device(struct iommu_domain *domain,
>>>                      virt_to_phys(zdev->dma_table));
>>>       }
>>>   out_free:
>>> +    mutex_unlock(&zdev->dma_domain_lock);
>>>       kfree(domain_device);
>>>         return rc;
>>> @@ -176,17 +186,22 @@ static void s390_iommu_detach_device(struct iommu_domain *domain,
>>>       }
>>>       spin_unlock_irqrestore(&s390_domain->list_lock, flags);
>>>   -    if (found && (zdev->s390_domain == s390_domain)) {
>>> +    mutex_lock(&zdev->dma_domain_lock);
>>> +    if (found && (zdev->s390_domain == s390_domain) &&
>>> +        dev_iommu_priv_get(dev)) {
>>>           zdev->s390_domain = NULL;
>>>           zpci_unregister_ioat(zdev, 0);
>>>           zpci_dma_init_device(zdev);
>>>       }
>>> +    mutex_unlock(&zdev->dma_domain_lock);
>>>   }
>>>     static struct iommu_device *s390_iommu_probe_device(struct device *dev)
>>>   {
>>>       struct zpci_dev *zdev = to_zpci_dev(dev);
>>>   +    dev_iommu_priv_set(dev, zdev);
>>> +
>>>       return &zdev->iommu_dev;
>>>   }
>>>   @@ -206,10 +221,28 @@ static void s390_iommu_release_device(struct device *dev)
>>>        *
>>>        * So let's call detach_dev from here if it hasn't been called before.
>>>        */
>>> -    if (zdev && zdev->s390_domain) {
>>> +    if (zdev) {
>>> +        /*
>>> +         * Clear priv to block further attaches for this device,
>>> +         * ensure detaches don't init DMA.  Hold the domain lock
>>> +         * to ensure that attach/detach get a consistent view of
>>> +         * whether or not the device is released.
>>> +         */
>>> +        mutex_lock(&zdev->dma_domain_lock);
>>> +        dev_iommu_priv_set(dev, NULL);
>>> +        mutex_unlock(&zdev->dma_domain_lock);
>>> +        /* Make sure this device is removed from the domain list */
>>>           domain = iommu_get_domain_for_dev(dev);
>>
>>
>> Shouldn't you take the group_mutex before calling directly s390_iommu_detach_device from here?
>>
> 
> Well, we can't do that directly as the group mutex is only accessible to the iommu core.  But perhaps the right answer is to call back into the core here by just replacing the existing call to s390_iommu_detach_device below with a call to iommu_detach_device (1 line change).  Then we still ensure DMA is initialized next after this point.  I've been running tests with such a change for the last few hours and this works well.
> 

Nope, looking closer that won't work -- There is a small window between calling iommu_get_domain_for_dev and then the subsequent call to iommu_detach_device() where the group->domain could still change, which would trigger a WARN_ON in iommu_detach_device (I managed to trigger this once). 

But really, I think just leaving this as a call to s390_iommu_detach_device without the group_mutex held at this point is OK, all the detach will do at this point (since priv has already been nullified) is get the list_lock and clear the entry for the domain that happened to be the group domain at the time release_device started - and any attempts at further attaches will just quietly return 0 because priv is null.  I guess technically we could also inline that list_del work into release_device and not even call s390_iommu_detach_device.  But the reality is by the time we've reached this point the device is going away and so it's going to have to be removed from all associated domains very soon, not just the one that happens to be active -- So this combined with my 2nd patch (sending shortly as a v4 series) would ensure the device gets removed from each domain, either from a detach_dev or a domain_free call.

> But this also made me notice another subtle issue re: a mismatch between the number of kallocs and kfrees for s390_domain_device.  Need to investigate that further before a next version.
> 
>>
>>>           if (domain)
>>>               s390_iommu_detach_device(domain, dev);
>>
>>
>>
>>
>>> +        /* Now ensure DMA is initialized from here */
>>> +        mutex_lock(&zdev->dma_domain_lock);
>>> +        if (zdev->s390_domain) {
>>> +            zdev->s390_domain = NULL;
>>> +            zpci_unregister_ioat(zdev, 0);
>>> +            zpci_dma_init_device(zdev);
>>> +        }
>>> +        mutex_unlock(&zdev->dma_domain_lock);
>>>       }
>>>   }
>>>  
>>
>