[libvirt] [PATCH] qemu: Label all TAP FDs

[Martin Kletzander]

On Wed, Aug 20, 2014 at 09:58:53AM +0200, Michal Privoznik wrote:
>On 20.08.2014 05:49, Martin Kletzander wrote:
>> On Tue, Aug 19, 2014 at 03:18:02PM +0200, Michal Privoznik wrote:
>>> https://bugzilla.redhat.com/show_bug.cgi?id=1095636
>>>
>>> When starting up the domain the domain's NICs are allocated. As of
>>> 1f24f682 (v1.0.6) we are able to use multiqueue feature on virtio
>>> NICs. It breaks network processing into multiple queues which can be
>>> processed in parallel by different host CPUs. The queues are, however,
>>> created by opening /dev/net/tun several times. Unfortunately, only the
>>> first FD in the row is labelled so when turning the multiqueue feature
>>> on in the guest, qemu will get AVC denial. Make sure we label all the
>>> FDs needed.
>>>
>>> Moreover, the default label of /dev/net/tun doesn't allow
>>> attaching a queue:
>>>
>>>    type=AVC msg=audit(1399622478.790:893): avc:  denied  { attach_queue }
>>>    for  pid=7585 comm="qemu-kvm"
>>>    scontext=system_u:system_r:svirt_t:s0:c638,c877
>>>    tcontext=system_u:system_r:virtd_t:s0-s0:c0.c1023
>>
>> Wow, I didn't even know you could have range in the "level" field (I
>> think it doesn't make sense, but I don't understand why it is allowed
>> by SELinux).
>>
>>>    tclass=tun_socket
>>>
>>> And as suggested by SELinux maintainers, the tun FD should be labeled

Oh, it's probably too late for s/labeled/labelled/, right? :-/

[...]
>>> +    for (i = 0; i < tapfdSize; i++) {
>>> +        if (virSecurityManagerSetTapFDLabel(driver->securityManager,
>>> +                                            vm->def, tapfd[i]) < 0)
>>> +            goto cleanup;
>>> +    }
>>> +
>>
>> Shouldn't there be the same loop for vhostfd[i]?  Although it won't
>> probably be > 1.  Just ckecking.
>
>No, vhost FDs are not connected queue to. Frankly, I don't fully
>understand all the details, but hey - it works :-)
>

I didn't mean that WRT the multiqueue problem, I just wondered if
vhostfd (if passed to qemu as well) cannot have similar problem (only
vhostFDs[0] labelled) in the future (which we could fix before it
appears).

[...]
>>
>> This looks much better now.  Although it looks way too similar to
>> virSecuritySELinuxSetImageFDLabel() :)  I think it might be worth
>> keeping just one of these two functions, let's say ...SetFDLabel() in
>> order not to complicate things, and assign it to both needed fields in
>> the domain structure.
>
>Yes and no. While SetTapFDLabel needs to set domain process label
>(seclabel->label) SetImageFDLabel needs to use image label
>(seclabel->imagelabel). And I don't think it's worth wrap these two

Yep, you're right.

>functions into one wrapper function (I doubt it'll be readable more too).
>

We could argue about the readability, but it doesn't make sense now
when they are different.

>>
>> ACK with that changed.
>
>So I'm taking this as ACK without any change required and pushing. Thanks!
>

If you can fix the commit message, please do, otherwise the ACK stands
as guessed.

Martin
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20140820/4880cf40/attachment-0001.sig>