[libvirt] [PATCH] virt-aa-helper: generate rules for gl enabled graphics devices

[Christian Ehrhardt]

On Wed, Jan 16, 2019 at 10:37 AM Michal Privoznik <mprivozn at redhat.com> wrote:
>
> On 1/15/19 5:34 PM, 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 at 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;
> > +        }
> > +    }
> > +
> > +    if (virDomainGraphicsDefHasOpenGL(ctl->def))
> > +        vah_add_file(&buf, "/dev/dri", "r");
> > +
> > +    if (needDefaultRenderNode) {
> > +        const char *rendernode = virHostGetDRMRenderNode();
> > +        if (rendernode)
> > +            vah_add_file(&buf, virHostGetDRMRenderNode(), "rw");
>
> This leaks @rendernode and also looks a bit strange to call the function
> twice.

Yes you are totally right, thanks for the suggestion.

>
> ACK with this squashed in:
>
> diff --git i/src/security/virt-aa-helper.c w/src/security/virt-aa-helper.c
> index 327a8a0202..60fea9b174 100644
> --- i/src/security/virt-aa-helper.c
> +++ w/src/security/virt-aa-helper.c
> @@ -1086,9 +1086,11 @@ get_files(vahControl * ctl)
>          vah_add_file(&buf, "/dev/dri", "r");
>
>      if (needDefaultRenderNode) {
> -        const char *rendernode = virHostGetDRMRenderNode();
> -        if (rendernode)
> -            vah_add_file(&buf, virHostGetDRMRenderNode(), "rw");
> +        char *rendernode = virHostGetDRMRenderNode();
> +        if (rendernode) {
> +            vah_add_file(&buf, rendernode, "rw");
> +            VIR_FREE(rendernode);
> +        }
>      }
>
>      if (ctl->def->ngraphics == 1 &&
>
>
> I believe you have commit access, right? If not let me know and I will
> push it.

I have access, but let me create a V2 with Eriks comment addressed first.

>
> Michal



-- 
Christian Ehrhardt
Software Engineer, Ubuntu Server
Canonical Ltd