On Tue, Mar 17, 2026 at 09:06:55 -0500, Wesley Hershberger via Devel wrote:
Introduce a read-only `tapfd` element for direct interfaces (macvtap), which contains the path to the backing tapfd for that interface (e.g. `/dev/tapXX`).
The element is only included when the domain is being formatted for internal consumption (VIR_DOMAIN_DEF_FORMAT_STATUS) and is not accepted in user-provided XML (!VIR_DOMAIN_DEF_PARSE_INACTIVE).
This is used by the AppArmor security driver when re-generating profiles.
Partial-Resolves: https://gitlab.com/libvirt/libvirt/-/issues/692 Bug-Ubuntu: https://bugs.launchpad.net/bugs/2126574 Signed-off-by: Wesley Hershberger <wesley.hershberger@canonical.com> --- Resending this patch as I've not recieved a response on my previous submission. Fixed the bug URL in the commit message as I missed that feedback item on my last mail.
This submission is a partial revision of a previous series with a fix for the macvtap component of gitlab#692 [1][2]. I haven't had bandwidth to resolve the blockcommit component since the complexity there is somewhat higher (and is also lower priority for us).
I kept the separate `tapfd` element rather than reusing the existing `backend` element (virDomainNetBackend.tap) to avoid making a user-visible change [3]. I'd be happy to use the existing field instead if you think that would make more sense.
I opted not to introduce/modify a security driver API for FD+path as the patch here is sufficient to resolve the bug, but would be willing to do so if that would make this change more palatable.
I've opened a MR to libvirt-tck with test cases that demonstrate the bugs that this fixes [4]. apparmor/110-macvtap.t passes with this patch applied.
Thanks for the reviews and continued consideration. ~Wesley
[1] https://lists.libvirt.org/archives/list/devel@lists.libvirt.org/thread/UNNBQ... [2] https://gitlab.com/libvirt/libvirt/-/issues/692 [3] https://libvirt.org/formatdomain.html#setting-network-backend-specific-optio... [4] https://gitlab.com/libvirt/libvirt-tck/-/merge_requests/73 --- Changes in v3: - Fix buglink in commit message - Link to v2: https://lists.libvirt.org/archives/list/devel@lists.libvirt.org/thread/IPEBL...
Changes in v2: - Drop `virt-aa-helper: Ask for no deny rule...` as it was applied - Drop `qemu: Store blockcommit permissions...` due to unresolved concerns - Pass tapfd path through netdef instead of resolving from fd - Link to v1: https://lists.libvirt.org/archives/list/devel@lists.libvirt.org/thread/UNNBQ... --- src/conf/domain_conf.c | 8 ++++++++ src/conf/domain_conf.h | 1 + src/hypervisor/domain_interface.c | 2 +- src/lxc/lxc_process.c | 1 + src/qemu/qemu_interface.c | 1 + src/security/security_apparmor.c | 1 + src/security/virt-aa-helper.c | 5 +++++ src/util/virnetdevmacvlan.c | 18 +++++++++++------- src/util/virnetdevmacvlan.h | 4 +++-
This patch does a bit too much in one patch. It'll need to be split to the following parts: - plumbing of the tapfd into status XML - feeding apparmor the status XML - actual apparmor change to use the tapfd
9 files changed, 32 insertions(+), 9 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 03a05366e1..9714a1e141 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -2971,6 +2971,7 @@ virDomainNetDefFree(virDomainNetDef *def) g_free(def->virtio); g_free(def->coalesce); g_free(def->sourceDev); + g_free(def->tapfdpath);
virNetDevIPInfoClear(&def->guestIP); virNetDevIPInfoClear(&def->hostIP); @@ -10634,6 +10635,10 @@ virDomainNetDefParseXML(virDomainXMLOption *xmlopt, return NULL; }
+ if (!(flags & VIR_DOMAIN_DEF_PARSE_INACTIVE)) { + def->tapfdpath = virXPathString("string(./tapfd/@path)", ctxt); + } + if (virNetworkPortOptionsParseXML(ctxt, &def->isolatedPort) < 0) return NULL;
@@ -26004,6 +26009,9 @@ virDomainNetDefFormat(virBuffer *buf, if (def->mtu) virBufferAsprintf(buf, "<mtu size='%u'/>\n", def->mtu);
+ if (def->tapfdpath && (flags & VIR_DOMAIN_DEF_FORMAT_STATUS)) + virBufferAsprintf(buf, "<tapfd path='%s'/>\n", def->tapfdpath);
use virBufferEscapeString to ensure the path is formatted with XML entities escaped Since it's an separate element, ideally add an example of the above to one of the test cases in tests/qemustatusxml2xmldata/. It'll prevent potential gotcha if anyone ever added such element. modern-in.xml/modern-out.xml is the catch-all for random newly added stuff without specific purpose.
+ virDomainNetDefCoalesceFormatXML(buf, def->coalesce);
virDomainDeviceInfoFormat(buf, &def->info, flags | VIR_DOMAIN_DEF_FORMAT_ALLOW_BOOT
diff --git a/src/security/security_apparmor.c b/src/security/security_apparmor.c index 40f13ec1a5..3ff80e1dc9 100644 --- a/src/security/security_apparmor.c +++ b/src/security/security_apparmor.c @@ -156,6 +156,7 @@ load_profile(virSecurityManager *mgr G_GNUC_UNUSED,
if (virDomainDefFormatInternal(def, NULL, &buf, VIR_DOMAIN_DEF_FORMAT_SECURE | + VIR_DOMAIN_DEF_FORMAT_STATUS | VIR_DOMAIN_DEF_FORMAT_VOLUME_TRANSLATED) < 0) return -1;
diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c index e932e79dab..60e03c2ce8 100644 --- a/src/security/virt-aa-helper.c +++ b/src/security/virt-aa-helper.c @@ -1192,6 +1192,11 @@ get_files(vahControl * ctl) vhu->type) != 0) return -1; } + + if (net->tapfdpath) { + if (vah_add_file(&buf, net->tapfdpath, "rwk") != 0) + return -1; + } }
for (i = 0; i < ctl->def->nmems; i++) {
The two hunks above belong to one or two separate patches based on how you'd want to justify it in the commit messages.
diff --git a/src/util/virnetdevmacvlan.c b/src/util/virnetdevmacvlan.c index cde9d70eef..fcf63e08ff 100644 --- a/src/util/virnetdevmacvlan.c +++ b/src/util/virnetdevmacvlan.c @@ -152,24 +152,24 @@ int virNetDevMacVLanDelete(const char *ifname) int virNetDevMacVLanTapOpen(const char *ifname, int *tapfd, - size_t tapfdSize) + size_t tapfdSize, + char **tapname) { int retries = 10; int ret = -1; int ifindex; size_t i = 0; - g_autofree char *tapname = NULL;
if (virNetDevGetIndex(ifname, &ifindex) < 0) return -1;
- tapname = g_strdup_printf("/dev/tap%d", ifindex); + *tapname = g_strdup_printf("/dev/tap%d", ifindex);
for (i = 0; i < tapfdSize; i++) { int fd = -1;
while (fd < 0) { - if ((fd = open(tapname, O_RDWR)) >= 0) { + if ((fd = open(*tapname, O_RDWR)) >= 0) { tapfd[i] = fd; } else if (retries-- > 0) { /* may need to wait for udev to be done */ @@ -178,7 +178,7 @@ virNetDevMacVLanTapOpen(const char *ifname, /* However, if haven't succeeded, quit. */ virReportSystemError(errno, _("cannot open macvtap tap device %1$s"), - tapname); + *tapname); goto cleanup; } } @@ -188,6 +188,7 @@ virNetDevMacVLanTapOpen(const char *ifname,
cleanup: if (ret < 0) { + g_free(*tapname);
This leaves the pointer dangling in the return-via-argument. Use g_clear_pointer(tapname, g_free) instead to ensure it's NULL-ed out or assign to the return-via-argument only at success (e.g. via g_steal_pointer from the local variable).
while (i--) VIR_FORCE_CLOSE(tapfd[i]); }
With all of the requested changes, you can directly use: Reviewed-by: Peter Krempa <pkrempa@redhat.com> in the patches.