linux-kernel - RE: [RFC 06/20] iommu: Add iommu_device_init[exit]_user

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: <BN9PR11MB543327D25DCE242919F7909A8CA79@BN9PR11MB5433.namprd11.prod.outlook.com>
Date:   Mon, 27 Sep 2021 13:32:34 +0000
From:   "Tian, Kevin" <kevin.tian@...el.com>
To:     Jason Gunthorpe <jgg@...dia.com>
CC:     "kvm@...r.kernel.org" <kvm@...r.kernel.org>,
        "jasowang@...hat.com" <jasowang@...hat.com>,
        "kwankhede@...dia.com" <kwankhede@...dia.com>,
        "hch@....de" <hch@....de>,
        "jean-philippe@...aro.org" <jean-philippe@...aro.org>,
        "Jiang, Dave" <dave.jiang@...el.com>,
        "Raj, Ashok" <ashok.raj@...el.com>,
        "corbet@....net" <corbet@....net>,
        "parav@...lanox.com" <parav@...lanox.com>,
        "alex.williamson@...hat.com" <alex.williamson@...hat.com>,
        "lkml@...ux.net" <lkml@...ux.net>,
        "david@...son.dropbear.id.au" <david@...son.dropbear.id.au>,
        "dwmw2@...radead.org" <dwmw2@...radead.org>,
        "Tian, Jun J" <jun.j.tian@...el.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "lushenming@...wei.com" <lushenming@...wei.com>,
        "pbonzini@...hat.com" <pbonzini@...hat.com>,
        "robin.murphy@....com" <robin.murphy@....com>
Subject: RE: [RFC 06/20] iommu: Add iommu_device_init[exit]_user_dma
 interfaces

> From: Jason Gunthorpe
> Sent: Monday, September 27, 2021 9:10 PM
> 
> On Mon, Sep 27, 2021 at 01:00:08PM +0000, Tian, Kevin wrote:
> 
> > > I think for such a narrow usage you should not change the struct
> > > device_driver. Just have pci_stub call a function to flip back to user
> > > mode.
> >
> > Here we want to ensure that kernel dma should be blocked
> > if the group is already marked for user-dma. If we just blindly
> > do it for any driver at this point (as you commented earlier):
> >
> > +       ret = iommu_set_kernel_ownership(dev);
> > +       if (ret)
> > +               return ret;
> >
> > how would pci-stub reach its function to indicate that it doesn't
> > do dma and flip back?
> 
> > Do you envision a simpler policy that no driver can be bound
> > to the group if it's already set for user-dma? what about vfio-pci
> > itself?
> 
> Yes.. I'm not sure there is a good use case to allow the stub drivers
> to load/unload while a VFIO is running. At least, not a strong enough
> one to justify a global change to the driver core..

I'm fine with not loading pci-stub. From the very 1st commit msg
looks pci-stub was introduced before vfio to prevent host driver 
loading when doing device assignment with KVM. I'm not sure 
whether other usages are built on pci-stub later, but in general it's 
not good to position devices in a same group into different usages.

but I'm little worried that even vfio-pci itself cannot be bound now,
which implies that all devices in a group which are intended to be
used by the user must be bound to vfio-pci in a breath before the 
user attempts to open any of them, i.e. late-binding and device-
hotplug is disallowed after the initial open. I'm not sure how 
important such an usage would be, but it does cause user-tangible
semantics change.

Alex?

> 
> > > > +static int iommu_dev_viable(struct device *dev, void *data)
> > > > +{
> > > > +	enum dma_hint hint = *data;
> > > > +	struct device_driver *drv = READ_ONCE(dev->driver);
> > >
> > > Especially since this isn't locked properly or safe.
> >
> > I have the same worry when copying from vfio. Not sure how
> > vfio gets safe with this approach...
> 
> Fixing the locking in vfio_dev_viable is part of deleting the unbound
> list. Once it properly uses the device_lock and doesn't race with the
> driver core like this things are much better. Don't copy this stuff
> into the iommu core without fixing it.

sure. Above was just a quickly-baked sample code to match your 
thought.

> 
> https://github.com/jgunthorpe/linux/commit/fa6abb318ccca114da12c0b5b1
> 23c99131ace926
> https://github.com/jgunthorpe/linux/commit/45980bd90b023d1eea56df70d
> 1c395bdf4cc7cf1
> 
> I can't remember if the above is contingent on some of the mdev
> cleanups or not.. Have to get back to it.
> 

my home network has some problem to access above links. Will check it
tomorrow and follow the fix when working on the formal change in 
iommu core.

Thanks
Kevin