Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1240490 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v3 2/3] uio_pci_generic: add MSI/MSI-X support "Michael S. Tsirkin" <mst@redhat.com> - 2015-10-06 16:40 +0200
    Re: [PATCH v3 2/3] uio_pci_generic: add MSI/MSI-X support Vlad Zolotarov <vladz@cloudius-systems.com> - 2015-10-06 16:50 +0200
      Re: [PATCH v3 2/3] uio_pci_generic: add MSI/MSI-X support "Michael S. Tsirkin" <mst@redhat.com> - 2015-10-06 17:00 +0200
        Re: [PATCH v3 2/3] uio_pci_generic: add MSI/MSI-X support Vlad Zolotarov <vladz@cloudius-systems.com> - 2015-10-06 17:30 +0200
        Re: [PATCH v3 2/3] uio_pci_generic: add MSI/MSI-X support Avi Kivity <avi@scylladb.com> - 2015-10-06 17:30 +0200
          Re: [PATCH v3 2/3] uio_pci_generic: add MSI/MSI-X support Alex Williamson <alex.williamson@redhat.com> - 2015-10-06 21:00 +0200
            Re: [PATCH v3 2/3] uio_pci_generic: add MSI/MSI-X support Stephen Hemminger <stephen@networkplumber.org> - 2015-10-06 23:40 +0200
              Re: [PATCH v3 2/3] uio_pci_generic: add MSI/MSI-X support Alex Williamson <alex.williamson@redhat.com> - 2015-10-06 23:50 +0200
                Re: [PATCH v3 2/3] uio_pci_generic: add MSI/MSI-X support Vlad Zolotarov <vladz@cloudius-systems.com> - 2015-10-07 10:00 +0200
                Re: [PATCH v3 2/3] uio_pci_generic: add MSI/MSI-X support Vlad Zolotarov <vladz@cloudius-systems.com> - 2015-10-07 10:10 +0200
                  Re: [PATCH v3 2/3] uio_pci_generic: add MSI/MSI-X support Vlad Zolotarov <vladz@cloudius-systems.com> - 2015-10-07 10:10 +0200
            Re: [PATCH v3 2/3] uio_pci_generic: add MSI/MSI-X support Avi Kivity <avi@scylladb.com> - 2015-10-07 09:00 +0200
              Re: [PATCH v3 2/3] uio_pci_generic: add MSI/MSI-X support Avi Kivity <avi@scylladb.com> - 2015-10-07 18:40 +0200
                Re: [PATCH v3 2/3] uio_pci_generic: add MSI/MSI-X support "Michael S. Tsirkin" <mst@redhat.com> - 2015-10-07 23:10 +0200
                  Re: [PATCH v3 2/3] uio_pci_generic: add MSI/MSI-X support Gleb Natapov <gleb@scylladb.com> - 2015-10-08 06:20 +0200
                    Re: [PATCH v3 2/3] uio_pci_generic: add MSI/MSI-X support "Michael S. Tsirkin" <mst@redhat.com> - 2015-10-08 09:50 +0200
                      Re: [PATCH v3 2/3] uio_pci_generic: add MSI/MSI-X support Gleb Natapov <gleb@scylladb.com> - 2015-10-08 10:00 +0200
                        Re: [PATCH v3 2/3] uio_pci_generic: add MSI/MSI-X support "Michael S. Tsirkin" <mst@redhat.com> - 2015-10-08 11:40 +0200
                          Re: [PATCH v3 2/3] uio_pci_generic: add MSI/MSI-X support Gleb Natapov <gleb@scylladb.com> - 2015-10-08 11:50 +0200
                            Re: [PATCH v3 2/3] uio_pci_generic: add MSI/MSI-X support "Michael S. Tsirkin" <mst@redhat.com> - 2015-10-08 14:20 +0200
                  Re: [PATCH v3 2/3] uio_pci_generic: add MSI/MSI-X support Avi Kivity <avi@scylladb.com> - 2015-10-08 07:40 +0200
                    Re: [PATCH v3 2/3] uio_pci_generic: add MSI/MSI-X support "Michael S. Tsirkin" <mst@redhat.com> - 2015-10-08 09:40 +0200
                      Re: [PATCH v3 2/3] uio_pci_generic: add MSI/MSI-X support Avi Kivity <avi@scylladb.com> - 2015-10-08 10:50 +0200
                        Re: [PATCH v3 2/3] uio_pci_generic: add MSI/MSI-X support "Michael S. Tsirkin" <mst@redhat.com> - 2015-10-08 11:20 +0200
                          Re: [PATCH v3 2/3] uio_pci_generic: add MSI/MSI-X support Avi Kivity <avi@scylladb.com> - 2015-10-08 11:50 +0200
                            Re: [PATCH v3 2/3] uio_pci_generic: add MSI/MSI-X support "Michael S. Tsirkin" <mst@redhat.com> - 2015-10-08 14:10 +0200
                              Re: [PATCH v3 2/3] uio_pci_generic: add MSI/MSI-X support Gleb Natapov <gleb@scylladb.com> - 2015-10-08 14:30 +0200
                                Re: [PATCH v3 2/3] uio_pci_generic: add MSI/MSI-X support "Michael S. Tsirkin" <mst@redhat.com> - 2015-10-08 15:30 +0200
                                  Re: [PATCH v3 2/3] uio_pci_generic: add MSI/MSI-X support Gleb Natapov <gleb@scylladb.com> - 2015-10-08 15:30 +0200
                                    Re: [PATCH v3 2/3] uio_pci_generic: add MSI/MSI-X support "Michael S. Tsirkin" <mst@redhat.com> - 2015-10-08 18:50 +0200
                                      Re: [PATCH v3 2/3] uio_pci_generic: add MSI/MSI-X support Gleb Natapov <gleb@scylladb.com> - 2015-10-08 19:10 +0200
                                        Re: [PATCH v3 2/3] uio_pci_generic: add MSI/MSI-X support "Michael S. Tsirkin" <mst@redhat.com> - 2015-10-08 19:40 +0200
                                          Re: [PATCH v3 2/3] uio_pci_generic: add MSI/MSI-X support Gleb Natapov <gleb@scylladb.com> - 2015-10-08 20:00 +0200
                                          Re: [PATCH v3 2/3] uio_pci_generic: add MSI/MSI-X support Greg KH <gregkh@linuxfoundation.org> - 2015-10-08 20:40 +0200
                    Re: [PATCH v3 2/3] uio_pci_generic: add MSI/MSI-X support "Michael S. Tsirkin" <mst@redhat.com> - 2015-10-08 10:40 +0200
                      Re: [PATCH v3 2/3] uio_pci_generic: add MSI/MSI-X support Gleb Natapov <gleb@scylladb.com> - 2015-10-08 11:00 +0200
                      Re: [PATCH v3 2/3] uio_pci_generic: add MSI/MSI-X support Avi Kivity <avi@scylladb.com> - 2015-10-08 11:20 +0200
                        Re: [PATCH v3 2/3] uio_pci_generic: add MSI/MSI-X support "Michael S. Tsirkin" <mst@redhat.com> - 2015-10-08 12:30 +0200
                          Re: [PATCH v3 2/3] uio_pci_generic: add MSI/MSI-X support Avi Kivity <avi@scylladb.com> - 2015-10-08 15:30 +0200
                            Re: [PATCH v3 2/3] uio_pci_generic: add MSI/MSI-X support "Michael S. Tsirkin" <mst@redhat.com> - 2015-10-08 16:20 +0200
                            Re: [PATCH v3 2/3] uio_pci_generic: add MSI/MSI-X support Alex Williamson <alex.williamson@redhat.com> - 2015-10-08 17:40 +0200
              Re: [PATCH v3 2/3] uio_pci_generic: add MSI/MSI-X support Alex Williamson <alex.williamson@redhat.com> - 2015-10-07 18:40 +0200
                Re: [PATCH v3 2/3] uio_pci_generic: add MSI/MSI-X support "Michael S. Tsirkin" <mst@redhat.com> - 2015-10-07 22:10 +0200
            Re: [PATCH v3 2/3] uio_pci_generic: add MSI/MSI-X support Vlad Zolotarov <vladz@cloudius-systems.com> - 2015-10-07 10:00 +0200
              Re: [PATCH v3 2/3] uio_pci_generic: add MSI/MSI-X support "Michael S. Tsirkin" <mst@redhat.com> - 2015-10-08 10:50 +0200

