[libvirt] [PATCH] qemu: Allow /dev/dri/renderD128

Michal Privoznik posted 1 patch 7 years, 1 month ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/48a5971b0e8d2f787d21014bb9f3e9738832b6f6.1486545986.git.mprivozn@redhat.com
docs/drvqemu.html.in               | 1 +
src/qemu/qemu.conf                 | 3 ++-
src/qemu/qemu_cgroup.c             | 1 +
src/qemu/test_libvirtd_qemu.aug.in | 1 +
4 files changed, 5 insertions(+), 1 deletion(-)
[libvirt] [PATCH] qemu: Allow /dev/dri/renderD128
Posted by Michal Privoznik 7 years, 1 month ago
This demand comes from qemu_egl_rendernode_open() in qemu source
code. It is needed for virgl to work with qemu:///system
connection. The session one works just fine.

Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
---
 docs/drvqemu.html.in               | 1 +
 src/qemu/qemu.conf                 | 3 ++-
 src/qemu/qemu_cgroup.c             | 1 +
 src/qemu/test_libvirtd_qemu.aug.in | 1 +
 4 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/docs/drvqemu.html.in b/docs/drvqemu.html.in
index fa1eca78a..c7c34c9f2 100644
--- a/docs/drvqemu.html.in
+++ b/docs/drvqemu.html.in
@@ -397,6 +397,7 @@ chmod o+x /path/to/directory
 /dev/random, /dev/urandom,
 /dev/ptmx, /dev/kvm, /dev/kqemu,
 /dev/rtc, /dev/hpet, /dev/net/tun
+/dev/dri/renderD128
 </pre>
 
     <p>
diff --git a/src/qemu/qemu.conf b/src/qemu/qemu.conf
index a8cd369cb..087e6bcce 100644
--- a/src/qemu/qemu.conf
+++ b/src/qemu/qemu.conf
@@ -354,7 +354,8 @@
 #    "/dev/null", "/dev/full", "/dev/zero",
 #    "/dev/random", "/dev/urandom",
 #    "/dev/ptmx", "/dev/kvm", "/dev/kqemu",
-#    "/dev/rtc","/dev/hpet", "/dev/vfio/vfio"
+#    "/dev/rtc","/dev/hpet", "/dev/vfio/vfio",
+#    "/dev/dri/renderD128"
 #]
 #
 # RDMA migration requires the following extra files to be added to the list:
diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c
index 6c90d46d1..b47f714fc 100644
--- a/src/qemu/qemu_cgroup.c
+++ b/src/qemu/qemu_cgroup.c
@@ -47,6 +47,7 @@ const char *const defaultDeviceACL[] = {
     "/dev/random", "/dev/urandom",
     "/dev/ptmx", "/dev/kvm", "/dev/kqemu",
     "/dev/rtc", "/dev/hpet", "/dev/vfio/vfio",
+    "/dev/dri/renderD128",
     NULL,
 };
 #define DEVICE_PTY_MAJOR 136
diff --git a/src/qemu/test_libvirtd_qemu.aug.in b/src/qemu/test_libvirtd_qemu.aug.in
index a749f0900..b7067f7b7 100644
--- a/src/qemu/test_libvirtd_qemu.aug.in
+++ b/src/qemu/test_libvirtd_qemu.aug.in
@@ -56,6 +56,7 @@ module Test_libvirtd_qemu =
     { "9" = "/dev/rtc" }
     { "10" = "/dev/hpet" }
     { "11" = "/dev/vfio/vfio" }
+    { "12" = "/dev/dri/renderD128" }
 }
 { "save_image_format" = "raw" }
 { "dump_image_format" = "raw" }
-- 
2.11.0

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu: Allow /dev/dri/renderD128
Posted by Daniel P. Berrange 7 years, 1 month ago
On Wed, Feb 08, 2017 at 10:26:26AM +0100, Michal Privoznik wrote:
> This demand comes from qemu_egl_rendernode_open() in qemu source
> code. It is needed for virgl to work with qemu:///system
> connection. The session one works just fine.
> 
> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
> ---
>  docs/drvqemu.html.in               | 1 +
>  src/qemu/qemu.conf                 | 3 ++-
>  src/qemu/qemu_cgroup.c             | 1 +
>  src/qemu/test_libvirtd_qemu.aug.in | 1 +
>  4 files changed, 5 insertions(+), 1 deletion(-)

> diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c
> index 6c90d46d1..b47f714fc 100644
> --- a/src/qemu/qemu_cgroup.c
> +++ b/src/qemu/qemu_cgroup.c
> @@ -47,6 +47,7 @@ const char *const defaultDeviceACL[] = {
>      "/dev/random", "/dev/urandom",
>      "/dev/ptmx", "/dev/kvm", "/dev/kqemu",
>      "/dev/rtc", "/dev/hpet", "/dev/vfio/vfio",
> +    "/dev/dri/renderD128",

Surely this is only needed in very specific scenarios. ie with
the virtio-vga 3d rendering enabled.

Allowing unconditional access to the DRI devices is a big
wide open door from security POV, for something few VMs
will ever need.

The global device whitelist is only for devices that we
expect every QEMU to unconditionally require.


Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://entangle-photo.org       -o-    http://search.cpan.org/~danberr/ :|

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu: Allow /dev/dri/renderD128
Posted by Michal Privoznik 7 years, 1 month ago
On 02/08/2017 10:31 AM, Daniel P. Berrange wrote:
> On Wed, Feb 08, 2017 at 10:26:26AM +0100, Michal Privoznik wrote:
>> This demand comes from qemu_egl_rendernode_open() in qemu source
>> code. It is needed for virgl to work with qemu:///system
>> connection. The session one works just fine.
>>
>> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
>> ---
>>  docs/drvqemu.html.in               | 1 +
>>  src/qemu/qemu.conf                 | 3 ++-
>>  src/qemu/qemu_cgroup.c             | 1 +
>>  src/qemu/test_libvirtd_qemu.aug.in | 1 +
>>  4 files changed, 5 insertions(+), 1 deletion(-)
> 
>> diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c
>> index 6c90d46d1..b47f714fc 100644
>> --- a/src/qemu/qemu_cgroup.c
>> +++ b/src/qemu/qemu_cgroup.c
>> @@ -47,6 +47,7 @@ const char *const defaultDeviceACL[] = {
>>      "/dev/random", "/dev/urandom",
>>      "/dev/ptmx", "/dev/kvm", "/dev/kqemu",
>>      "/dev/rtc", "/dev/hpet", "/dev/vfio/vfio",
>> +    "/dev/dri/renderD128",
> 
> Surely this is only needed in very specific scenarios. ie with
> the virtio-vga 3d rendering enabled.
> 
> Allowing unconditional access to the DRI devices is a big
> wide open door from security POV, for something few VMs
> will ever need.
> 
> The global device whitelist is only for devices that we
> expect every QEMU to unconditionally require.

I can argue the same about /dev/vfio/vfio and yet we have it on the list.

Michal

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu: Allow /dev/dri/renderD128
Posted by Daniel P. Berrange 7 years, 1 month ago
On Wed, Feb 08, 2017 at 10:44:53AM +0100, Michal Privoznik wrote:
> On 02/08/2017 10:31 AM, Daniel P. Berrange wrote:
> > On Wed, Feb 08, 2017 at 10:26:26AM +0100, Michal Privoznik wrote:
> >> This demand comes from qemu_egl_rendernode_open() in qemu source
> >> code. It is needed for virgl to work with qemu:///system
> >> connection. The session one works just fine.
> >>
> >> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
> >> ---
> >>  docs/drvqemu.html.in               | 1 +
> >>  src/qemu/qemu.conf                 | 3 ++-
> >>  src/qemu/qemu_cgroup.c             | 1 +
> >>  src/qemu/test_libvirtd_qemu.aug.in | 1 +
> >>  4 files changed, 5 insertions(+), 1 deletion(-)
> > 
> >> diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c
> >> index 6c90d46d1..b47f714fc 100644
> >> --- a/src/qemu/qemu_cgroup.c
> >> +++ b/src/qemu/qemu_cgroup.c
> >> @@ -47,6 +47,7 @@ const char *const defaultDeviceACL[] = {
> >>      "/dev/random", "/dev/urandom",
> >>      "/dev/ptmx", "/dev/kvm", "/dev/kqemu",
> >>      "/dev/rtc", "/dev/hpet", "/dev/vfio/vfio",
> >> +    "/dev/dri/renderD128",
> > 
> > Surely this is only needed in very specific scenarios. ie with
> > the virtio-vga 3d rendering enabled.
> > 
> > Allowing unconditional access to the DRI devices is a big
> > wide open door from security POV, for something few VMs
> > will ever need.
> > 
> > The global device whitelist is only for devices that we
> > expect every QEMU to unconditionally require.
> 
> I can argue the same about /dev/vfio/vfio and yet we have it on the list.

I consider that to be a bug we should fix too. It should only ever have
been added if the guest is actually using vfio.

Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://entangle-photo.org       -o-    http://search.cpan.org/~danberr/ :|

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu: Allow /dev/dri/renderD128
Posted by Michal Privoznik 7 years, 1 month ago
On 02/08/2017 11:16 AM, Daniel P. Berrange wrote:
> On Wed, Feb 08, 2017 at 10:44:53AM +0100, Michal Privoznik wrote:
>> On 02/08/2017 10:31 AM, Daniel P. Berrange wrote:
>>> On Wed, Feb 08, 2017 at 10:26:26AM +0100, Michal Privoznik wrote:
>>>> This demand comes from qemu_egl_rendernode_open() in qemu source
>>>> code. It is needed for virgl to work with qemu:///system
>>>> connection. The session one works just fine.
>>>>
>>>> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
>>>> ---
>>>>  docs/drvqemu.html.in               | 1 +
>>>>  src/qemu/qemu.conf                 | 3 ++-
>>>>  src/qemu/qemu_cgroup.c             | 1 +
>>>>  src/qemu/test_libvirtd_qemu.aug.in | 1 +
>>>>  4 files changed, 5 insertions(+), 1 deletion(-)
>>>
>>>> diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c
>>>> index 6c90d46d1..b47f714fc 100644
>>>> --- a/src/qemu/qemu_cgroup.c
>>>> +++ b/src/qemu/qemu_cgroup.c
>>>> @@ -47,6 +47,7 @@ const char *const defaultDeviceACL[] = {
>>>>      "/dev/random", "/dev/urandom",
>>>>      "/dev/ptmx", "/dev/kvm", "/dev/kqemu",
>>>>      "/dev/rtc", "/dev/hpet", "/dev/vfio/vfio",
>>>> +    "/dev/dri/renderD128",
>>>
>>> Surely this is only needed in very specific scenarios. ie with
>>> the virtio-vga 3d rendering enabled.
>>>
>>> Allowing unconditional access to the DRI devices is a big
>>> wide open door from security POV, for something few VMs
>>> will ever need.
>>>
>>> The global device whitelist is only for devices that we
>>> expect every QEMU to unconditionally require.
>>
>> I can argue the same about /dev/vfio/vfio and yet we have it on the list.
> 
> I consider that to be a bug we should fix too. It should only ever have
> been added if the guest is actually using vfio.

Agreed. So just to be clear on the design, if we automatically enable
/dev/vfio/vfio in devices cgroup on domain startup (of course only for
domains that are doing pci passtrhough^Wassignment), you would be okay
with that?
Similarly, allowing /dev/dri/renderD128 for domains with virgl on their
startup.

Michal

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu: Allow /dev/dri/renderD128
Posted by Daniel P. Berrange 7 years, 1 month ago
On Wed, Feb 08, 2017 at 11:28:33AM +0100, Michal Privoznik wrote:
> On 02/08/2017 11:16 AM, Daniel P. Berrange wrote:
> > On Wed, Feb 08, 2017 at 10:44:53AM +0100, Michal Privoznik wrote:
> >> On 02/08/2017 10:31 AM, Daniel P. Berrange wrote:
> >>> On Wed, Feb 08, 2017 at 10:26:26AM +0100, Michal Privoznik wrote:
> >>>> This demand comes from qemu_egl_rendernode_open() in qemu source
> >>>> code. It is needed for virgl to work with qemu:///system
> >>>> connection. The session one works just fine.
> >>>>
> >>>> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
> >>>> ---
> >>>>  docs/drvqemu.html.in               | 1 +
> >>>>  src/qemu/qemu.conf                 | 3 ++-
> >>>>  src/qemu/qemu_cgroup.c             | 1 +
> >>>>  src/qemu/test_libvirtd_qemu.aug.in | 1 +
> >>>>  4 files changed, 5 insertions(+), 1 deletion(-)
> >>>
> >>>> diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c
> >>>> index 6c90d46d1..b47f714fc 100644
> >>>> --- a/src/qemu/qemu_cgroup.c
> >>>> +++ b/src/qemu/qemu_cgroup.c
> >>>> @@ -47,6 +47,7 @@ const char *const defaultDeviceACL[] = {
> >>>>      "/dev/random", "/dev/urandom",
> >>>>      "/dev/ptmx", "/dev/kvm", "/dev/kqemu",
> >>>>      "/dev/rtc", "/dev/hpet", "/dev/vfio/vfio",
> >>>> +    "/dev/dri/renderD128",
> >>>
> >>> Surely this is only needed in very specific scenarios. ie with
> >>> the virtio-vga 3d rendering enabled.
> >>>
> >>> Allowing unconditional access to the DRI devices is a big
> >>> wide open door from security POV, for something few VMs
> >>> will ever need.
> >>>
> >>> The global device whitelist is only for devices that we
> >>> expect every QEMU to unconditionally require.
> >>
> >> I can argue the same about /dev/vfio/vfio and yet we have it on the list.
> > 
> > I consider that to be a bug we should fix too. It should only ever have
> > been added if the guest is actually using vfio.
> 
> Agreed. So just to be clear on the design, if we automatically enable
> /dev/vfio/vfio in devices cgroup on domain startup (of course only for
> domains that are doing pci passtrhough^Wassignment), you would be okay
> with that?

We'd need to actually check the PCI device config to see if it
is set to use vfio or the legacy impl or the default, so slightly
more complex, but yes to the general principle.

Also might need updating on hotplug/unplug.

> Similarly, allowing /dev/dri/renderD128 for domains with virgl on their
> startup.

Yep

Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://entangle-photo.org       -o-    http://search.cpan.org/~danberr/ :|

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu: Allow /dev/dri/renderD128
Posted by Cole Robinson 7 years, 1 month ago
On 02/08/2017 05:28 AM, Michal Privoznik wrote:
> On 02/08/2017 11:16 AM, Daniel P. Berrange wrote:
>> On Wed, Feb 08, 2017 at 10:44:53AM +0100, Michal Privoznik wrote:
>>> On 02/08/2017 10:31 AM, Daniel P. Berrange wrote:
>>>> On Wed, Feb 08, 2017 at 10:26:26AM +0100, Michal Privoznik wrote:
>>>>> This demand comes from qemu_egl_rendernode_open() in qemu source
>>>>> code. It is needed for virgl to work with qemu:///system
>>>>> connection. The session one works just fine.
>>>>>
>>>>> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
>>>>> ---
>>>>>  docs/drvqemu.html.in               | 1 +
>>>>>  src/qemu/qemu.conf                 | 3 ++-
>>>>>  src/qemu/qemu_cgroup.c             | 1 +
>>>>>  src/qemu/test_libvirtd_qemu.aug.in | 1 +
>>>>>  4 files changed, 5 insertions(+), 1 deletion(-)
>>>>
>>>>> diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c
>>>>> index 6c90d46d1..b47f714fc 100644
>>>>> --- a/src/qemu/qemu_cgroup.c
>>>>> +++ b/src/qemu/qemu_cgroup.c
>>>>> @@ -47,6 +47,7 @@ const char *const defaultDeviceACL[] = {
>>>>>      "/dev/random", "/dev/urandom",
>>>>>      "/dev/ptmx", "/dev/kvm", "/dev/kqemu",
>>>>>      "/dev/rtc", "/dev/hpet", "/dev/vfio/vfio",
>>>>> +    "/dev/dri/renderD128",
>>>>
>>>> Surely this is only needed in very specific scenarios. ie with
>>>> the virtio-vga 3d rendering enabled.
>>>>
>>>> Allowing unconditional access to the DRI devices is a big
>>>> wide open door from security POV, for something few VMs
>>>> will ever need.
>>>>
>>>> The global device whitelist is only for devices that we
>>>> expect every QEMU to unconditionally require.
>>>
>>> I can argue the same about /dev/vfio/vfio and yet we have it on the list.
>>
>> I consider that to be a bug we should fix too. It should only ever have
>> been added if the guest is actually using vfio.
> 
> Agreed. So just to be clear on the design, if we automatically enable
> /dev/vfio/vfio in devices cgroup on domain startup (of course only for
> domains that are doing pci passtrhough^Wassignment), you would be okay
> with that?
> Similarly, allowing /dev/dri/renderD128 for domains with virgl on their
> startup.
> 

Note, check jtomko's earlier patch which aims to do that for /dev/dri/renderD128

- Cole

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list