Message-ID: <20191120084358-mutt-send-email-mst@kernel.org>
Date:   Wed, 20 Nov 2019 09:15:17 -0500
From:   "Michael S. Tsirkin" <mst@...hat.com>
To:     Jason Gunthorpe <jgg@...pe.ca>
Cc:     Jason Wang <jasowang@...hat.com>,
        Parav Pandit <parav@...lanox.com>,
        Jeff Kirsher <jeffrey.t.kirsher@...el.com>,
        davem@...emloft.net, gregkh@...uxfoundation.org,
        Dave Ertman <david.m.ertman@...el.com>, netdev@...r.kernel.org,
        linux-rdma@...r.kernel.org, nhorman@...hat.com,
        sassmann@...hat.com, Kiran Patil <kiran.patil@...el.com>,
        Alex Williamson <alex.williamson@...hat.com>,
        Tiwei Bie <tiwei.bie@...el.com>
Subject: Re: [net-next v2 1/1] virtual-bus: Implementation of Virtual Bus

On Wed, Nov 20, 2019 at 09:38:35AM -0400, Jason Gunthorpe wrote:
> On Tue, Nov 19, 2019 at 10:59:20PM -0500, Jason Wang wrote:
> 
> > > > The interface between vfio and userspace is
> > > > based on virtio which is IMHO much better than
> > > > a vendor specific one. userspace stays vendor agnostic.
> > > 
> > > Why is that even a good thing? It is much easier to provide drivers
> > > via qemu/etc in user space then it is to make kernel upgrades. We've
> > > learned this lesson many times.
> > 
> > For upgrades, since we had a unified interface. It could be done
> > through:
> > 
> > 1) switch the datapath from hardware to software (e.g vhost)
> > 2) unload and load the driver
> > 3) switch teh datapath back
> > 
> > Having drivers in user space have other issues, there're a lot of
> > customers want to stick to kernel drivers.
> 
> So you want to support upgrade of kernel modules, but runtime
> upgrading the userspace part is impossible? Seems very strange to me.

It's still true, you have to kill userspace to update a shared library.

Not to mention that things like rust encourage static builds so you
can't update a library even if you were willing to risk doing
that.

> > > This is why we have had the philosophy that if it doesn't need to be
> > > in the kernel it should be in userspace.
> > 
> > Let me clarify again. For this framework, it aims to support both
> > kernel driver and userspce driver. For this series, it only contains
> > the kernel driver part. What it did is to allow kernel virtio driver
> > to control vDPA devices. Then we can provide a unified interface for
> > all of the VM, containers and bare metal. For this use case, I don't
> > see a way to leave the driver in userspace other than injecting
> > traffic back through vhost/TAP which is ugly.
> 
> Binding to the other kernel virtio drivers is a reasonable
> justification, but none of this comes through in the patch cover
> letters or patch commit messages.

Yea this could have been communicated better.

> > > > That has lots of security and portability implications and isn't
> > > > appropriate for everyone.
> > > 
> > > This is already using vfio. It doesn't make sense to claim that using
> > > vfio properly is somehow less secure or less portable.
> > > 
> > > What I find particularly ugly is that this 'IFC VF NIC' driver
> > > pretends to be a mediated vfio device, but actually bypasses all the
> > > mediated device ops for managing dma security and just directly plugs
> > > the system IOMMU for the underlying PCI device into vfio.
> > 
> > Well, VFIO have multiple types of API. The design is to stick the VFIO
> > DMA model like container work for making DMA API work for userspace
> > driver.
> 
> Well, it doesn't, that model, for security, is predicated on vfio
> being the exclusive owner of the device. For instance if the kernel
> driver were to perform DMA as well then security would be lost.

The assumption at least IFC driver makes is that the kernel
driver does no DMA.

> > > I suppose this little hack is what is motivating this abuse of vfio in
> > > the first place?
> > > 
> > > Frankly I think a kernel driver touching a PCI function for which vfio
> > > is now controlling the system iommu for is a violation of the security
> > > model, and I'm very surprised AlexW didn't NAK this idea.
> > >
> > > Perhaps it is because none of the patches actually describe how the
> > > DMA security model for this so-called mediated device works? :(
> > >
> > > Or perhaps it is because this submission is split up so much it is
> > > hard to see what is being proposed? (I note this IFC driver is the
> > > first user of the mdev_set_iommu_device() function)
> > 
> > Are you objecting the mdev_set_iommu_deivce() stuffs here?
> 
> I'm questioning if it fits the vfio PCI device security model, yes.

If you look at the IFC patch you'll find it doesn't do DMA, that's
what makes it secure.

> > > > It is kernel's job to abstract hardware away and present a unified
> > > > interface as far as possible.
> > > 
> > > Sure, you could create a virtio accelerator driver framework in our
> > > new drivers/accel I hear was started. That could make some sense, if
> > > we had HW that actually required/benefited from kernel involvement.
> > 
> > The framework is not designed specifically for your card. It tries to be
> > generic to support every types of virtio hardware devices, it's not
> > tied to any bus (e.g PCI) and any vendor. So it's not only a question
> > of how to slice a PCIE ethernet device.
> 
> That doesn't explain why this isn't some new driver subsystem and
> instead treats vfio as a driver multiplexer.
> 
> Jason

That motivation's missing.

-- 
MST

Powered by blists - more mailing lists