Page 2 of 3 — ← Prev page 1 [2] 3  Next page →

#1241972

On 08/10/15 00:05, Michael S. Tsirkin wrote:
> On Wed, Oct 07, 2015 at 07:39:16PM +0300, Avi Kivity wrote:
>> That's what I thought as well, but apparently adding msix support to the
>> already insecure uio drivers is even worse.
> I'm glad you finally agree what these drivers are doing is insecure.
>
> And basically kernel cares about security, no one wants to maintain insecure stuff.
>
> So you guys should think harder whether this code makes any sense upstream.

You simply ignore everything I write, cherry-picking the word "insecure" 
as if it makes your point.  That is very frustrating.

The kernel is not secure against root, even in the restricted "will it 
oops" sense.  You can oops it easily, try dd if=/dev/urandom of=/dev/mem 
(or of=/dev/sda for a more satisfying oops).

> Getting support from kernel is probably the biggest reason to put code
> upstream, and this driver taints kernel unconditionally so you don't get
> that.

The biggest reason is that if a driver gets upstream, in a year or two 
it is universally available.


> Alternatively, 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.

It is not.  The problem we are trying to solve, and please consider the 
following as if written in all caps, is that some configurations do not 
have an iommu or cannot use it for performance reasons.

It is good practice to defend against root oopsing the kernel, but in 
some cases it cannot be achieved.  A trivial example is a nommu kernel, 
this is another.  In these cases we can give up on this goal, because it 
is not the only reason for the kernel's existence, there are others.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1242019

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.

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

[toc] | [prev] | [next] | [standalone]

#1242077

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?

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

[toc] | [prev] | [next] | [standalone]

#1242101

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.

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

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.

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

[toc] | [prev] | [next] | [standalone]

#1242124

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.

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

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

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

[toc] | [prev] | [next] | [standalone]

#1242286

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@redhat.com


> 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@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1242296

On Thu, Oct 08, 2015 at 03:06:07PM +0300, Michael S. Tsirkin wrote:
> 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.
> 
Most (if not all) uio_pci_generic users enable pci bus mastering. The
fact that they do that without even tainting the kernel like the patch
does make current situation much worse that with the patch.

> > 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.
> 
Common, you were CCed to at least this one:

 We have a solution that makes use of IOMMU support with vfio.  The 
 problem is there are multiple cases where that support is either not 
 available, or using the IOMMU provides excess overhead.


http://dpdk.org/ml/archives/dev/2015-October/024560.html

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

[toc] | [prev] | [next] | [standalone]

#1242337

On Thu, Oct 08, 2015 at 03:27:37PM +0300, Gleb Natapov wrote:
> On Thu, Oct 08, 2015 at 03:06:07PM +0300, Michael S. Tsirkin wrote:
> > 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.
> > 
> Most (if not all) uio_pci_generic users enable pci bus mastering. The
> fact that they do that without even tainting the kernel like the patch
> does make current situation much worse that with the patch.

It isn't worse. It's a sane interface. Whoever enables bus mastering
must be careful.  If userspace enables bus mastering then userspace
needs to be very careful with the device to avoid corrupting kernel
memory.  If kernel does it, it's kernel's responsibility.

> > > 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.
> > 
> Common, you were CCed to at least this one:
> 
>  We have a solution that makes use of IOMMU support with vfio.  The 
>  problem is there are multiple cases where that support is either not 
>  available, or using the IOMMU provides excess overhead.
> 
> 
> http://dpdk.org/ml/archives/dev/2015-October/024560.html

Thanks for the correction.  I didn't notice that one, and I
misunderstood Avi's comment to mean it's just a theoretical case (taking
"apparently" to mean "maybe").  So someone else did bring it up, it's
not just Avi and Vlad.  I'm sorry, I take my comment back.  It might
help to mention "iommu overhead on pre ivy-bridge x86 systems" - that is
what this email seems to refer to.

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

[toc] | [prev] | [next] | [standalone]

#1242340

On Thu, Oct 08, 2015 at 04:20:04PM +0300, Michael S. Tsirkin wrote:
> On Thu, Oct 08, 2015 at 03:27:37PM +0300, Gleb Natapov wrote:
> > On Thu, Oct 08, 2015 at 03:06:07PM +0300, Michael S. Tsirkin wrote:
> > > 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.
> > > 
> > Most (if not all) uio_pci_generic users enable pci bus mastering. The
> > fact that they do that without even tainting the kernel like the patch
> > does make current situation much worse that with the patch.
> 
> It isn't worse. It's a sane interface. Whoever enables bus mastering
> must be careful.  If userspace enables bus mastering then userspace
> needs to be very careful with the device to avoid corrupting kernel
> memory.  If kernel does it, it's kernel's responsibility.
> 
Although this definition of sanity sounds strange to me, but lets
flow with it for the sake of this email: would it be OK if proposed
interface refused to work if bus mastering is not already enabled by
userspace?
 
--
			Gleb.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1242597

On Thu, Oct 08, 2015 at 04:28:34PM +0300, Gleb Natapov wrote:
> On Thu, Oct 08, 2015 at 04:20:04PM +0300, Michael S. Tsirkin wrote:
> > On Thu, Oct 08, 2015 at 03:27:37PM +0300, Gleb Natapov wrote:
> > > On Thu, Oct 08, 2015 at 03:06:07PM +0300, Michael S. Tsirkin wrote:
> > > > 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.
> > > > 
> > > Most (if not all) uio_pci_generic users enable pci bus mastering. The
> > > fact that they do that without even tainting the kernel like the patch
> > > does make current situation much worse that with the patch.
> > 
> > It isn't worse. It's a sane interface. Whoever enables bus mastering
> > must be careful.  If userspace enables bus mastering then userspace
> > needs to be very careful with the device to avoid corrupting kernel
> > memory.  If kernel does it, it's kernel's responsibility.
> > 
> Although this definition of sanity sounds strange to me, but lets
> flow with it for the sake of this email: would it be OK if proposed
> interface refused to work if bus mastering is not already enabled by
> userspace?

An interface could be acceptable if there's a fallback where it
works without BM but slower (e.g. poll pending bits).

But not the proposed one.

Really, there's more to making msi-x work with
userspace drivers than this patch. As I keep telling people, you would
basically reimplement vfio/pci. Go over it, and see for yourself.
Almost everything it does is relevant for msi-x.  It's just wrong to
duplicate so much code.


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

[toc] | [prev] | [next] | [standalone]

#1242618

