Re: [PATCH v3 2/3] uio_pci_generic: add MSI/MSI-X support

[Michael S. Tsirkin]

On Thu, Oct 08, 2015 at 12:44:09PM +0300, Avi Kivity wrote:
> 
> 
> On 10/08/2015 12:16 PM, Michael S. Tsirkin wrote:
> >On Thu, Oct 08, 2015 at 11:46:30AM +0300, Avi Kivity wrote:
> >>
> >>On 10/08/2015 10:32 AM, Michael S. Tsirkin wrote:
> >>>On Thu, Oct 08, 2015 at 08:33:45AM +0300, Avi Kivity wrote:
> >>>>It is good practice to defend against root oopsing the kernel, but in some
> >>>>cases it cannot be achieved.
> >>>Absolutely. That's one of the issues with these patches. They don't even
> >>>try where it's absolutely possible.
> >>>
> >>Are you referring to blocking the maps of the msix BAR areas?
> >For example. There are more. I listed some of the issues on the mailing
> >list, and I might have missed some.  VFIO has code to address all this,
> >people should share code to avoid duplication, or at least read it
> >to understand the issues.
> 
> All but one of those are unrelated to the patch that adds msix support.

They are related because msix support enables bus mastering.  Without it
device is passive and can't harm anyone. With it, suddently you need to
be very careful with the device to avoid corrupting kernel memory.

> >
> >>I think there is value in that.  The value is small because a
> >>corruption is more likely in the dynamic memory responsible for tens
> >>of millions of DMA operations per second, rather than a static 4K
> >>area, but it exists.
> >There are other bugs which will hurt e.g. each time application does not
> >exit gracefully.
> 
> uio_pci_generic disables DMA when the device is removed, so we're safe here,
> at least if files are released before the address space.

No, not really.

You seem to insist on *me* going into VFIO code, digging out
rationale for everything it does and then spelling it out.

If I do it just this once, will you then believe that maybe we don't
have to re-discover all issues and maybe all of VFIO/PCI code shouldn't
just be duplicated in UIO?

The rationale is that when you open the device next, kernel will enable
bus master and if device is in a bad state it might immediately start
doing DMA all over the place.  And it's on open so userspace doesn't
have the chance to bring it to a good state yet.

commit bc4fba77124e2fe4eb14bcb52875c0b0228deace
    vfio-pci: Attempt bus/slot reset on release
fwiw

> >
> >But well, heh :) That's precisely my feeling about the whole "running
> >userspace drivers without an IOMMU" project. The value is small
> >since modern hardware has fast IOMMUs, but it exists.
> >
> 
> For users that don't have iommus at all (usually because it is taken by the
> hypervisor),
> it has great value.

Isn't this what I said? Let me repeat:

	most of the problem you are trying to solve is for
	virtualization - and it is is better addressed at the hypervisor level.
	There are enough opensource hypervisors out there - work on IOMMU
	support there would be time well spent.

http://mid.gmane.org/20151007230553-mutt-send-email-mst@xxxxxxxxxx


> I can't comment on iommu overhead; for my use case it is likely negligible
> and we will use an iommu when available; but apparently it matters for
> others.

You and Vlad are the only ones who brought this up.
So maybe you should not bring it up anymore.

-- 
MST
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/