linux-kernel - Re: [PATCH v4 1/2] iommu/s390: Fix race with release

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <aa4ce332b6e280ec741064935fdd3e6fbabd8dbe.camel@linux.ibm.com>
Date:   Fri, 02 Sep 2022 09:49:45 +0200
From:   Niklas Schnelle <schnelle@...ux.ibm.com>
To:     Matthew Rosato <mjrosato@...ux.ibm.com>,
        Pierre Morel <pmorel@...ux.ibm.com>, iommu@...ts.linux.dev
Cc:     linux-s390@...r.kernel.org, borntraeger@...ux.ibm.com,
        hca@...ux.ibm.com, gor@...ux.ibm.com,
        gerald.schaefer@...ux.ibm.com, agordeev@...ux.ibm.com,
        svens@...ux.ibm.com, joro@...tes.org, will@...nel.org,
        robin.murphy@....com, jgg@...dia.com, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v4 1/2] iommu/s390: Fix race with release_device ops

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@...ux.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.