On Thu, Oct 08, 2015 at 07:43:04PM +0300, Michael S. Tsirkin wrote:
> On Thu, Oct 08, 2015 at 04:28:34PM +0300, Gleb Natapov wrote:
> > On Thu, Oct 08, 2015 at 04:20:04PM +0300, Michael S. Tsirkin wrote:
> > > On Thu, Oct 08, 2015 at 03:27:37PM +0300, Gleb Natapov wrote:
> > > > On Thu, Oct 08, 2015 at 03:06:07PM +0300, Michael S. Tsirkin wrote:
> > > > > 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.
> > > > > 
> > > > Most (if not all) uio_pci_generic users enable pci bus mastering. The
> > > > fact that they do that without even tainting the kernel like the patch
> > > > does make current situation much worse that with the patch.
> > > 
> > > It isn't worse. It's a sane interface. Whoever enables bus mastering
> > > must be careful.  If userspace enables bus mastering then userspace
> > > needs to be very careful with the device to avoid corrupting kernel
> > > memory.  If kernel does it, it's kernel's responsibility.
> > > 
> > Although this definition of sanity sounds strange to me, but lets
> > flow with it for the sake of this email: would it be OK if proposed
> > interface refused to work if bus mastering is not already enabled by
> > userspace?
> 
> An interface could be acceptable if there's a fallback where it
> works without BM but slower (e.g. poll pending bits).
> 
OK.

> But not the proposed one.
>
Why? Greg is against ioctl interface so it will be reworked, by besides
that what is wrong with the concept of binding msi-x interrupt to
eventfd?
 
> Really, there's more to making msi-x work with
> userspace drivers than this patch. As I keep telling people, you would
> basically reimplement vfio/pci. Go over it, and see for yourself.
> Almost everything it does is relevant for msi-x.  It's just wrong to
> duplicate so much code.
> 
The patch is tested and works with msi-x. Restricting access to msi-x
registers that vfio does is not relevant here.

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

[toc] | [prev] | [next] | [standalone]

#1242638

On Thu, Oct 08, 2015 at 08:01:21PM +0300, Gleb Natapov wrote:
> On Thu, Oct 08, 2015 at 07:43:04PM +0300, Michael S. Tsirkin wrote:
> > On Thu, Oct 08, 2015 at 04:28:34PM +0300, Gleb Natapov wrote:
> > > On Thu, Oct 08, 2015 at 04:20:04PM +0300, Michael S. Tsirkin wrote:
> > > > On Thu, Oct 08, 2015 at 03:27:37PM +0300, Gleb Natapov wrote:
> > > > > On Thu, Oct 08, 2015 at 03:06:07PM +0300, Michael S. Tsirkin wrote:
> > > > > > 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.
> > > > > > 
> > > > > Most (if not all) uio_pci_generic users enable pci bus mastering. The
> > > > > fact that they do that without even tainting the kernel like the patch
> > > > > does make current situation much worse that with the patch.
> > > > 
> > > > It isn't worse. It's a sane interface. Whoever enables bus mastering
> > > > must be careful.  If userspace enables bus mastering then userspace
> > > > needs to be very careful with the device to avoid corrupting kernel
> > > > memory.  If kernel does it, it's kernel's responsibility.
> > > > 
> > > Although this definition of sanity sounds strange to me, but lets
> > > flow with it for the sake of this email: would it be OK if proposed
> > > interface refused to work if bus mastering is not already enabled by
> > > userspace?
> > 
> > An interface could be acceptable if there's a fallback where it
> > works without BM but slower (e.g. poll pending bits).
> > 
> OK.
> 
> > But not the proposed one.
> >
> Why? Greg is against ioctl interface so it will be reworked, by besides
> that what is wrong with the concept of binding msi-x interrupt to
> eventfd?

It's not the binding. Managing msi-x just needs more than the puny
2 ioctls to get # of vectors and set eventfd.

It interacts in strange ways with reset, and with PM, and ...

> > Really, there's more to making msi-x work with
> > userspace drivers than this patch. As I keep telling people, you would
> > basically reimplement vfio/pci. Go over it, and see for yourself.
> > Almost everything it does is relevant for msi-x.  It's just wrong to
> > duplicate so much code.
> > 
> The patch is tested and works with msi-x. Restricting access to msi-x
> registers that vfio does is not relevant here.

It works *for you* with a specific userspace application. I have no idea
how you tested it, and what does the userspace in question do.  But it
seems pretty clear that there are a ton of very reasonable things that
one can do with a device and that break when you enable MSI-X.

You need to find a way to share that logic with vfio/pci.

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

[toc] | [prev] | [next] | [standalone]

#1242647

On Thu, Oct 08, 2015 at 08:39:10PM +0300, Michael S. Tsirkin wrote:
> On Thu, Oct 08, 2015 at 08:01:21PM +0300, Gleb Natapov wrote:
> > On Thu, Oct 08, 2015 at 07:43:04PM +0300, Michael S. Tsirkin wrote:
> > > On Thu, Oct 08, 2015 at 04:28:34PM +0300, Gleb Natapov wrote:
> > > > On Thu, Oct 08, 2015 at 04:20:04PM +0300, Michael S. Tsirkin wrote:
> > > > > On Thu, Oct 08, 2015 at 03:27:37PM +0300, Gleb Natapov wrote:
> > > > > > On Thu, Oct 08, 2015 at 03:06:07PM +0300, Michael S. Tsirkin wrote:
> > > > > > > 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.
> > > > > > > 
> > > > > > Most (if not all) uio_pci_generic users enable pci bus mastering. The
> > > > > > fact that they do that without even tainting the kernel like the patch
> > > > > > does make current situation much worse that with the patch.
> > > > > 
> > > > > It isn't worse. It's a sane interface. Whoever enables bus mastering
> > > > > must be careful.  If userspace enables bus mastering then userspace
> > > > > needs to be very careful with the device to avoid corrupting kernel
> > > > > memory.  If kernel does it, it's kernel's responsibility.
> > > > > 
> > > > Although this definition of sanity sounds strange to me, but lets
> > > > flow with it for the sake of this email: would it be OK if proposed
> > > > interface refused to work if bus mastering is not already enabled by
> > > > userspace?
> > > 
> > > An interface could be acceptable if there's a fallback where it
> > > works without BM but slower (e.g. poll pending bits).
> > > 
> > OK.
> > 
> > > But not the proposed one.
> > >
> > Why? Greg is against ioctl interface so it will be reworked, by besides
> > that what is wrong with the concept of binding msi-x interrupt to
> > eventfd?
> 
> It's not the binding. Managing msi-x just needs more than the puny
> 2 ioctls to get # of vectors and set eventfd.
> 
> It interacts in strange ways with reset, and with PM, and ...
> 
Sorry, I need examples of what you mean. DMA also "interacts in strange
ways with reset, and with PM, and ..." and it does not have any special
handling anywhere in uio-generic. So what special properties msi-x posses
which are not part of a dma. We already agreed that if enabling of bus
mastering is done by userspace all the responsibilities pertaining to
it are also lay in userspace.

> > > Really, there's more to making msi-x work with
> > > userspace drivers than this patch. As I keep telling people, you would
> > > basically reimplement vfio/pci. Go over it, and see for yourself.
> > > Almost everything it does is relevant for msi-x.  It's just wrong to
> > > duplicate so much code.
> > > 
> > The patch is tested and works with msi-x. Restricting access to msi-x
> > registers that vfio does is not relevant here.
> 
> It works *for you* with a specific userspace application. I have no idea
> how you tested it, and what does the userspace in question do.  But it
> seems pretty clear that there are a ton of very reasonable things that
> one can do with a device and that break when you enable MSI-X.
> 
I do not follow. What things break when you enable MSI-X and why would
you enable MSI-X if things that previously worked breaks for you.
Look we cannot work with such vague statements, please be more specific
about issues that needs to be addressed. So far I got two:

 1. kernel should not enable pci bust mastering, leave it to userspace
