linux-kernel - Re: [PATCH v2 0/9] Introduce vfio-pci-core subsystem

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: <20210210170847.GE4247@nvidia.com>
Date:   Wed, 10 Feb 2021 13:08:47 -0400
From:   Jason Gunthorpe <jgg@...dia.com>
To:     Alex Williamson <alex.williamson@...hat.com>
CC:     "Tian, Kevin" <kevin.tian@...el.com>,
        Max Gurtovoy <mgurtovoy@...dia.com>,
        "cohuck@...hat.com" <cohuck@...hat.com>,
        "kvm@...r.kernel.org" <kvm@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "liranl@...dia.com" <liranl@...dia.com>,
        "oren@...dia.com" <oren@...dia.com>,
        "tzahio@...dia.com" <tzahio@...dia.com>,
        "leonro@...dia.com" <leonro@...dia.com>,
        "yarong@...dia.com" <yarong@...dia.com>,
        "aviadye@...dia.com" <aviadye@...dia.com>,
        "shahafs@...dia.com" <shahafs@...dia.com>,
        "artemp@...dia.com" <artemp@...dia.com>,
        "kwankhede@...dia.com" <kwankhede@...dia.com>,
        "ACurrid@...dia.com" <ACurrid@...dia.com>,
        "gmataev@...dia.com" <gmataev@...dia.com>,
        "cjia@...dia.com" <cjia@...dia.com>,
        "mjrosato@...ux.ibm.com" <mjrosato@...ux.ibm.com>,
        "yishaih@...dia.com" <yishaih@...dia.com>,
        "aik@...abs.ru" <aik@...abs.ru>,
        "Zhao, Yan Y" <yan.y.zhao@...el.com>
Subject: Re: [PATCH v2 0/9] Introduce vfio-pci-core subsystem

On Wed, Feb 10, 2021 at 09:37:46AM -0700, Alex Williamson wrote:

> > >  register a migration region and intercept guest writes to specific
> > > registers. [PATCH 4/9] demonstrates the former but not the latter
> > > (which is allowed in v1).  
> > 
> > And this is why, the ROI to wrapper every vfio op in a PCI op just to
> > keep vfio_pci_device completely private is poor :(
> 
> Says someone who doesn't need to maintain the core, fixing bugs and
> adding features, while not breaking vendor driver touching private data
> in unexpected ways ;)

Said as someone that maintains a driver subsystem 25x larger than VFIO
that is really experienced in "crazy things drivers do". :)

Private/public is rarely at the top of my worries, and I'm confident
to say this is the general kernel philosophy. Again, look anywhere, we
rarely have private data split out of major structs like struct
device, struct netdev, struct pci_device, etc. This data has to be
public because we are using C and we expect inline functions,
container_of() and so on to work. It is rarely done with hidden
structs.

If we can get private data in some places it is a nice win, but not
worth making a mess to achieve. eg I would not give up the normal
container_of pattern just to obscure some struct, the overall ROI is
bad.

Drivers always do unexpected and crazy things, I wouldn't get fixated
on touching private data as the worst sin a driver can do :(

So, no, I don't agree that exposing a struct vfio_pci_device is the
end of the world - it is normal in the kernel to do this kind of
thing, and yes drivers can do crazy things with that if crazy slips
past the review process.

Honestly I expect people to test their drivers and fix things if a
core change broke them. It happens, QA finds it, it gets fixed, normal
stuff for Linux, IMHO.

> > > Then what exact extension is talked here by creating another subsystem
> > > module? or are we talking about some general library which can be
> > > shared by underlying mdev device drivers to reduce duplicated
> > > emulation code?  
> > 
> > IMHO it is more a design philosophy that the end driver should
> > implement the vfio_device_ops directly vs having a stack of ops
> > structs.

> Like Kevin though, I don't really understand the hand-wave
> application to mdev.  Sure, vfio-mdev could be collapsed now that
> we've rejected that there could be other drivers binding to mdev
> devices,

Again, I think the point Max was trying to make is only that vfio_mdev
can follow the same design as proposed here and replace the
mdev_parent_ops with the vfio_device_ops.

Jason