Re: [libvirt] Using Libvirt to change the bridge a virtual network card of a running vm is connected to
I fixed the patch and added it to https://bugzilla.redhat.com/show_bug.cgi?id=784767.
On 01/26/2012 07:10 AM, Hendrik Schwartke wrote:
I fixed the patch and added it to https://bugzilla.redhat.com/show_bug.cgi?id=784767.
Thanks. It's easier to review if we don't have to click through the link, so can you also send the updated patch to this list? -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Of course: diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 4b60839..edf7433 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -40,6 +40,8 @@ #include "qemu_cgroup.h" #include "locking/domain_lock.h" #include "network/bridge_driver.h" +#include "util/virnetdev.h" +#include "util/virnetdevbridge.h" #define VIR_FROM_THIS VIR_FROM_QEMU @@ -1163,6 +1165,44 @@ static virDomainNetDefPtr qemuDomainFindNet(virDomainObjPtr vm, return NULL; } +int qemuDomainChangeNetBridge(virDomainNetDefPtr olddev, + virDomainNetDefPtr newdev) +{ + const char *oldbridge = olddev->data.bridge.brname; + const char *newbridge = newdev->data.bridge.brname; + + VIR_DEBUG("Change bridge for interface %s: %s -> %s", + olddev->ifname, oldbridge, newbridge); + + if (!virNetDevExists(newbridge)) { + qemuReportError(VIR_ERR_OPERATION_FAILED, "%s", + _("unable to remove port from bridge")); + return -1; + } + + if (oldbridge&& + virNetDevBridgeRemovePort(oldbridge, olddev->ifname)< 0) { + qemuReportError(VIR_ERR_OPERATION_FAILED, + _("unable to remove port from bridge %s"), oldbridge); + return -1; + } + if (virNetDevBridgeAddPort(newbridge, newdev->ifname)< 0) { + qemuReportError(VIR_ERR_OPERATION_FAILED, + _("unable to add port to bridge %s"), newbridge); + if (virNetDevBridgeAddPort(oldbridge, olddev->ifname)< 0) { + qemuReportError(VIR_ERR_OPERATION_FAILED, + _("unable to recover former state by adding port to bridge %s"), oldbridge); + VIR_FREE(olddev->data.bridge.brname); + } + return -1; + } + VIR_FREE(olddev->data.bridge.brname); + olddev->data.bridge.brname = newdev->data.bridge.brname; + newdev->data.bridge.brname = NULL; + return 0; +} + + int qemuDomainChangeNetLinkState(struct qemud_driver *driver, virDomainObjPtr vm, virDomainNetDefPtr dev, @@ -1293,6 +1333,13 @@ int qemuDomainChangeNet(struct qemud_driver *driver, return -1; } + if (olddev->type == VIR_DOMAIN_NET_TYPE_BRIDGE +&& dev->type == VIR_DOMAIN_NET_TYPE_BRIDGE +&& STRNEQ_NULLABLE(olddev->data.bridge.brname, dev->data.bridge.brname)) { + if ((ret = qemuDomainChangeNetBridge(olddev, dev))< 0) + return ret; + } + if (olddev->linkstate != dev->linkstate) { if ((ret = qemuDomainChangeNetLinkState(driver, vm, olddev, dev->linkstate))< 0) return ret; diff --git a/src/qemu/qemu_hotplug.h b/src/qemu/qemu_hotplug.h index 0310361..1e1f75c 100644 --- a/src/qemu/qemu_hotplug.h +++ b/src/qemu/qemu_hotplug.h @@ -80,6 +80,8 @@ int qemuDomainChangeNetLinkState(struct qemud_driver *driver, virDomainObjPtr vm, virDomainNetDefPtr dev, int linkstate); +int qemuDomainChangeNetBridge(virDomainNetDefPtr olddev, + virDomainNetDefPtr newdev); int qemuDomainDetachPciDiskDevice(struct qemud_driver *driver, virDomainObjPtr vm, virDomainDeviceDefPtr dev); On 26.01.2012 15:18, Eric Blake wrote:
I fixed the patch and added it to https://bugzilla.redhat.com/show_bug.cgi?id=784767. Thanks. It's easier to review if we don't have to click through the
On 01/26/2012 07:10 AM, Hendrik Schwartke wrote: link, so can you also send the updated patch to this list?
Sorry for taking so long to actually review this patch. (The fact that it didn't apply cleanly (neither the version you sent in email, nor the version attached to https://bugzilla.redhat.com/784767) put it just beyond the threshold of action (the method you used to generate/send the patch resulted in a corrupt patch that couldn't be applied with "git am" or "git am -3"). In general, it's best to send patches to the mailing list with "git send-email", or failing that, by running "git format-patch", then attaching the result to a mail message (git send-email is *greatly* preferred).) (In the end, I pasted the chunks into my work tree by hand.) Your patch addresses one specific case (interface type is 'bridge', and doesn't change, but the bridge name does change) of a large class that could/should be handled (e.g. switch between type='network' and type='bridge', optionally bounce the link to force a DHCP refresh, optionally detach/reattach the device if required (e.g. to switch between macvtap and standard bridge connections)). My opinion is that the existing patch can go in as a useful starting point, and we can expand on it (if someone else has a differing opinion, please say so!) Other than the minimal functionality, I only saw few nits in the code, which can be fixed up before pushing, so ACK from me, but we may want to wait until after the freeze is over and 0.9.11 is released to push. On 01/26/2012 10:29 AM, Hendrik Schwartke wrote:
Of course:
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 4b60839..edf7433 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -40,6 +40,8 @@ #include "qemu_cgroup.h" #include "locking/domain_lock.h" #include "network/bridge_driver.h" +#include "util/virnetdev.h" +#include "util/virnetdevbridge.h"
"util/" is unnecessary, since the util directory is already in the include path.
#define VIR_FROM_THIS VIR_FROM_QEMU
@@ -1163,6 +1165,44 @@ static virDomainNetDefPtr qemuDomainFindNet(virDomainObjPtr vm, return NULL; }
+int qemuDomainChangeNetBridge(virDomainNetDefPtr olddev, + virDomainNetDefPtr newdev)
Although the existing qemuDomainChangeLinkState is a global function, it is unnecessarily so, as is this new function. I think it should just be static (and qemuDomainChangeLinkState should also be made static).
+{ + const char *oldbridge = olddev->data.bridge.brname; + const char *newbridge = newdev->data.bridge.brname; + + VIR_DEBUG("Change bridge for interface %s: %s -> %s", + olddev->ifname, oldbridge, newbridge); + + if (!virNetDevExists(newbridge)) { + qemuReportError(VIR_ERR_OPERATION_FAILED, "%s", + _("unable to remove port from bridge")); + return -1; + } + + if (oldbridge&& + virNetDevBridgeRemovePort(oldbridge, olddev->ifname)< 0) { + qemuReportError(VIR_ERR_OPERATION_FAILED, + _("unable to remove port from bridge %s"), oldbridge); + return -1; + } + if (virNetDevBridgeAddPort(newbridge, newdev->ifname)< 0) { + qemuReportError(VIR_ERR_OPERATION_FAILED, + _("unable to add port to bridge %s"), newbridge); + if (virNetDevBridgeAddPort(oldbridge, olddev->ifname)< 0) { + qemuReportError(VIR_ERR_OPERATION_FAILED, + _("unable to recover former state by adding port to bridge %s"), oldbridge); + VIR_FREE(olddev->data.bridge.brname);
This free is unnecessary - the parent object (and all its minions) will be freed by the caller anyway.
+ } + return -1; + } + VIR_FREE(olddev->data.bridge.brname); + olddev->data.bridge.brname = newdev->data.bridge.brname; + newdev->data.bridge.brname = NULL; + return 0; +} + + int qemuDomainChangeNetLinkState(struct qemud_driver *driver, virDomainObjPtr vm, virDomainNetDefPtr dev, @@ -1293,6 +1333,13 @@ int qemuDomainChangeNet(struct qemud_driver *driver, return -1; }
+ if (olddev->type == VIR_DOMAIN_NET_TYPE_BRIDGE +&& dev->type == VIR_DOMAIN_NET_TYPE_BRIDGE +&& STRNEQ_NULLABLE(olddev->data.bridge.brname, dev->data.bridge.brname)) { + if ((ret = qemuDomainChangeNetBridge(olddev, dev))< 0) + return ret; + } + if (olddev->linkstate != dev->linkstate) { if ((ret = qemuDomainChangeNetLinkState(driver, vm, olddev, dev->linkstate))< 0) return ret; diff --git a/src/qemu/qemu_hotplug.h b/src/qemu/qemu_hotplug.h index 0310361..1e1f75c 100644 --- a/src/qemu/qemu_hotplug.h +++ b/src/qemu/qemu_hotplug.h @@ -80,6 +80,8 @@ int qemuDomainChangeNetLinkState(struct qemud_driver *driver, virDomainObjPtr vm, virDomainNetDefPtr dev, int linkstate); +int qemuDomainChangeNetBridge(virDomainNetDefPtr olddev, + virDomainNetDefPtr newdev); int qemuDomainDetachPciDiskDevice(struct qemud_driver *driver, virDomainObjPtr vm, virDomainDeviceDefPtr dev);
On 26.01.2012 15:18, Eric Blake wrote:
I fixed the patch and added it to https://bugzilla.redhat.com/show_bug.cgi?id=784767. Thanks. It's easier to review if we don't have to click through the
On 01/26/2012 07:10 AM, Hendrik Schwartke wrote: link, so can you also send the updated patch to this list?
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On 03/27/2012 06:01 PM, Laine Stump wrote:
Sorry for taking so long to actually review this patch. (The fact that it didn't apply cleanly (neither the version you sent in email, nor the version attached to https://bugzilla.redhat.com/784767) put it just beyond the threshold of action (the method you used to generate/send the patch resulted in a corrupt patch that couldn't be applied with "git am" or "git am -3"). In general, it's best to send patches to the mailing list with "git send-email", or failing that, by running "git format-patch", then attaching the result to a mail message (git send-email is *greatly* preferred).)
(In the end, I pasted the chunks into my work tree by hand.)
Your patch addresses one specific case (interface type is 'bridge', and doesn't change, but the bridge name does change) of a large class that could/should be handled (e.g. switch between type='network' and type='bridge', optionally bounce the link to force a DHCP refresh, optionally detach/reattach the device if required (e.g. to switch between macvtap and standard bridge connections)).
And if I'm not mistaken, there's also talk on the qemu list about implementing a way to make qemu inform at least the virtio net driver in the guest to do a DHCP refresh, such as after a migration, as another aspect of things to think about.
My opinion is that the existing patch can go in as a useful starting point, and we can expand on it (if someone else has a differing opinion, please say so!)
Other than the minimal functionality, I only saw few nits in the code, which can be fixed up before pushing, so ACK from me, but we may want to wait until after the freeze is over and 0.9.11 is released to push.
I'm okay if it goes in now, before any second release candidate is built, but since you have some cleaned up nits in your tree, I'll let you be the ultimate judge of when to push. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
From: Hendrik Schwartke <hendrik@os-t.de> Previously the only attribute of a network device that could be modified by virUpdateDeviceFlags() ("virsh update-device") was the link state; attempts to change any other attribute would log an error and fail. (One notable exception to this was changing the bridge used by an interface of type='bridge' - that wasn't flagged as an error, but still did nothing.) This patch adds recognition of a change in bridge device name, and supports reconnecting the guest's interface to the new device. --- This had too many modifications from the original patch for me to push without sending in for a 3rd party review. Here are the changes from Hendrik's original patch: Things I noted in my original review: 1) remove unnecessary "util/" from #includes 2) change qemuDomainChangeNetBridge from global to static, since it's only used in the one file. 3) Don't free olddev->data.bridge.brname after failure to add the tap to the new bridge, since the caller will free all of olddev anyway. Things I didn't notice until after I reviewed and ACKed (but fortunately before I pushed): 4) When adding the tap to the new bridge, use olddev->ifname instead of newdev->ifname - the one in newdef is evilly cleared out by the parser, and ifname isn't allowed to change anyway, so using olddev->ifname is proper. 5) Add a case for VIR_DOMAIN_NET_TYPE_BRIDGE to the switch that checks for changes to disallowed items for each type of interface. Even before adding the new functionality, absence of this case had meant that attempts to change link state of a type='bridge' interface would have failed (so I was right that this patch fixes an existing bug, but was wrong about exactly what the bug was.) I have run make check and done functional checking on this code - it does properly change the bridge of an existing guest interface without needing to detach it. src/qemu/qemu_hotplug.c | 54 +++++++++++++++++++++++++++++++++++++++++++++++ 1 files changed, 54 insertions(+), 0 deletions(-) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 38163ba..66837c4 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -40,6 +40,8 @@ #include "qemu_cgroup.h" #include "locking/domain_lock.h" #include "network/bridge_driver.h" +#include "virnetdev.h" +#include "virnetdevbridge.h" #include "virnetdevtap.h" #define VIR_FROM_THIS VIR_FROM_QEMU @@ -1191,6 +1193,40 @@ static virDomainNetDefPtr qemuDomainFindNet(virDomainObjPtr vm, return NULL; } +static +int qemuDomainChangeNetBridge(virDomainNetDefPtr olddev, + virDomainNetDefPtr newdev) +{ + const char *oldbridge = olddev->data.bridge.brname; + const char *newbridge = newdev->data.bridge.brname; + + VIR_DEBUG("Change bridge for interface %s: %s -> %s", + olddev->ifname, oldbridge, newbridge); + + if (virNetDevExists(newbridge) != 1) { + qemuReportError(VIR_ERR_OPERATION_FAILED, + _("bridge %s doesn't exist"), newbridge); + return -1; + } + + if (oldbridge && + virNetDevBridgeRemovePort(oldbridge, olddev->ifname) < 0) { + return -1; + } + if (virNetDevBridgeAddPort(newbridge, olddev->ifname) < 0) { + if (virNetDevBridgeAddPort(oldbridge, olddev->ifname) < 0) { + qemuReportError(VIR_ERR_OPERATION_FAILED, + _("unable to recover former state by adding port" + "to bridge %s"), oldbridge); + } + return -1; + } + VIR_FREE(olddev->data.bridge.brname); + olddev->data.bridge.brname = newdev->data.bridge.brname; + newdev->data.bridge.brname = NULL; + return 0; +} + int qemuDomainChangeNetLinkState(struct qemud_driver *driver, virDomainObjPtr vm, virDomainNetDefPtr dev, @@ -1279,6 +1315,16 @@ int qemuDomainChangeNet(struct qemud_driver *driver, break; + case VIR_DOMAIN_NET_TYPE_BRIDGE: + /* allow changing brname, but not portprofile */ + if (!virNetDevVPortProfileEqual(olddev->data.bridge.virtPortProfile, + dev->data.bridge.virtPortProfile)) { + qemuReportError(VIR_ERR_NO_SUPPORT, + _("cannot modify bridge network device configuration")); + return -1; + } + break; + case VIR_DOMAIN_NET_TYPE_INTERNAL: if (STRNEQ_NULLABLE(olddev->data.internal.name, dev->data.internal.name)) { qemuReportError(VIR_ERR_NO_SUPPORT, @@ -1321,6 +1367,14 @@ int qemuDomainChangeNet(struct qemud_driver *driver, return -1; } + if (olddev->type == VIR_DOMAIN_NET_TYPE_BRIDGE + && dev->type == VIR_DOMAIN_NET_TYPE_BRIDGE + && STRNEQ_NULLABLE(olddev->data.bridge.brname, + dev->data.bridge.brname)) { + if ((ret = qemuDomainChangeNetBridge(olddev, dev)) < 0) + return ret; + } + if (olddev->linkstate != dev->linkstate) { if ((ret = qemuDomainChangeNetLinkState(driver, vm, olddev, dev->linkstate)) < 0) return ret; -- 1.7.7.6
On Thu, Mar 29, 2012 at 04:38:50PM -0400, Laine Stump wrote:
From: Hendrik Schwartke <hendrik@os-t.de>
Previously the only attribute of a network device that could be modified by virUpdateDeviceFlags() ("virsh update-device") was the link state; attempts to change any other attribute would log an error and fail. (One notable exception to this was changing the bridge used by an interface of type='bridge' - that wasn't flagged as an error, but still did nothing.)
This patch adds recognition of a change in bridge device name, and supports reconnecting the guest's interface to the new device. ---
This had too many modifications from the original patch for me to push without sending in for a 3rd party review. Here are the changes from Hendrik's original patch:
Things I noted in my original review:
1) remove unnecessary "util/" from #includes
2) change qemuDomainChangeNetBridge from global to static, since it's only used in the one file.
3) Don't free olddev->data.bridge.brname after failure to add the tap to the new bridge, since the caller will free all of olddev anyway.
Things I didn't notice until after I reviewed and ACKed (but fortunately before I pushed):
4) When adding the tap to the new bridge, use olddev->ifname instead of newdev->ifname - the one in newdef is evilly cleared out by the parser, and ifname isn't allowed to change anyway, so using olddev->ifname is proper.
5) Add a case for VIR_DOMAIN_NET_TYPE_BRIDGE to the switch that checks for changes to disallowed items for each type of interface. Even before adding the new functionality, absence of this case had meant that attempts to change link state of a type='bridge' interface would have failed (so I was right that this patch fixes an existing bug, but was wrong about exactly what the bug was.)
I have run make check and done functional checking on this code - it does properly change the bridge of an existing guest interface without needing to detach it.
src/qemu/qemu_hotplug.c | 54 +++++++++++++++++++++++++++++++++++++++++++++++ 1 files changed, 54 insertions(+), 0 deletions(-)
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 38163ba..66837c4 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -40,6 +40,8 @@ #include "qemu_cgroup.h" #include "locking/domain_lock.h" #include "network/bridge_driver.h" +#include "virnetdev.h" +#include "virnetdevbridge.h" #include "virnetdevtap.h"
#define VIR_FROM_THIS VIR_FROM_QEMU @@ -1191,6 +1193,40 @@ static virDomainNetDefPtr qemuDomainFindNet(virDomainObjPtr vm, return NULL; }
+static +int qemuDomainChangeNetBridge(virDomainNetDefPtr olddev, + virDomainNetDefPtr newdev) +{ + const char *oldbridge = olddev->data.bridge.brname; + const char *newbridge = newdev->data.bridge.brname; + + VIR_DEBUG("Change bridge for interface %s: %s -> %s", + olddev->ifname, oldbridge, newbridge); + + if (virNetDevExists(newbridge) != 1) { + qemuReportError(VIR_ERR_OPERATION_FAILED, + _("bridge %s doesn't exist"), newbridge); + return -1; + } + + if (oldbridge && + virNetDevBridgeRemovePort(oldbridge, olddev->ifname) < 0) { + return -1; + } + if (virNetDevBridgeAddPort(newbridge, olddev->ifname) < 0) { + if (virNetDevBridgeAddPort(oldbridge, olddev->ifname) < 0) { + qemuReportError(VIR_ERR_OPERATION_FAILED, + _("unable to recover former state by adding port" + "to bridge %s"), oldbridge); + } + return -1; + }
I think you need to emit 2 audit notifications here, one for the bridge being removed and one for the bridge being added. Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On 03/29/2012 05:52 PM, Daniel P. Berrange wrote:
On Thu, Mar 29, 2012 at 04:38:50PM -0400, Laine Stump wrote:
+ if (oldbridge && + virNetDevBridgeRemovePort(oldbridge, olddev->ifname) < 0) { + return -1; + } + if (virNetDevBridgeAddPort(newbridge, olddev->ifname) < 0) { + if (virNetDevBridgeAddPort(oldbridge, olddev->ifname) < 0) { + qemuReportError(VIR_ERR_OPERATION_FAILED, + _("unable to recover former state by adding port" + "to bridge %s"), oldbridge); + } + return -1; + } I think you need to emit 2 audit notifications here, one for the bridge being removed and one for the bridge being added.
That does sound like a good idea, but the current virDomainAuditNet() function only reports MAC address, and virDomainAuditNetDevice() only reports "/dev/net/tun" - neither of them gives any information about the name of tap device or which bridge it is being attached to. Right now, here are the audit messages that are logged when I do a full device detach/attach of a network device: type=VIRT_RESOURCE msg=audit(1333090567.694:1051): pid=0 uid=0 auid=4294967295 ses=4294967295 subj=system_u:system_r:virtd_t:s0-s0:c0.c1023 msg='virt=kvm resrc=net reason=detach vm="F14" uuid=de3aa186-be64-088e-b64a-a1a03e023ffd old-net=52:54:00:00:01:81 new-net=?: exe="/usr/sbin/libvirtd" hostname=? addr=? terminal=? res=success' type=VIRT_RESOURCE msg=audit(1333090573.195:1053): pid=0 uid=0 auid=4294967295 ses=4294967295 subj=system_u:system_r:virtd_t:s0-s0:c0.c1023 msg='virt=kvm resrc=net reason=open vm="F14" uuid=de3aa186-be64-088e-b64a-a1a03e023ffd net=52:54:00:00:01:81 path="/dev/net/tun" rdev=0A:C8: exe="/usr/sbin/libvirtd" hostname=? addr=? terminal=? res=success' type=VIRT_RESOURCE msg=audit(1333090573.196:1054): pid=0 uid=0 auid=4294967295 ses=4294967295 subj=system_u:system_r:virtd_t:s0-s0:c0.c1023 msg='virt=kvm resrc=net reason=open vm="F14" uuid=de3aa186-be64-088e-b64a-a1a03e023ffd net=52:54:00:00:01:81 path="/dev/vhost-net" rdev=0A:EE: exe="/usr/sbin/libvirtd" hostname=? addr=? terminal=? res=success' type=VIRT_RESOURCE msg=audit(1333090574.092:1055): pid=0 uid=0 auid=4294967295 ses=4294967295 subj=system_u:system_r:virtd_t:s0-s0:c0.c1023 msg='virt=kvm resrc=net reason=attach vm="F14" uuid=de3aa186-be64-088e-b64a-a1a03e023ffd old-net=? new-net=52:54:00:00:01:81: exe="/usr/sbin/libvirtd" hostname=? addr=? terminal=? res=success' It does a good job of telling me the MAC address that's going to be used by the domain, but nothing about how it's connected to the network. If we're staying within the current boundaries of reporting, is there really value in logging a pair of messages that are ultimately just telling us that the same mac address was detached then immediately reattached, but not saying anything about what it was connected to? Alternately, if we're going to start reporting about changes in network connection, shouldn't we also be reporting what those connections are to begin with? (I think that's a change in scope of what's being audited, and requires a separate patch if we decide we want to do that).
On Fri, Mar 30, 2012 at 03:13:31AM -0400, Laine Stump wrote:
On 03/29/2012 05:52 PM, Daniel P. Berrange wrote:
On Thu, Mar 29, 2012 at 04:38:50PM -0400, Laine Stump wrote:
+ if (oldbridge && + virNetDevBridgeRemovePort(oldbridge, olddev->ifname) < 0) { + return -1; + } + if (virNetDevBridgeAddPort(newbridge, olddev->ifname) < 0) { + if (virNetDevBridgeAddPort(oldbridge, olddev->ifname) < 0) { + qemuReportError(VIR_ERR_OPERATION_FAILED, + _("unable to recover former state by adding port" + "to bridge %s"), oldbridge); + } + return -1; + } I think you need to emit 2 audit notifications here, one for the bridge being removed and one for the bridge being added.
That does sound like a good idea, but the current virDomainAuditNet() function only reports MAC address, and virDomainAuditNetDevice() only reports "/dev/net/tun" - neither of them gives any information about the name of tap device or which bridge it is being attached to.
Right now, here are the audit messages that are logged when I do a full device detach/attach of a network device:
type=VIRT_RESOURCE msg=audit(1333090567.694:1051): pid=0 uid=0 auid=4294967295 ses=4294967295 subj=system_u:system_r:virtd_t:s0-s0:c0.c1023 msg='virt=kvm resrc=net reason=detach vm="F14" uuid=de3aa186-be64-088e-b64a-a1a03e023ffd old-net=52:54:00:00:01:81 new-net=?: exe="/usr/sbin/libvirtd" hostname=? addr=? terminal=? res=success'
type=VIRT_RESOURCE msg=audit(1333090573.195:1053): pid=0 uid=0 auid=4294967295 ses=4294967295 subj=system_u:system_r:virtd_t:s0-s0:c0.c1023 msg='virt=kvm resrc=net reason=open vm="F14" uuid=de3aa186-be64-088e-b64a-a1a03e023ffd net=52:54:00:00:01:81 path="/dev/net/tun" rdev=0A:C8: exe="/usr/sbin/libvirtd" hostname=? addr=? terminal=? res=success' type=VIRT_RESOURCE msg=audit(1333090573.196:1054): pid=0 uid=0 auid=4294967295 ses=4294967295 subj=system_u:system_r:virtd_t:s0-s0:c0.c1023 msg='virt=kvm resrc=net reason=open vm="F14" uuid=de3aa186-be64-088e-b64a-a1a03e023ffd net=52:54:00:00:01:81 path="/dev/vhost-net" rdev=0A:EE: exe="/usr/sbin/libvirtd" hostname=? addr=? terminal=? res=success' type=VIRT_RESOURCE msg=audit(1333090574.092:1055): pid=0 uid=0 auid=4294967295 ses=4294967295 subj=system_u:system_r:virtd_t:s0-s0:c0.c1023 msg='virt=kvm resrc=net reason=attach vm="F14" uuid=de3aa186-be64-088e-b64a-a1a03e023ffd old-net=? new-net=52:54:00:00:01:81: exe="/usr/sbin/libvirtd" hostname=? addr=? terminal=? res=success'
It does a good job of telling me the MAC address that's going to be used by the domain, but nothing about how it's connected to the network.
Hmm, this seems flawed to me. The intent of the auditing code we added was to provide information about any host resource that the VM is associated with. So I'm really surprised we're not providing any info about the host network interface. I suspect this should be fixed.
If we're staying within the current boundaries of reporting, is there really value in logging a pair of messages that are ultimately just telling us that the same mac address was detached then immediately reattached, but not saying anything about what it was connected to? Alternately, if we're going to start reporting about changes in network connection, shouldn't we also be reporting what those connections are to begin with? (I think that's a change in scope of what's being audited, and requires a separate patch if we decide we want to do that).
I think we should still audit this change, even though current audit rules appear broken. Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On 03/30/2012 06:25 AM, Daniel P. Berrange wrote:
On Fri, Mar 30, 2012 at 03:13:31AM -0400, Laine Stump wrote:
On 03/29/2012 05:52 PM, Daniel P. Berrange wrote:
On Thu, Mar 29, 2012 at 04:38:50PM -0400, Laine Stump wrote:
+ if (oldbridge && + virNetDevBridgeRemovePort(oldbridge, olddev->ifname) < 0) { + return -1; + } + if (virNetDevBridgeAddPort(newbridge, olddev->ifname) < 0) { + if (virNetDevBridgeAddPort(oldbridge, olddev->ifname) < 0) { + qemuReportError(VIR_ERR_OPERATION_FAILED, + _("unable to recover former state by adding port" + "to bridge %s"), oldbridge); + } + return -1; + } I think you need to emit 2 audit notifications here, one for the bridge being removed and one for the bridge being added. That does sound like a good idea, but the current virDomainAuditNet() function only reports MAC address, and virDomainAuditNetDevice() only reports "/dev/net/tun" - neither of them gives any information about the name of tap device or which bridge it is being attached to.
Right now, here are the audit messages that are logged when I do a full device detach/attach of a network device:
type=VIRT_RESOURCE msg=audit(1333090567.694:1051): pid=0 uid=0 auid=4294967295 ses=4294967295 subj=system_u:system_r:virtd_t:s0-s0:c0.c1023 msg='virt=kvm resrc=net reason=detach vm="F14" uuid=de3aa186-be64-088e-b64a-a1a03e023ffd old-net=52:54:00:00:01:81 new-net=?: exe="/usr/sbin/libvirtd" hostname=? addr=? terminal=? res=success'
type=VIRT_RESOURCE msg=audit(1333090573.195:1053): pid=0 uid=0 auid=4294967295 ses=4294967295 subj=system_u:system_r:virtd_t:s0-s0:c0.c1023 msg='virt=kvm resrc=net reason=open vm="F14" uuid=de3aa186-be64-088e-b64a-a1a03e023ffd net=52:54:00:00:01:81 path="/dev/net/tun" rdev=0A:C8: exe="/usr/sbin/libvirtd" hostname=? addr=? terminal=? res=success' type=VIRT_RESOURCE msg=audit(1333090573.196:1054): pid=0 uid=0 auid=4294967295 ses=4294967295 subj=system_u:system_r:virtd_t:s0-s0:c0.c1023 msg='virt=kvm resrc=net reason=open vm="F14" uuid=de3aa186-be64-088e-b64a-a1a03e023ffd net=52:54:00:00:01:81 path="/dev/vhost-net" rdev=0A:EE: exe="/usr/sbin/libvirtd" hostname=? addr=? terminal=? res=success' type=VIRT_RESOURCE msg=audit(1333090574.092:1055): pid=0 uid=0 auid=4294967295 ses=4294967295 subj=system_u:system_r:virtd_t:s0-s0:c0.c1023 msg='virt=kvm resrc=net reason=attach vm="F14" uuid=de3aa186-be64-088e-b64a-a1a03e023ffd old-net=? new-net=52:54:00:00:01:81: exe="/usr/sbin/libvirtd" hostname=? addr=? terminal=? res=success'
It does a good job of telling me the MAC address that's going to be used by the domain, but nothing about how it's connected to the network. Hmm, this seems flawed to me. The intent of the auditing code we added was to provide information about any host resource that the VM is associated with. So I'm really surprised we're not providing any info about the host network interface. I suspect this should be fixed.
If we're staying within the current boundaries of reporting, is there really value in logging a pair of messages that are ultimately just telling us that the same mac address was detached then immediately reattached, but not saying anything about what it was connected to? Alternately, if we're going to start reporting about changes in network connection, shouldn't we also be reporting what those connections are to begin with? (I think that's a change in scope of what's being audited, and requires a separate patch if we decide we want to do that). I think we should still audit this change, even though current audit rules appear broken.
Okay. Would the patch I've attached here be adequate? If so, I'll squash it into the rest of the patch. Beyond that, to fix the problem of general inadequacy in the current audits, would it be enough to add tap device and bridge names to the current attach and detach log messages? Or is the audit message format very strict, and we would need to do a separate log message for tap device and for bridge device?
On Fri, Mar 30, 2012 at 10:30:01AM -0400, Laine Stump wrote:
On 03/30/2012 06:25 AM, Daniel P. Berrange wrote:
On Fri, Mar 30, 2012 at 03:13:31AM -0400, Laine Stump wrote:
On 03/29/2012 05:52 PM, Daniel P. Berrange wrote:
On Thu, Mar 29, 2012 at 04:38:50PM -0400, Laine Stump wrote:
+ if (oldbridge && + virNetDevBridgeRemovePort(oldbridge, olddev->ifname) < 0) { + return -1; + } + if (virNetDevBridgeAddPort(newbridge, olddev->ifname) < 0) { + if (virNetDevBridgeAddPort(oldbridge, olddev->ifname) < 0) { + qemuReportError(VIR_ERR_OPERATION_FAILED, + _("unable to recover former state by adding port" + "to bridge %s"), oldbridge); + } + return -1; + } I think you need to emit 2 audit notifications here, one for the bridge being removed and one for the bridge being added. That does sound like a good idea, but the current virDomainAuditNet() function only reports MAC address, and virDomainAuditNetDevice() only reports "/dev/net/tun" - neither of them gives any information about the name of tap device or which bridge it is being attached to.
Right now, here are the audit messages that are logged when I do a full device detach/attach of a network device:
type=VIRT_RESOURCE msg=audit(1333090567.694:1051): pid=0 uid=0 auid=4294967295 ses=4294967295 subj=system_u:system_r:virtd_t:s0-s0:c0.c1023 msg='virt=kvm resrc=net reason=detach vm="F14" uuid=de3aa186-be64-088e-b64a-a1a03e023ffd old-net=52:54:00:00:01:81 new-net=?: exe="/usr/sbin/libvirtd" hostname=? addr=? terminal=? res=success'
type=VIRT_RESOURCE msg=audit(1333090573.195:1053): pid=0 uid=0 auid=4294967295 ses=4294967295 subj=system_u:system_r:virtd_t:s0-s0:c0.c1023 msg='virt=kvm resrc=net reason=open vm="F14" uuid=de3aa186-be64-088e-b64a-a1a03e023ffd net=52:54:00:00:01:81 path="/dev/net/tun" rdev=0A:C8: exe="/usr/sbin/libvirtd" hostname=? addr=? terminal=? res=success' type=VIRT_RESOURCE msg=audit(1333090573.196:1054): pid=0 uid=0 auid=4294967295 ses=4294967295 subj=system_u:system_r:virtd_t:s0-s0:c0.c1023 msg='virt=kvm resrc=net reason=open vm="F14" uuid=de3aa186-be64-088e-b64a-a1a03e023ffd net=52:54:00:00:01:81 path="/dev/vhost-net" rdev=0A:EE: exe="/usr/sbin/libvirtd" hostname=? addr=? terminal=? res=success' type=VIRT_RESOURCE msg=audit(1333090574.092:1055): pid=0 uid=0 auid=4294967295 ses=4294967295 subj=system_u:system_r:virtd_t:s0-s0:c0.c1023 msg='virt=kvm resrc=net reason=attach vm="F14" uuid=de3aa186-be64-088e-b64a-a1a03e023ffd old-net=? new-net=52:54:00:00:01:81: exe="/usr/sbin/libvirtd" hostname=? addr=? terminal=? res=success'
It does a good job of telling me the MAC address that's going to be used by the domain, but nothing about how it's connected to the network. Hmm, this seems flawed to me. The intent of the auditing code we added was to provide information about any host resource that the VM is associated with. So I'm really surprised we're not providing any info about the host network interface. I suspect this should be fixed.
If we're staying within the current boundaries of reporting, is there really value in logging a pair of messages that are ultimately just telling us that the same mac address was detached then immediately reattached, but not saying anything about what it was connected to? Alternately, if we're going to start reporting about changes in network connection, shouldn't we also be reporting what those connections are to begin with? (I think that's a change in scope of what's being audited, and requires a separate patch if we decide we want to do that). I think we should still audit this change, even though current audit rules appear broken.
Okay. Would the patch I've attached here be adequate? If so, I'll squash it into the rest of the patch.
Beyond that, to fix the problem of general inadequacy in the current audits, would it be enough to add tap device and bridge names to the current attach and detach log messages? Or is the audit message format very strict, and we would need to do a separate log message for tap device and for bridge device?
From 3b277f2c2138b98145d6b5e18c844be5d43b8b29 Mon Sep 17 00:00:00 2001 From: Laine Stump <laine@laine.org> Date: Fri, 30 Mar 2012 10:13:37 -0400 Subject: [PATCH] qemu: add audit logs when switching bridges
This adds in a standard audit log for detaching and attaching a network device when the bridge being used is changed.
The discussion about this led to the idea that the audit logs being generated are insufficient, since they don't say anything about which tap device is used, nor about which bridge it is attached to, but that should be fixed by a separate patch, and this gets the current patch properly wired into the infrastructure. --- src/qemu/qemu_hotplug.c | 8 +++++--- 1 files changed, 5 insertions(+), 3 deletions(-)
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 66837c4..7699e9d 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -1194,7 +1194,8 @@ static virDomainNetDefPtr qemuDomainFindNet(virDomainObjPtr vm, }
static -int qemuDomainChangeNetBridge(virDomainNetDefPtr olddev, +int qemuDomainChangeNetBridge(virDomainObjPtr vm, + virDomainNetDefPtr olddev, virDomainNetDefPtr newdev) { const char *oldbridge = olddev->data.bridge.brname; @@ -1221,9 +1222,11 @@ int qemuDomainChangeNetBridge(virDomainNetDefPtr olddev, } return -1; } + virDomainAuditNet(vm, NULL, olddev, "detach", true);
This needs to be moved up, otherwise if we successfully detach, but fail to attach we'll not emit the audit message. Also we I think we should log "detach", false if detaching fails, and likewise 'attach', false' if attaching fails
VIR_FREE(olddev->data.bridge.brname); olddev->data.bridge.brname = newdev->data.bridge.brname; newdev->data.bridge.brname = NULL; + virDomainAuditNet(vm, NULL, olddev, "attach", true); return 0; }
Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On 03/30/2012 10:39 AM, Daniel P. Berrange wrote:
On Fri, Mar 30, 2012 at 10:30:01AM -0400, Laine Stump wrote:
From 3b277f2c2138b98145d6b5e18c844be5d43b8b29 Mon Sep 17 00:00:00 2001 From: Laine Stump <laine@laine.org> Date: Fri, 30 Mar 2012 10:13:37 -0400 Subject: [PATCH] qemu: add audit logs when switching bridges
This adds in a standard audit log for detaching and attaching a network device when the bridge being used is changed.
The discussion about this led to the idea that the audit logs being generated are insufficient, since they don't say anything about which tap device is used, nor about which bridge it is attached to, but that should be fixed by a separate patch, and this gets the current patch properly wired into the infrastructure. --- src/qemu/qemu_hotplug.c | 8 +++++--- 1 files changed, 5 insertions(+), 3 deletions(-)
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 66837c4..7699e9d 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -1194,7 +1194,8 @@ static virDomainNetDefPtr qemuDomainFindNet(virDomainObjPtr vm, }
static -int qemuDomainChangeNetBridge(virDomainNetDefPtr olddev, +int qemuDomainChangeNetBridge(virDomainObjPtr vm, + virDomainNetDefPtr olddev, virDomainNetDefPtr newdev) { const char *oldbridge = olddev->data.bridge.brname; @@ -1221,9 +1222,11 @@ int qemuDomainChangeNetBridge(virDomainNetDefPtr olddev, } return -1; } + virDomainAuditNet(vm, NULL, olddev, "detach", true); This needs to be moved up, otherwise if we successfully detach, but fail to attach we'll not emit the audit message. Also we I think we should log "detach", false if detaching fails, and likewise 'attach', false' if attaching fails
Okay, I've redone the auditing calls, placing them immediately after each call to add/remove port, calling regardless of whether the operation was successful or not. I haven't addressed the higher level problem of adding bridge info to the audit message (although I have made sure that the NetDef object sent to the audit function has the proper information - that's why the brname is moved around at different times than previously). Does this (squashed into the initial patch that had no auditing) look adequate? (I've tested this code, including failure to attach to the new bridge (by trying to attach to "eth0"), and it does work as you suggest).
On 03/30/2012 12:22 PM, Laine Stump wrote:
Subject: [PATCH] qemu: add audit logs when switching bridges
This adds in a standard audit log for detaching and attaching a network device when the bridge being used is changed.
All *attempts* to detach or attach a tap to a bridge are logged, along with whether or not they are successful.
The discussion about this led to the idea that the audit logs being generated are insufficient, since they don't say anything about which tap device is used, nor about which bridge it is attached to, but that should be fixed by a separate patch, and this gets the current patch properly wired into the infrastructure. --- src/qemu/qemu_hotplug.c | 36 ++++++++++++++++++++++++------------ 1 files changed, 24 insertions(+), 12 deletions(-)
ACK. This looks like you have correctly called into the audit framework, and I agree that a later patch to improve the audit framework to output additional useful information is now possible. I recommend that get this in before rc2. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 03/30/2012 06:23 PM, Eric Blake wrote:
Subject: [PATCH] qemu: add audit logs when switching bridges
This adds in a standard audit log for detaching and attaching a network device when the bridge being used is changed.
All *attempts* to detach or attach a tap to a bridge are logged, along with whether or not they are successful.
The discussion about this led to the idea that the audit logs being generated are insufficient, since they don't say anything about which tap device is used, nor about which bridge it is attached to, but that should be fixed by a separate patch, and this gets the current patch properly wired into the infrastructure. --- src/qemu/qemu_hotplug.c | 36 ++++++++++++++++++++++++------------ 1 files changed, 24 insertions(+), 12 deletions(-) ACK. This looks like you have correctly called into the audit
On 03/30/2012 12:22 PM, Laine Stump wrote: framework, and I agree that a later patch to improve the audit framework to output additional useful information is now possible.
I recommend that get this in before rc2.
Thanks to Hendrik Schwartke for writing the original patch, and to Eric and Dan for the reviews and suggestions! I squashed the Auditing code into my earlier modification of Hendrik's bridge-change patch, and pushed (I also added references to the two open BZes that are related to this functionality, one upstream and one for RHEL).
participants (4)
-
Daniel P. Berrange -
Eric Blake -
Hendrik Schwartke -
Laine Stump