to do before configuring msi-x
 2. if bus mustering is disabled then poll for interrupts

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

[toc] | [prev] | [next] | [standalone]

#1242681

On Thu, Oct 08, 2015 at 08:39:10PM +0300, Michael S. Tsirkin wrote:
> > Why? Greg is against ioctl interface so it will be reworked, by besides
> > that what is wrong with the concept of binding msi-x interrupt to
> > eventfd?
> 
> It's not the binding. Managing msi-x just needs more than the puny
> 2 ioctls to get # of vectors and set eventfd.
> 
> It interacts in strange ways with reset, and with PM, and ...

Can we please drop this thread right now.  The proposed patches are not
acceptable as-is, and everyone knows that.  The developers are going to
go off and redo things and propose a new set of patches, let's see what
the result is of that work and we can take it from there.

Random complaints about this existing patch is not useful at all
anymore, there's nothing needed to convince anyone about anything here.

thanks,

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

[toc] | [prev] | [next] | [standalone]

#1242053

On Thu, Oct 08, 2015 at 08:33:45AM +0300, Avi Kivity wrote:
> On 08/10/15 00:05, Michael S. Tsirkin wrote:
> >On Wed, Oct 07, 2015 at 07:39:16PM +0300, Avi Kivity wrote:
> >>That's what I thought as well, but apparently adding msix support to the
> >>already insecure uio drivers is even worse.
> >I'm glad you finally agree what these drivers are doing is insecure.
> >
> >And basically kernel cares about security, no one wants to maintain insecure stuff.
> >
> >So you guys should think harder whether this code makes any sense upstream.
> 
> You simply ignore everything I write, cherry-picking the word "insecure" as
> if it makes your point.  That is very frustrating.

And I'm sorry about the frustration.  I didn't intend to twist your
words. It's just that I had to spend literally hours trying to explain
that security matters in kernel, and all I was getting back was a
summary "there's no security issue because there are other way to
corrupt memory".

So I was glad when it looked like there's finally an agreement that yes,
there's value in validating userspace input and yes, it's insecure
not to do this.

> It is good practice to defend against root oopsing the kernel, but in some
> cases it cannot be achieved.

I originally included ways to fix issues that I pointed out, ranging
from harder to implement with more overhead but more secure to easier to
implement with less overhead but less secure.  There didn't seem to be
an understanding that the issues are there at all, so I stopped doing
that - seemed like a waste of time.

For example, will it kill your performance to reset devices cleanly, on
open and close, protect them from writes into MSI config, BAR registers
and related capablities etc etc?  And if not, why are you people wasting
time arguing about that?  The only thing I heard is that it's a hassle.
That's true (though if you follow my advice and try to share code with
vfio/pci you get a lot of this logic for free).  So it's an
understandable argument if you just need something that works, quickly.
But if it's such a stopgap hack, there's no need to insist on it
upstream.

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

[toc] | [prev] | [next] | [standalone]

#1242089

On Thu, Oct 08, 2015 at 11:32:50AM +0300, Michael S. Tsirkin wrote:
> On Thu, Oct 08, 2015 at 08:33:45AM +0300, Avi Kivity wrote:
> > On 08/10/15 00:05, Michael S. Tsirkin wrote:
> > >On Wed, Oct 07, 2015 at 07:39:16PM +0300, Avi Kivity wrote:
> > >>That's what I thought as well, but apparently adding msix support to the
> > >>already insecure uio drivers is even worse.
> > >I'm glad you finally agree what these drivers are doing is insecure.
> > >
> > >And basically kernel cares about security, no one wants to maintain insecure stuff.
> > >
> > >So you guys should think harder whether this code makes any sense upstream.
> > 
> > You simply ignore everything I write, cherry-picking the word "insecure" as
> > if it makes your point.  That is very frustrating.
> 
> And I'm sorry about the frustration.  I didn't intend to twist your
> words. It's just that I had to spend literally hours trying to explain
> that security matters in kernel, and all I was getting back was a
> summary "there's no security issue because there are other way to
> corrupt memory".
> 
That's not the (only) answer that you were given. The answers that
you constantly ignore is that the patch in question does not add any
new ways to corrupt memory which are not possible using _upstream_
uio_pci_generic device, so the fact that uio_pci_generic can corrupt
memory cannot be used as a reason to not apply patches that do not corrupt
any memory. You seams to be constantly arguing that uio_pci_generic is
not suitable for upstream.

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

[toc] | [prev] | [next] | [standalone]

#1242104

On 10/08/2015 11:32 AM, Michael S. Tsirkin wrote:
> On Thu, Oct 08, 2015 at 08:33:45AM +0300, Avi Kivity wrote:
>> On 08/10/15 00:05, Michael S. Tsirkin wrote:
>>> On Wed, Oct 07, 2015 at 07:39:16PM +0300, Avi Kivity wrote:
>>>> That's what I thought as well, but apparently adding msix support to the
>>>> already insecure uio drivers is even worse.
>>> I'm glad you finally agree what these drivers are doing is insecure.
>>>
>>> And basically kernel cares about security, no one wants to maintain insecure stuff.
>>>
>>> So you guys should think harder whether this code makes any sense upstream.
>> You simply ignore everything I write, cherry-picking the word "insecure" as
>> if it makes your point.  That is very frustrating.
> And I'm sorry about the frustration.  I didn't intend to twist your
> words. It's just that I had to spend literally hours trying to explain
> that security matters in kernel, and all I was getting back was a
> summary "there's no security issue because there are other way to
> corrupt memory".

The word security has several meanings.  The primary meaning is "defense 
against a malicious attacker".  In that sense, there is no added value 
at all, because the attacker is already root, and can already access all 
of kernel and user memory.  Even if the attacker is not root, and just 
has access to a non-iommu-protected device, they can still DMA to and 
from any memory they like.

This sense of the word however is irrelevant for this conversation; the 
user already gave up on it when they chose to use uio_pci_generic 
(either because they have no iommu, or because they need the extra 
performance).

Do we agree that security, in the sense of defense against a malicious 
attacker, is irrelevant for this conversation?

A secondary meaning is protection against inadvertent bugs.  Yes, a 
faulty memory write that happens to land in the msix page, can cause a 
random memory word to be overwritten.  But so can a faulty memory write 
into the rings, or the data structures that support virtual->physical 
translation, the data structures that describe the packets before 
translation, the memory allocator or pool.  The patch extends the 
vulnerable surface, but by a negligible amount.

>
> So I was glad when it looked like there's finally an agreement that yes,
> there's value in validating userspace input and yes, it's insecure
> not to do this.



>
>> It is good practice to defend against root oopsing the kernel, but in some
>> cases it cannot be achieved.
> I originally included ways to fix issues that I pointed out, ranging
> from harder to implement with more overhead but more secure to easier to
> implement with less overhead but less secure.  There didn't seem to be
> an understanding that the issues are there at all, so I stopped doing
> that - seemed like a waste of time.
>
> For example, will it kill your performance to reset devices cleanly, on
> open and close,

I don't recall this being mentioned at all.  It seems completely 
unrelated to a patch adding msix support to uio_pci_generic.

>   protect them from writes into MSI config, BAR registers
> and related capablities etc etc?

Obviously the userspace driver has to write to the BAR area.

If you're talking about the BAR setup registers, yes there is some 
(tiny) value in that, but how is it related to this patch?

