Re: [libvirt] [PATCH] virt-aa-helper: generate rules for gl enabled graphics devices
On Thu, Jan 17, 2019 at 11:05 AM Erik Skultety <eskultet(a)redhat.com> wrote:
On Tue, Jan 15, 2019 at 06:34:41PM +0200, Christian Ehrhardt wrote:
> This adds the virt-aa-helper support for gl enabled graphics devices to
> generate rules for the needed rendernode paths.
>
> Example in domain xml:
> <graphics type='spice'>
> <gl enable='yes' rendernode='/dev/dri/bar'/>
> </graphics>
>
> results in:
> "/dev/dri" r,
> "/dev/dri/bar" rw,
>
> Special cases are:
> - multiple devices with rendernodes -> all are added
> - non explicit rendernodes -> follow recently added virHostGetDRMRenderNode
> - rendernode without opengl (in egl-headless for example) -> still add
> the node
>
> Fixes: https://bugs.launchpad.net/ubuntu/+source/libvirt/+bug/1757085
>
> Signed-off-by: Christian Ehrhardt <christian.ehrhardt(a)canonical.com>
> ---
> src/security/virt-aa-helper.c | 20 +++++++++++++++++++-
> 1 file changed, 19 insertions(+), 1 deletion(-)
>
> diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c
> index 64a425671d..327a8a0202 100644
> --- a/src/security/virt-aa-helper.c
> +++ b/src/security/virt-aa-helper.c
> @@ -938,7 +938,7 @@ get_files(vahControl * ctl)
> size_t i;
> char *uuid;
> char uuidstr[VIR_UUID_STRING_BUFLEN];
> - bool needsVfio = false, needsvhost = false;
> + bool needsVfio = false, needsvhost = false, needDefaultRenderNode = false;
>
> /* verify uuid is same as what we were given on the command line */
> virUUIDFormat(ctl->def->uuid, uuidstr);
> @@ -1062,6 +1062,10 @@ get_files(vahControl * ctl)
> for (i = 0; i < ctl->def->ngraphics; i++) {
> virDomainGraphicsDefPtr graphics = ctl->def->graphics[i];
> size_t n;
> + const char *rendernode = virDomainGraphicsGetRenderNode(graphics);
> +
> + if (rendernode)
> + vah_add_file(&buf, rendernode, "rw");
>
> for (n = 0; n < graphics->nListens; n++) {
> virDomainGraphicsListenDef listenObj = graphics->listens[n];
> @@ -1071,6 +1075,20 @@ get_files(vahControl * ctl)
> vah_add_file(&buf, listenObj.socket, "rw"))
> goto cleanup;
> }
> +
> + if (graphics->data.spice.gl == VIR_TRISTATE_BOOL_YES) {
> + if (!rendernode)
> + needDefaultRenderNode = true;
> + }
^This feels a bit detached from the hunk above.
Agreed, better grouped in an upcoming V2
Also, what about EGL_HEADLESS,
you will want to add (potentially a default one) rendernode for egl_headless
too.
Yes I'd want EGL Headless as well, using the suggested
virDomainGraphicsNeedsAutoRenderNode to cover both.
As it does the same checks, but better - thanks for the hint!
You could also optimize this with
virDomainGraphicsNeedsAutoRenderNode and cover both graphics types.
> + }
> +
> + if (virDomainGraphicsDefHasOpenGL(ctl->def))
> + vah_add_file(&buf, "/dev/dri", "r");
Why is ^this needed?
I started on this before the virDomainGraphics helpers and before you
changed it to always pass a defined value.
Back then qemu then would have enumerated /dev/dri, but you are right
we - no more - need the base dir since we pass an explicit value.
In case I later on find cases this still triggers I can come back with
a small patch.
> +
> + if (needDefaultRenderNode) {
> + const char *rendernode = virHostGetDRMRenderNode();
> + if (rendernode)
> + vah_add_file(&buf, virHostGetDRMRenderNode(), "rw");
IUC the virDomainGraphicsNeedsAutoRenderNode part would come ^here.
Yeah, since we already iterate on "graphics" elements above and there
is no drawback in adding the default virHostGetDRMRenderNode path
twice (would only happen in awkward guest XMLs anyway) this can be
simplified even further.
it essentially becomes:
"if it has explicit node add it, otherwise if it needs a rendernode at
all add the default"
I also agree with Michal's suggestion.
Erik
--
Christian Ehrhardt
Software Engineer, Ubuntu Server
Canonical Ltd