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

[Niklas Schnelle <schnelle@linux.ibm.com>]

On Thu, 2022-09-01 at 16:28 -0400, Matthew Rosato wrote:
> > 


> On 9/1/22 5:37 AM, Niklas Schnelle wrote:
> > On Thu, 2022-09-01 at 09:56 +0200, Pierre Morel wrote:
> > > On 8/31/22 22:12, 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  | 39 ++++++++++++++++++++++++++++++++++---
> > > >   3 files changed, 38 insertions(+), 3 deletions(-)
> > > > 
> > > > 
> > ---8<---
> > > >   
> > > > @@ -206,10 +221,28 @@ static void s390_iommu_release_device(struct device *dev)
> > > > 
> > ---8<---
> > > > +		/* 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);
> > > 
> > > Sorry if it is a stupid question, but two things looks strange to me:
> > > 
> > > - having DMA initialized just after having unregistered the IOAT
> > > Is that really all we need to unregister before calling dma_init_device?
> 
> This is also how s390-iommu has been handling detach_dev (and still does)
> 
> > > - having DMA initialized inside the release_device callback:
> > > Why isn't it done in the device_probe ?
> > 
> > As I understand it iommu_release_device() which calls this code is only
> > used when a device goes away. So, I think you're right in that it makes
> > little sense to re-initialize DMA at this point, it's going to be torn
> > down immediately after anyway. I do wonder if it would be an acceptably
> > small change to just set zdev->s390_domain = NULL here and leave DMA
> > uninitialized while making zpci_dma_exit_device() deal with that e.g.
> > by doing nothing if zdev->dma_table is NULL but I'm not sure.
> 
> Right -- since it's a fix, I was trying to keep the changes minimal and this behavior (re-init DMA even on release_device) was existing, it was just always done within s390_iommu_detach_device before.
> 
> If you want, I could experiment with setting zdev->dma_table = NULL on the release path only (and checking it in zpci_dma_exit_device())
> 

Your current approach is fine with me. After all this oddity of
detaching on release and initializing DMA is existing behavior.