Protecting the MSI area in the BARs _is_ related to the patch.  I agree 
it adds value, if small.

>    And if not, why are you people wasting
> time arguing about that?

I you want to use your position as maintainer of uio_pci_generic to get 
people to overhaul the driver for you with unrelated changes, they will 
object.  I can understand a maintainer pointing out the right way to do 
something rather than the wrong way.  But piling on a list of unrelated 
features as prerequisites is, in my opinion, abuse.

Let me repeat that pci_uio_generic is already used for userspace 
drivers, with all the issues that you point out, for a long while now. 
These issues are not exposed by the requirement to use msix. You are not 
protecting the kernel in any way by blocking the patch, you are only 
protecting people with iommu-less configurations from using their hardware.

>    The only thing I heard is that it's a hassle.
> That's true (though if you follow my advice and try to share code with
> vfio/pci you get a lot of this logic for free).

My thinking was that vfio was for secure (in the "defense against 
malicious attackers" sense) while uio_pci_generic was, de-facto at 
least, for use by trusted users.

We are in the strange situation that the Alex is open to adding an 
insecure mode to vfio, while you object to a patch which does not change 
the security of uio_pci_generic in any way; it only makes it more usable 
at the cost of a tiny increase in the bug surface.

>    So it's an
> understandable argument if you just need something that works, quickly.
> But if it's such a stopgap hack, there's no need to insist on it
> upstream.

It is not more or less a hack than uio_pci_generic allowing DMA, or 
/dev/mem, or the module loading interface, or nommu kernels. Security is 
just one aspect of the kernel, not the only one.

It's perfectly reasonable to taint the kernel when insecure DMA is 
enabled, and to allow the administrator to disable the interface 
completely.  What I don't understand is why, given that the user allows 
DMA, we should prevent them from using MSIX in addition.

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

[toc] | [prev] | [next] | [standalone]

#1242172

On Thu, Oct 08, 2015 at 12:19:20PM +0300, Avi Kivity wrote:
> 
> 
> On 10/08/2015 11:32 AM, Michael S. Tsirkin wrote:
> >On Thu, Oct 08, 2015 at 08:33:45AM +0300, Avi Kivity wrote:
> >>On 08/10/15 00:05, Michael S. Tsirkin wrote:
> >>>On Wed, Oct 07, 2015 at 07:39:16PM +0300, Avi Kivity wrote:
> >>>>That's what I thought as well, but apparently adding msix support to the
> >>>>already insecure uio drivers is even worse.
> >>>I'm glad you finally agree what these drivers are doing is insecure.
> >>>
> >>>And basically kernel cares about security, no one wants to maintain insecure stuff.
> >>>
> >>>So you guys should think harder whether this code makes any sense upstream.
> >>You simply ignore everything I write, cherry-picking the word "insecure" as
> >>if it makes your point.  That is very frustrating.
> >And I'm sorry about the frustration.  I didn't intend to twist your
> >words. It's just that I had to spend literally hours trying to explain
> >that security matters in kernel, and all I was getting back was a
> >summary "there's no security issue because there are other way to
> >corrupt memory".
> 
> The word security has several meanings.  The primary meaning is "defense
> against a malicious attacker".  In that sense, there is no added value at
> all, because the attacker is already root, and can already access all of
> kernel and user memory.  Even if the attacker is not root, and just has
> access to a non-iommu-protected device, they can still DMA to and from any
> memory they like.
> 
> This sense of the word however is irrelevant for this conversation; the user
> already gave up on it when they chose to use uio_pci_generic (either because
> they have no iommu, or because they need the extra performance).
> 
> Do we agree that security, in the sense of defense against a malicious
> attacker, is irrelevant for this conversation?

No. uio_pci_generic currently can be used in a secure way in
a sense that it's protected againt malicious attacker,
assuming you bind it to a device that does not do DMA.


> A secondary meaning is protection against inadvertent bugs.  Yes, a faulty
> memory write that happens to land in the msix page, can cause a random
> memory word to be overwritten.  But so can a faulty memory write into the
> rings, or the data structures that support virtual->physical translation,
> the data structures that describe the packets before translation, the memory
> allocator or pool.  The patch extends the vulnerable surface, but by a
> negligible amount.
> 
> >
> >So I was glad when it looked like there's finally an agreement that yes,
> >there's value in validating userspace input and yes, it's insecure
> >not to do this.
> 
> 
> 
> >
> >>It is good practice to defend against root oopsing the kernel, but in some
> >>cases it cannot be achieved.
> >I originally included ways to fix issues that I pointed out, ranging
> >from harder to implement with more overhead but more secure to easier to
> >implement with less overhead but less secure.  There didn't seem to be
> >an understanding that the issues are there at all, so I stopped doing
> >that - seemed like a waste of time.
> >
> >For example, will it kill your performance to reset devices cleanly, on
> >open and close,
> 
> I don't recall this being mentioned at all.

http://mid.gmane.org/20151006005527-mutt-send-email-mst@redhat.com

But really, this is just off the top of my head.
These are all issues VFIO developers encountered
and fixed over the years. Go into that code, read it,
and you will discover the issues and the solutions.

>  It seems completely unrelated
> to a patch adding msix support to uio_pci_generic.

It isn't unrelated. It's because with MSIX patch you are enabling bus
mastering in kernel.  So if you start device in a bad state it will
corrupt kernel memory.

> >  protect them from writes into MSI config, BAR registers
> >and related capablities etc etc?
> 
> Obviously the userspace driver has to write to the BAR area.
> 
> If you're talking about the BAR setup registers, yes there is some (tiny)
> value in that, but how is it related to this patch?

If you don't, moving BARs will move the MSI-X region and
protecting it won't help.

> Protecting the MSI area in the BARs _is_ related to the patch.  I agree it
> adds value, if small.
> 
> >   And if not, why are you people wasting
> >time arguing about that?
> 
> I you want to use your position as maintainer of uio_pci_generic to get
> people to overhaul the driver for you with unrelated changes, they will
> object.  I can understand a maintainer pointing out the right way to do
> something rather than the wrong way.  But piling on a list of unrelated
> features as prerequisites is, in my opinion, abuse.

I don't see them as unrelated.  Basically you want to turn
uio_pci_generic into vfio/pci except without an IOMMU.  You will need a
lot of VFIO code then.  That will need a lot of work.  You seem to blame
me for this but IMHO that's because patch author has chosen a wrong
approach.

> Let me repeat that pci_uio_generic is already used for userspace drivers,
> with all the issues that you point out, for a long while now. These issues
> are not exposed by the requirement to use msix.

I answered this already. I don't agree with this.

> You are not protecting the
> kernel in any way by blocking the patch, you are only protecting people with
> iommu-less configurations from using their hardware.

Because it's either this patch or nothing at all? I don't believe that.
Someone come along and write a better one.

> >   The only thing I heard is that it's a hassle.
> >That's true (though if you follow my advice and try to share code with
> >vfio/pci you get a lot of this logic for free).
> 
> My thinking was that vfio was for secure (in the "defense against malicious
> attackers" sense) while uio_pci_generic was, de-facto at least, for use by
> trusted users.

And some are using it in very broken ways. Yes. But now you want
to fix this in stone by tying a kernel/userspace interface
to their broken ways. I think that would be a mistake.

> We are in the strange situation that the Alex is open to adding an insecure
> mode to vfio, 

I don't find this strange. It seems to make sense. VFIO is
already used with DMA capable devices.

> while you object to a patch which does not change the security
> of uio_pci_generic in any way; it only makes it more usable at the cost of a
> tiny increase in the bug surface.

I don't agree with this either. This depends on the device.

> >   So it's an
> >understandable argument if you just need something that works, quickly.
> >But if it's such a stopgap hack, there's no need to insist on it
> >upstream.
> 
> It is not more or less a hack than uio_pci_generic allowing DMA,

It doesn't. sysfs does.

> or
> /dev/mem, or the module loading interface, or nommu kernels. Security is
> just one aspect of the kernel, not the only one.
>
> It's perfectly reasonable to taint the kernel when insecure DMA is enabled,
> and to allow the administrator to disable the interface completely.  What I
> don't understand is why, given that the user allows DMA, we should prevent
> them from using MSIX in addition.

There's no need to prevent MSIX with or without DMA.

But UIO uses sysfs for device access. So if we program MSIX we need to
extend sysfs to protect a ton of registers that are MSIX related from
the user, and do a bunch of setup and cleanup otherwise kernel will be
very confused.

It might be surprising to you how many registers are MSIX related,
but it's true.

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

[toc] | [prev] | [next] | [standalone]

#1242343

On 10/08/2015 01:26 PM, Michael S. Tsirkin wrote:
> On Thu, Oct 08, 2015 at 12:19:20PM +0300, Avi Kivity wrote:
>>
>> On 10/08/2015 11:32 AM, Michael S. Tsirkin wrote:
>>> On Thu, Oct 08, 2015 at 08:33:45AM +0300, Avi Kivity wrote:
>>>> On 08/10/15 00:05, Michael S. Tsirkin wrote:
>>>>> On Wed, Oct 07, 2015 at 07:39:16PM +0300, Avi Kivity wrote:
>>>>>> That's what I thought as well, but apparently adding msix support to the
>>>>>> already insecure uio drivers is even worse.
>>>>> I'm glad you finally agree what these drivers are doing is insecure.
>>>>>
>>>>> And basically kernel cares about security, no one wants to maintain insecure stuff.
>>>>>
>>>>> So you guys should think harder whether this code makes any sense upstream.
>>>> You simply ignore everything I write, cherry-picking the word "insecure" as
>>>> if it makes your point.  That is very frustrating.
>>> And I'm sorry about the frustration.  I didn't intend to twist your
>>> words. It's just that I had to spend literally hours trying to explain
>>> that security matters in kernel, and all I was getting back was a
>>> summary "there's no security issue because there are other way to
>>> corrupt memory".
>> The word security has several meanings.  The primary meaning is "defense
>> against a malicious attacker".  In that sense, there is no added value at
>> all, because the attacker is already root, and can already access all of
>> kernel and user memory.  Even if the attacker is not root, and just has
>> access to a non-iommu-protected device, they can still DMA to and from any
>> memory they like.
>>
>> This sense of the word however is irrelevant for this conversation; the user
>> already gave up on it when they chose to use uio_pci_generic (either because
>> they have no iommu, or because they need the extra performance).
>>
>> Do we agree that security, in the sense of defense against a malicious
>> attacker, is irrelevant for this conversation?
> No. uio_pci_generic currently can be used in a secure way in
> a sense that it's protected againt malicious attacker,
> assuming you bind it to a device that does not do DMA.

The context of the conversation is dpdk, which only supports DMA.

Do we agree that security, in the sense of defense against a malicious 
attacker, is irrelevant for this conversation, taking this under 
consideration?


>
>> A secondary meaning is protection against inadvertent bugs.  Yes, a faulty
>> memory write that happens to land in the msix page, can cause a random
>> memory word to be overwritten.  But so can a faulty memory write into the
>> rings, or the data structures that support virtual->physical translation,
>> the data structures that describe the packets before translation, the memory
>> allocator or pool.  The patch extends the vulnerable surface, but by a
>> negligible amount.
>>
>>> So I was glad when it looked like there's finally an agreement that yes,
>>> there's value in validating userspace input and yes, it's insecure
>>> not to do this.
>>
>>
>>>> It is good practice to defend against root oopsing the kernel, but in some
>>>> cases it cannot be achieved.
>>> I originally included ways to fix issues that I pointed out, ranging
>> >from harder to implement with more overhead but more secure to easier to
>>> implement with less overhead but less secure.  There didn't seem to be
>>> an understanding that the issues are there at all, so I stopped doing
>>> that - seemed like a waste of time.
>>>
>>> For example, will it kill your performance to reset devices cleanly, on
>>> open and close,
>> I don't recall this being mentioned at all.
> http://mid.gmane.org/20151006005527-mutt-send-email-mst@redhat.com

Down at the moment for me.

> But really, this is just off the top of my head.
> These are all issues VFIO developers encountered
> and fixed over the years. Go into that code, read it,
> and you will discover the issues and the solutions.

vfio is solving a different problem, the problem of security against a 
malicious attacker, one that I'm hoping to agree here that we aren't 
attempting to solve.

People have been happily using uio_pci_generic despite all those 
issues.  All they were missing was msix support.  You can't use that to 
force them to overhaul that driver, or to add a new subsystem to vfio.

>
>>   It seems completely unrelated
>> to a patch adding msix support to uio_pci_generic.
> It isn't unrelated. It's because with MSIX patch you are enabling bus
> mastering in kernel.  So if you start device in a bad state it will
> corrupt kernel memory.

You are right, this patch can regress secure users.

I'd be surprised if there are msix-capable pci devices that do not rely 
on DMA, though.

>
>>>   protect them from writes into MSI config, BAR registers
>>> and related capablities etc etc?
>> Obviously the userspace driver has to write to the BAR area.
>>
>> If you're talking about the BAR setup registers, yes there is some (tiny)
>> value in that, but how is it related to this patch?
> If you don't, moving BARs will move the MSI-X region and
> protecting it won't help.

Won't it just become invisible if you do?

If userspace starts playing with BARs, you lost already, whether msix is 
enabled or not doesn't matter.  It can shadow other BARs, for example.

This is a general weakness of uio_pci_generic, not something exposed by 
this patch.

>
>> Protecting the MSI area in the BARs _is_ related to the patch.  I agree it
>> adds value, if small.
>>
>>>    And if not, why are you people wasting
>>> time arguing about that?
>> I you want to use your position as maintainer of uio_pci_generic to get
>> people to overhaul the driver for you with unrelated changes, they will
>> object.  I can understand a maintainer pointing out the right way to do
>> something rather than the wrong way.  But piling on a list of unrelated
>> features as prerequisites is, in my opinion, abuse.
> I don't see them as unrelated.  Basically you want to turn
> uio_pci_generic into vfio/pci except without an IOMMU.

That is not what we want.  Simply adding msix support is sufficient for 
us.  Everything else was piled on afterwards.

> You will need a
> lot of VFIO code then.  That will need a lot of work.  You seem to blame
> me for this but IMHO that's because patch author has chosen a wrong
> approach.
>
>> Let me repeat that pci_uio_generic is already used for userspace drivers,
>> with all the issues that you point out, for a long while now. These issues
>> are not exposed by the requirement to use msix.
> I answered this already. I don't agree with this.

With the first sentence or the second?  I'm trying really hard to 
understand the problem.

>
>> You are not protecting the
>> kernel in any way by blocking the patch, you are only protecting people with
>> iommu-less configurations from using their hardware.
> Because it's either this patch or nothing at all? I don't believe that.
> Someone come along and write a better one.

 From your description, I can't imagine what the better patch looks 
like, except as a complete overhaul of uio_pci_generic.

Our requirement is to enable msix, not to pretend that it is secure 
while it allows DMA to any piece of memory in the system.

>
>>>    The only thing I heard is that it's a hassle.
>>> That's true (though if you follow my advice and try to share code with
>>> vfio/pci you get a lot of this logic for free).
>> My thinking was that vfio was for secure (in the "defense against malicious
>> attackers" sense) while uio_pci_generic was, de-facto at least, for use by
>> trusted users.
> And some are using it in very broken ways. Yes. But now you want
> to fix this in stone by tying a kernel/userspace interface
> to their broken ways. I think that would be a mistake.

At heart, the brokenness here is that you allow insecure DMA. No amount 
of changes will fix this.

We need an interface for users that are prepared to give up kernel/user 
protection, either because they have no other choice, or because they 
have performance requirements that mandate it. What extra value does 
protecting the BARs against movement add? Nothing.

>
>> We are in the strange situation that the Alex is open to adding an insecure
>> mode to vfio,
> I don't find this strange. It seems to make sense. VFIO is
> already used with DMA capable devices.

It's strange to me because it's charter was for iommu-protected device 
assignment, while uio_pci_generic is for generic pci userspace.


>
>> while you object to a patch which does not change the security
>> of uio_pci_generic in any way; it only makes it more usable at the cost of a
>> tiny increase in the bug surface.
> I don't agree with this either. This depends on the device.
>
>>>    So it's an
>>> understandable argument if you just need something that works, quickly.
>>> But if it's such a stopgap hack, there's no need to insist on it
>>> upstream.
>> It is not more or less a hack than uio_pci_generic allowing DMA,
> It doesn't. sysfs does.
>
>> or
>> /dev/mem, or the module loading interface, or nommu kernels. Security is
>> just one aspect of the kernel, not the only one.
>>
>> It's perfectly reasonable to taint the kernel when insecure DMA is enabled,
>> and to allow the administrator to disable the interface completely.  What I
>> don't understand is why, given that the user allows DMA, we should prevent
>> them from using MSIX in addition.
> There's no need to prevent MSIX with or without DMA.
>
> But UIO uses sysfs for device access. So if we program MSIX we need to
> extend sysfs to protect a ton of registers that are MSIX related from
> the user, and do a bunch of setup and cleanup otherwise kernel will be
> very confused.
>
> It might be surprising to you how many registers are MSIX related,
> but it's true.
>

Userspace can already do all of these things, confusing the kernel. It 
simply doesn't, which is why everything works.  It need only continue 
not to do so for everything to continue to work.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1242396

On Thu, Oct 08, 2015 at 04:20:12PM +0300, Avi Kivity wrote:
> On 10/08/2015 01:26 PM, Michael S. Tsirkin wrote:
> >On Thu, Oct 08, 2015 at 12:19:20PM +0300, Avi Kivity wrote:
> >>
> >>On 10/08/2015 11:32 AM, Michael S. Tsirkin wrote:
> >>>On Thu, Oct 08, 2015 at 08:33:45AM +0300, Avi Kivity wrote:
> >>>>On 08/10/15 00:05, Michael S. Tsirkin wrote:
> >>>>>On Wed, Oct 07, 2015 at 07:39:16PM +0300, Avi Kivity wrote:
> >>>>>>That's what I thought as well, but apparently adding msix support to the
> >>>>>>already insecure uio drivers is even worse.
> >>>>>I'm glad you finally agree what these drivers are doing is insecure.
> >>>>>
> >>>>>And basically kernel cares about security, no one wants to maintain insecure stuff.
> >>>>>
> >>>>>So you guys should think harder whether this code makes any sense upstream.
> >>>>You simply ignore everything I write, cherry-picking the word "insecure" as
> >>>>if it makes your point.  That is very frustrating.
> >>>And I'm sorry about the frustration.  I didn't intend to twist your
> >>>words. It's just that I had to spend literally hours trying to explain
> >>>that security matters in kernel, and all I was getting back was a
> >>>summary "there's no security issue because there are other way to
> >>>corrupt memory".
> >>The word security has several meanings.  The primary meaning is "defense
> >>against a malicious attacker".  In that sense, there is no added value at
> >>all, because the attacker is already root, and can already access all of
> >>kernel and user memory.  Even if the attacker is not root, and just has
> >>access to a non-iommu-protected device, they can still DMA to and from any
> >>memory they like.
> >>
> >>This sense of the word however is irrelevant for this conversation; the user
> >>already gave up on it when they chose to use uio_pci_generic (either because
> >>they have no iommu, or because they need the extra performance).
> >>
> >>Do we agree that security, in the sense of defense against a malicious
> >>attacker, is irrelevant for this conversation?
> >No. uio_pci_generic currently can be used in a secure way in
> >a sense that it's protected againt malicious attacker,
> >assuming you bind it to a device that does not do DMA.
> 
> The context of the conversation is dpdk, which only supports DMA.
> 
> Do we agree that security, in the sense of defense against a malicious
> attacker, is irrelevant for this conversation, taking this under
> consideration?

DPDK has a mode which they call UIO which seems to require people to
disable security.  I agree to that.  It's unfortunate that naming it UIO
gives the whole infrastructure a bad name for security.

But for upstreaming purposes, this doesn't matter:
we don't merge single use interfaces into Linux,
and I do care about interfaces of the code I maintain
being useful in a secure way.

> >
> >>A secondary meaning is protection against inadvertent bugs.  Yes, a faulty
> >>memory write that happens to land in the msix page, can cause a random
> >>memory word to be overwritten.  But so can a faulty memory write into the
> >>rings, or the data structures that support virtual->physical translation,
> >>the data structures that describe the packets before translation, the memory
> >>allocator or pool.  The patch extends the vulnerable surface, but by a
> >>negligible amount.
> >>
> >>>So I was glad when it looked like there's finally an agreement that yes,
> >>>there's value in validating userspace input and yes, it's insecure
> >>>not to do this.
> >>
> >>
> >>>>It is good practice to defend against root oopsing the kernel, but in some
> >>>>cases it cannot be achieved.
> >>>I originally included ways to fix issues that I pointed out, ranging
> >>>from harder to implement with more overhead but more secure to easier to
> >>>implement with less overhead but less secure.  There didn't seem to be
> >>>an understanding that the issues are there at all, so I stopped doing
> >>>that - seemed like a waste of time.
> >>>
> >>>For example, will it kill your performance to reset devices cleanly, on
> >>>open and close,
> >>I don't recall this being mentioned at all.
> >http://mid.gmane.org/20151006005527-mutt-send-email-mst@redhat.com
> 
> Down at the moment for me.

Grep this discussion for "reset", you will find it.

> >But really, this is just off the top of my head.
> >These are all issues VFIO developers encountered
> >and fixed over the years. Go into that code, read it,
> >and you will discover the issues and the solutions.
> 
> vfio is solving a different problem,

It's solving a bunch of problems, including various hardware quirks.

> the problem of security against a
> malicious attacker, one that I'm hoping to agree here that we aren't
> attempting to solve.

You aren't but you should.  uio_pci_generic does do it - though using
legacy interrupts as it does it didn't need to do a lot. But it does
check e.g. interrupt mask support, and doesn't just rely on userspace to
DTRT.

> People have been happily using uio_pci_generic despite all those issues.
> All they were missing was msix support.  You can't use that to force them to
> overhaul that driver, or to add a new subsystem to vfio.

By the way, there's a very simple way to make generic UIO useful on VFs.
Don't enable bus mastering, trigger a timer once a while.


> >
> >>  It seems completely unrelated
> >>to a patch adding msix support to uio_pci_generic.
> >It isn't unrelated. It's because with MSIX patch you are enabling bus
> >mastering in kernel.  So if you start device in a bad state it will
> >corrupt kernel memory.
> 
> You are right, this patch can regress secure users.
> 
> I'd be surprised if there are msix-capable pci devices that do not rely on
> DMA, though.

I would not be surprised at all. The PCI Express specification says:
	MSI/MSI-X interrupt support, which is optional for PCI 3.0 devices, is
	required for PCI Express devices.

> >
> >>>  protect them from writes into MSI config, BAR registers
> >>>and related capablities etc etc?
> >>Obviously the userspace driver has to write to the BAR area.
> >>
> >>If you're talking about the BAR setup registers, yes there is some (tiny)
> >>value in that, but how is it related to this patch?
> >If you don't, moving BARs will move the MSI-X region and
> >protecting it won't help.
> 
> Won't it just become invisible if you do?
> 
> If userspace starts playing with BARs, you lost already, whether msix is
> enabled or not doesn't matter.  It can shadow other BARs, for example.

Of the same device? Sure, but then you only break this device.

At least for PCI Express devices are all behind bridges so
they can't shadow each other BARs.

And for VFs, they don't shadow each other BARs IIRC.


> This is a general weakness of uio_pci_generic, not something exposed by this
> patch.

Patch enables MSIX. Thus the need to protect the MSI-X region. To protect it
you need to make sure it does not move around :)

