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: <e64025a7-c066-8b6e-34aa-d0cf204aca89@linux.ibm.com>
Date:   Mon, 29 Aug 2022 09:38:11 -0400
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.12.0
Subject: Re: [PATCH v2] iommu/s390: Fix race with release_device ops
Content-Language: en-US
To:     Heiko Carstens <hca@linux.ibm.com>
Cc:     iommu@lists.linux.dev, linux-s390@vger.kernel.org,
        schnelle@linux.ibm.com, pmorel@linux.ibm.com,
        borntraeger@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: <20220826194721.250123-1-mjrosato@linux.ibm.com>
 <Ywyc/WxFg3zawXL5@osiris>
From:   Matthew Rosato <mjrosato@linux.ibm.com>
In-Reply-To: <Ywyc/WxFg3zawXL5@osiris>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 8/29/22 7:03 AM, Heiko Carstens wrote:
> On Fri, Aug 26, 2022 at 03:47:21PM -0400, 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>
> ...
>> +	/* 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);
> ...
>>  	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);
>> -		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);
> 
> Stupid question: but how is this not racy when the spinlock is
> released between doing something that depends on an empty list and
> actually adding to the list later, after the lock had been released?
> 

Oh, that's a good point...  This was re-arranged to simplify error backout cases, but theoretically yeah there could be 2 competing threads with different apertures that would both list_add here when really first-in should win and the 2nd should be rejected.  That's no good.  In practice this wouldn't happen today because of the device<->domain relationship we have in s390, but we still need to avoid introducing such an issue.

We should probably just go back to doing the aperture check at the later point at the same time we list_add and live with the more complicated backout path -- I'll have a closer look but either way there will be a v3 that changes this, thanks.

>> +		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);
>>  		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);
> 
> Looking at the patch and this code it is also anything but obvious
> which _data_ is actually protected by the mutex. Anyway.. just some
> stupid comments while briefly looking at the patch :)

Fair; largely, it's purpose is to protect the setting of zdev->s390_domain (which indicates e.g. whether s390-iommu or s390 pci-dma is being used to access the device).  However we also use it to protect the updating/checking of the priv value (e.g. probe state of the device) as this is relevant to what change we make to s390_domain during an attach/detach and we want to make that decision based on a consistent 'probed' state.  Since there will already be a v3 I will try to add a comment to its definition in pci.h and/or maybe here in release_device.