[libvirt] [PATCH] network: get vlan info for Open vSwitch interfaces from proper source
This bug was revealed by the crash described in https://bugzilla.redhat.com/show_bug.cgi?id=852383 The vlan info pointer sent to virNetDevOpenvswitchAddPort should never be non-NULL unless there is at least one tag. The factthat such a vlan info pointer was receveid pointed out that a caller was passing the wrong pointer. Instead of sending &net->vlan, the result of virDomainNetGetActualVlan(net) should be sent - that function will look for vlan info in net->data.network.actual->vlan, and in cany case return NULL instead of a pointer if the vlan info it finds has no tags. Aside from causing the crash, sending a hardcoded &net->vlan has the effect of ignoring vlan info from a <network> or <portgroup> config. --- Since I'm not online in a regular fashion for the next few days (too bad I wasn't online in the 12 hours or so *before* the 0.10.0 release instead of after :-/), I would appreciate if whoever ACKs this could push it. Thanks! src/qemu/qemu_command.c | 2 +- src/uml/uml_conf.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 8c32a4d..25f2451 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -258,7 +258,7 @@ qemuNetworkIfaceConnect(virDomainDefPtr def, err = virNetDevTapCreateInBridgePort(brname, &net->ifname, &net->mac, def->uuid, &tapfd, virDomainNetGetActualVirtPortProfile(net), - &net->vlan, + virDomainNetGetActualVlan(net), tap_create_flags); virDomainAuditNetDevice(def, net, "/dev/net/tun", tapfd >= 0); if (err < 0) { diff --git a/src/uml/uml_conf.c b/src/uml/uml_conf.c index 5461b42..410f3e2 100644 --- a/src/uml/uml_conf.c +++ b/src/uml/uml_conf.c @@ -141,7 +141,7 @@ umlConnectTapDevice(virConnectPtr conn, if (virNetDevTapCreateInBridgePort(bridge, &net->ifname, &net->mac, vm->uuid, NULL, virDomainNetGetActualVirtPortProfile(net), - &net->vlan, + virDomainNetGetActualVlan(net), VIR_NETDEV_TAP_CREATE_IFUP) < 0) { if (template_ifname) VIR_FREE(net->ifname); -- 1.7.11.4
On 08/29/2012 06:43 PM, Laine Stump wrote:
This bug was revealed by the crash described in
https://bugzilla.redhat.com/show_bug.cgi?id=852383
The vlan info pointer sent to virNetDevOpenvswitchAddPort should never be non-NULL unless there is at least one tag. The factthat such a vlan info pointer was receveid pointed out that a caller was passing the wrong pointer. Instead of sending&net->vlan, the result of virDomainNetGetActualVlan(net) should be sent - that function will look for vlan info in net->data.network.actual->vlan, and in cany case return NULL instead of a pointer if the vlan info it finds has no tags.
Aside from causing the crash, sending a hardcoded&net->vlan has the effect of ignoring vlan info from a<network> or<portgroup> config. ---
Since I'm not online in a regular fashion for the next few days (too bad I wasn't online in the 12 hours or so *before* the 0.10.0 release instead of after :-/), I would appreciate if whoever ACKs this could push it. Thanks!
Laine, unfortunately, the libvirtd still is crash without my patch after applying your patch :(
src/qemu/qemu_command.c | 2 +- src/uml/uml_conf.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 8c32a4d..25f2451 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -258,7 +258,7 @@ qemuNetworkIfaceConnect(virDomainDefPtr def, err = virNetDevTapCreateInBridgePort(brname,&net->ifname,&net->mac, def->uuid,&tapfd, virDomainNetGetActualVirtPortProfile(net), -&net->vlan, + virDomainNetGetActualVlan(net), tap_create_flags); virDomainAuditNetDevice(def, net, "/dev/net/tun", tapfd>= 0); if (err< 0) { diff --git a/src/uml/uml_conf.c b/src/uml/uml_conf.c index 5461b42..410f3e2 100644 --- a/src/uml/uml_conf.c +++ b/src/uml/uml_conf.c @@ -141,7 +141,7 @@ umlConnectTapDevice(virConnectPtr conn, if (virNetDevTapCreateInBridgePort(bridge,&net->ifname,&net->mac, vm->uuid, NULL, virDomainNetGetActualVirtPortProfile(net), -&net->vlan, + virDomainNetGetActualVlan(net), VIR_NETDEV_TAP_CREATE_IFUP)< 0) { if (template_ifname) VIR_FREE(net->ifname);
On Aug 29, 2012, at 6:19 AM, Alex Jia wrote:
On 08/29/2012 06:43 PM, Laine Stump wrote:
This bug was revealed by the crash described in
https://bugzilla.redhat.com/show_bug.cgi?id=852383
The vlan info pointer sent to virNetDevOpenvswitchAddPort should never be non-NULL unless there is at least one tag. The factthat such a vlan info pointer was receveid pointed out that a caller was passing the wrong pointer. Instead of sending&net->vlan, the result of virDomainNetGetActualVlan(net) should be sent - that function will look for vlan info in net->data.network.actual->vlan, and in cany case return NULL instead of a pointer if the vlan info it finds has no tags.
Aside from causing the crash, sending a hardcoded&net->vlan has the effect of ignoring vlan info from a<network> or<portgroup> config. ---
Since I'm not online in a regular fashion for the next few days (too bad I wasn't online in the 12 hours or so *before* the 0.10.0 release instead of after :-/), I would appreciate if whoever ACKs this could push it. Thanks!
Laine, unfortunately, the libvirtd still is crash without my patch after applying your patch :(
Hi Alex and Laine: What I notice is, that with Laine's patch, for virtualport of type openvswitch, if I do NOT define a VLAN, it crashes. If I do, then I can start my virtual machines. Without Laine's patch, it's the same, which leads me to believe Laine's patch only fixes a symptom of this issue. I'm looking into this now, will see how far I get with it today. But it appears to me to be a bug in the logic which is handling VLANs at a higher layer. Thanks, Kyle
src/qemu/qemu_command.c | 2 +- src/uml/uml_conf.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 8c32a4d..25f2451 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -258,7 +258,7 @@ qemuNetworkIfaceConnect(virDomainDefPtr def, err = virNetDevTapCreateInBridgePort(brname,&net->ifname,&net->mac, def->uuid,&tapfd, virDomainNetGetActualVirtPortProfile(net), -&net->vlan, + virDomainNetGetActualVlan(net), tap_create_flags); virDomainAuditNetDevice(def, net, "/dev/net/tun", tapfd>= 0); if (err< 0) { diff --git a/src/uml/uml_conf.c b/src/uml/uml_conf.c index 5461b42..410f3e2 100644 --- a/src/uml/uml_conf.c +++ b/src/uml/uml_conf.c @@ -141,7 +141,7 @@ umlConnectTapDevice(virConnectPtr conn, if (virNetDevTapCreateInBridgePort(bridge,&net->ifname,&net->mac, vm->uuid, NULL, virDomainNetGetActualVirtPortProfile(net), -&net->vlan, + virDomainNetGetActualVlan(net), VIR_NETDEV_TAP_CREATE_IFUP)< 0) { if (template_ifname) VIR_FREE(net->ifname);
On Aug 29, 2012, at 9:49 AM, Kyle Mestery (kmestery) wrote:
On Aug 29, 2012, at 6:19 AM, Alex Jia wrote:
On 08/29/2012 06:43 PM, Laine Stump wrote:
This bug was revealed by the crash described in
https://bugzilla.redhat.com/show_bug.cgi?id=852383
The vlan info pointer sent to virNetDevOpenvswitchAddPort should never be non-NULL unless there is at least one tag. The factthat such a vlan info pointer was receveid pointed out that a caller was passing the wrong pointer. Instead of sending&net->vlan, the result of virDomainNetGetActualVlan(net) should be sent - that function will look for vlan info in net->data.network.actual->vlan, and in cany case return NULL instead of a pointer if the vlan info it finds has no tags.
Aside from causing the crash, sending a hardcoded&net->vlan has the effect of ignoring vlan info from a<network> or<portgroup> config. ---
Since I'm not online in a regular fashion for the next few days (too bad I wasn't online in the 12 hours or so *before* the 0.10.0 release instead of after :-/), I would appreciate if whoever ACKs this could push it. Thanks!
Laine, unfortunately, the libvirtd still is crash without my patch after applying your patch :(
Hi Alex and Laine:
What I notice is, that with Laine's patch, for virtualport of type openvswitch, if I do NOT define a VLAN, it crashes. If I do, then I can start my virtual machines. Without Laine's patch, it's the same, which leads me to believe Laine's patch only fixes a symptom of this issue.
I'm looking into this now, will see how far I get with it today. But it appears to me to be a bug in the logic which is handling VLANs at a higher layer.
Thanks, Kyle
I just sent a patch, which combined with Laine's patches, stop the crashes in the latest libvirt code when using Open vSwitch virtual ports. I think this should go into the 0.10.1 release for this Friday, because without it libvirt crashes consistently with Open vSwitch virtualports. Thanks, Kyle
src/qemu/qemu_command.c | 2 +- src/uml/uml_conf.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 8c32a4d..25f2451 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -258,7 +258,7 @@ qemuNetworkIfaceConnect(virDomainDefPtr def, err = virNetDevTapCreateInBridgePort(brname,&net->ifname,&net->mac, def->uuid,&tapfd, virDomainNetGetActualVirtPortProfile(net), -&net->vlan, + virDomainNetGetActualVlan(net), tap_create_flags); virDomainAuditNetDevice(def, net, "/dev/net/tun", tapfd>= 0); if (err< 0) { diff --git a/src/uml/uml_conf.c b/src/uml/uml_conf.c index 5461b42..410f3e2 100644 --- a/src/uml/uml_conf.c +++ b/src/uml/uml_conf.c @@ -141,7 +141,7 @@ umlConnectTapDevice(virConnectPtr conn, if (virNetDevTapCreateInBridgePort(bridge,&net->ifname,&net->mac, vm->uuid, NULL, virDomainNetGetActualVirtPortProfile(net), -&net->vlan, + virDomainNetGetActualVlan(net), VIR_NETDEV_TAP_CREATE_IFUP)< 0) { if (template_ifname) VIR_FREE(net->ifname);
On 08/29/2012 10:49 AM, Kyle Mestery (kmestery) wrote:
On Aug 29, 2012, at 6:19 AM, Alex Jia wrote:
On 08/29/2012 06:43 PM, Laine Stump wrote:
This bug was revealed by the crash described in
https://bugzilla.redhat.com/show_bug.cgi?id=852383
The vlan info pointer sent to virNetDevOpenvswitchAddPort should never be non-NULL unless there is at least one tag. The factthat such a vlan info pointer was receveid pointed out that a caller was passing the wrong pointer. Instead of sending&net->vlan, the result of virDomainNetGetActualVlan(net) should be sent - that function will look for vlan info in net->data.network.actual->vlan, and in cany case return NULL instead of a pointer if the vlan info it finds has no tags.
Aside from causing the crash, sending a hardcoded&net->vlan has the effect of ignoring vlan info from a<network> or<portgroup> config. ---
Since I'm not online in a regular fashion for the next few days (too bad I wasn't online in the 12 hours or so *before* the 0.10.0 release instead of after :-/), I would appreciate if whoever ACKs this could push it. Thanks! Laine, unfortunately, the libvirtd still is crash without my patch after applying your patch :(
Hi Alex and Laine:
What I notice is, that with Laine's patch, for virtualport of type openvswitch, if I do NOT define a VLAN, it crashes. If I do, then I can start my virtual machines. Without Laine's patch, it's the same, which leads me to believe Laine's patch only fixes a symptom of this issue.
No, there are actually two separate crashes and two separate issues - look at the two tracebacks in https://bugzilla.redhat.com/show_bug.cgi?id=852383 - they are different at the very top. One crash is due to improper buf usage, and the other due to a bad vlan pointer. The first doesn't show up until the second is fixed (or masked by checking for vlan->nTags > 0). And again, while the checking for vlan->nTags > 0 isn't a bad thing, doing that just masks the symptom of the "original" real issue, which was us sending &net->vlan rather than calling virDomainNetGetActualVlan().
participants (3)
-
Alex Jia -
Kyle Mestery (kmestery) -
Laine Stump