> >
> >>Protecting the MSI area in the BARs _is_ related to the patch.  I agree it
> >>adds value, if small.
> >>
> >>>   And if not, why are you people wasting
> >>>time arguing about that?
> >>I you want to use your position as maintainer of uio_pci_generic to get
> >>people to overhaul the driver for you with unrelated changes, they will
> >>object.  I can understand a maintainer pointing out the right way to do
> >>something rather than the wrong way.  But piling on a list of unrelated
> >>features as prerequisites is, in my opinion, abuse.
> >I don't see them as unrelated.  Basically you want to turn
> >uio_pci_generic into vfio/pci except without an IOMMU.
> 
> That is not what we want.  Simply adding msix support is sufficient for us.
> Everything else was piled on afterwards.

I'm not stopping you in any way. It's just not the kind of half-baked
interface we should include and support in the upstream kernel, IMHO.

> >You will need a
> >lot of VFIO code then.  That will need a lot of work.  You seem to blame
> >me for this but IMHO that's because patch author has chosen a wrong
> >approach.
> >
> >>Let me repeat that pci_uio_generic is already used for userspace drivers,
> >>with all the issues that you point out, for a long while now. These issues
> >>are not exposed by the requirement to use msix.
> >I answered this already. I don't agree with this.
> 
> With the first sentence or the second?  I'm trying really hard to understand
> the problem.

Enabling msix in kernel exposes additinonal issues.

> >
> >>You are not protecting the
> >>kernel in any way by blocking the patch, you are only protecting people with
> >>iommu-less configurations from using their hardware.
> >Because it's either this patch or nothing at all? I don't believe that.
> >Someone come along and write a better one.
> 
> From your description, I can't imagine what the better patch looks like,
> except as a complete overhaul of uio_pci_generic.

I posted several suggestions over the last several days.
I agree it would be a large change to uio_pci_generic, so
I think a vfio extension would make more sense.

> Our requirement is to enable msix, not to pretend that it is secure while it
> allows DMA to any piece of memory in the system.
> 
> >
> >>>   The only thing I heard is that it's a hassle.
> >>>That's true (though if you follow my advice and try to share code with
> >>>vfio/pci you get a lot of this logic for free).
> >>My thinking was that vfio was for secure (in the "defense against malicious
> >>attackers" sense) while uio_pci_generic was, de-facto at least, for use by
> >>trusted users.
> >And some are using it in very broken ways. Yes. But now you want
> >to fix this in stone by tying a kernel/userspace interface
> >to their broken ways. I think that would be a mistake.
> 
> At heart, the brokenness here is that you allow insecure DMA. No amount of
> changes will fix this.
>
> We need an interface for users that are prepared to give up kernel/user
> protection, either because they have no other choice, or because they have
> performance requirements that mandate it. What extra value does protecting
> the BARs against movement add? Nothing.

Sure. But I am not talking about this usecase. It's an unsupportable
mess, it does not matter for upstream API discussion.  Yea, performance.
But where would you stop? Will you next ask me to merge code that
accesses userspace pointers without checking, because performance? I can
easily see DPDK finding a way to make device DMA into
current_thread_info()->flags to wake a CPU that does monitor on it,
instead of an interrupt, because performance.  Voila, we now have to
keep struct task layout stable because it's part of userspace ABI.  And
so on.

So yes, there's userspace doing crazy things, and we don't want
to break it, but we need to consider the needs of a non-crazy
userspace when we build APIs.


> >
> >>We are in the strange situation that the Alex is open to adding an insecure
> >>mode to vfio,
> >I don't find this strange. It seems to make sense. VFIO is
> >already used with DMA capable devices.
> 
> It's strange to me because it's charter was for iommu-protected device
> assignment, while uio_pci_generic is for generic pci userspace.

I don't know where is the VFIO charter, but you can find the UIO charter
under Documentation. DPDK is not using it as designed.

> 
> >
> >>while you object to a patch which does not change the security
> >>of uio_pci_generic in any way; it only makes it more usable at the cost of a
> >>tiny increase in the bug surface.
> >I don't agree with this either. This depends on the device.
> >
> >>>   So it's an
> >>>understandable argument if you just need something that works, quickly.
> >>>But if it's such a stopgap hack, there's no need to insist on it
> >>>upstream.
> >>It is not more or less a hack than uio_pci_generic allowing DMA,
> >It doesn't. sysfs does.
> >
> >>or
> >>/dev/mem, or the module loading interface, or nommu kernels. Security is
> >>just one aspect of the kernel, not the only one.
> >>
> >>It's perfectly reasonable to taint the kernel when insecure DMA is enabled,
> >>and to allow the administrator to disable the interface completely.  What I
> >>don't understand is why, given that the user allows DMA, we should prevent
> >>them from using MSIX in addition.
> >There's no need to prevent MSIX with or without DMA.
> >
> >But UIO uses sysfs for device access. So if we program MSIX we need to
> >extend sysfs to protect a ton of registers that are MSIX related from
> >the user, and do a bunch of setup and cleanup otherwise kernel will be
> >very confused.
> >
> >It might be surprising to you how many registers are MSIX related,
> >but it's true.
> >
> 
> Userspace can already do all of these things, confusing the kernel. It
> simply doesn't, which is why everything works.  It need only continue not to
> do so for everything to continue to work.

You make it sound as if you are enabling existing APIs for new hardware.
That's not what these patches do, it's a new API you are building,
and new drivers will have to be written to use it. And the API
as defined here has subtle gotchas.  VFIO just solved most of them
already.

Instead of all this, I have a simple suggestion.
UIO spec says:

        For cards that don't generate interrupts but need to be
        polled, there is the possibility to set up a timer that
        triggers the interrupt handler at configurable time intervals.

add code to set this up in uio_pci_generic if there's no interrupt, and
wake userspace.  This will use existing code.  This won't help the new
DPDK interrupt mode (from June 2015), but it helps use it on existing
systems with no new interfaces.

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

[toc] | [prev] | [next] | [standalone]

Page 2 of 3 — ← Prev page 1 [2] 3  Next page →

Back to top | Article view | linux.kernel

csiph-web