[libvirt] [PATCH v2 0/2] Fix issues in qemuARPGetInterfaces and virArpTableGet
v1 -> v2: * Follow Chen Hanxiao's suggestion, Ignore 3 types of virtual interface backends * Adjusted the commit messages of patch 1/2 to reflect the code changes. Lin Ma (2): qemu: Remove network type limitation for qemuARPGetInterfaces virarptable: Return a virArpTablePtr when the nlmsghdr for loop is over src/qemu/qemu_driver.c | 4 +++- src/util/virarptable.c | 2 ++ 2 files changed, 5 insertions(+), 1 deletion(-) -- 2.15.1
When we call qemuARPGetInterfaces to get IP from the host's arp table, We ignore VIR_DOMAIN_NET_TYPE_ETHERNET, VIR_DOMAIN_NET_TYPE_VHOSTUSER and VIR_DOMAIN_NET_TYPE_DIRECT due to the host's arp table won't include the ip <-> mac entries about these type of backends. Signed-off-by: Lin Ma <lma@suse.com> --- src/qemu/qemu_driver.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 2f8d6915e1..f066e02f36 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -20940,7 +20940,9 @@ qemuARPGetInterfaces(virDomainObjPtr vm, goto cleanup; for (i = 0; i < vm->def->nnets; i++) { - if (vm->def->nets[i]->type != VIR_DOMAIN_NET_TYPE_NETWORK) + if (vm->def->nets[i]->type == VIR_DOMAIN_NET_TYPE_ETHERNET || + vm->def->nets[i]->type == VIR_DOMAIN_NET_TYPE_VHOSTUSER || + vm->def->nets[i]->type == VIR_DOMAIN_NET_TYPE_DIRECT) continue; virMacAddrFormat(&(vm->def->nets[i]->mac), macaddr); -- 2.15.1
On 09/13/2018 03:54 AM, Lin Ma wrote:
When we call qemuARPGetInterfaces to get IP from the host's arp table, We ignore VIR_DOMAIN_NET_TYPE_ETHERNET, VIR_DOMAIN_NET_TYPE_VHOSTUSER and
s/ignore/should ignore/
VIR_DOMAIN_NET_TYPE_DIRECT due to the host's arp table won't include the ip <-> mac entries about these type of backends.
Signed-off-by: Lin Ma <lma@suse.com> --- src/qemu/qemu_driver.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
OK - so not my area of expertise - I'm CC'g laine to get his opinion just in case he missed this... I always get overly cautious since there are many network types and this seems to be a rather far reaching change of fetch... You should reference commit e24d4c905 which added this code as it would help focus on when the change was made. Also since this really are disjoint problems, you should have created separate patches especially since none of Sukrit's changes have anything to do with this, but he's CC'd on the series because his change for patch 2/2 did break something... John
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 2f8d6915e1..f066e02f36 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -20940,7 +20940,9 @@ qemuARPGetInterfaces(virDomainObjPtr vm, goto cleanup;
for (i = 0; i < vm->def->nnets; i++) { - if (vm->def->nets[i]->type != VIR_DOMAIN_NET_TYPE_NETWORK) + if (vm->def->nets[i]->type == VIR_DOMAIN_NET_TYPE_ETHERNET || + vm->def->nets[i]->type == VIR_DOMAIN_NET_TYPE_VHOSTUSER || + vm->def->nets[i]->type == VIR_DOMAIN_NET_TYPE_DIRECT) continue;
virMacAddrFormat(&(vm->def->nets[i]->mac), macaddr);
On 09/14/2018 11:27 PM, John Ferlan wrote:
On 09/13/2018 03:54 AM, Lin Ma wrote:
When we call qemuARPGetInterfaces to get IP from the host's arp table, We ignore VIR_DOMAIN_NET_TYPE_ETHERNET, VIR_DOMAIN_NET_TYPE_VHOSTUSER and s/ignore/should ignore/
VIR_DOMAIN_NET_TYPE_DIRECT due to the host's arp table won't include the ip <-> mac entries about these type of backends.
Signed-off-by: Lin Ma <lma@suse.com> --- src/qemu/qemu_driver.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
OK - so not my area of expertise - I'm CC'g laine to get his opinion just in case he missed this... I always get overly cautious since there are many network types and this seems to be a rather far reaching change of fetch...
You should reference commit e24d4c905 which added this code as it would help focus on when the change was made.
Also since this really are disjoint problems, you should have created separate patches especially since none of Sukrit's changes have anything to do with this, but he's CC'd on the series because his change for patch 2/2 did break something...
John
Thanks for your review! Lin
On 09/13/2018 03:54 AM, Lin Ma wrote:
When we call qemuARPGetInterfaces to get IP from the host's arp table, We ignore VIR_DOMAIN_NET_TYPE_ETHERNET, VIR_DOMAIN_NET_TYPE_VHOSTUSER and VIR_DOMAIN_NET_TYPE_DIRECT due to the host's arp table won't include the ip <-> mac entries about these type of backends.
That's not necessarily true. A tap device created by <interface type='ethernet'> can be given a host-side IP address/prefix (right in the XML as a matter of fact: https://libvirt.org/formatdomain.html#ipconfig ) thus making traffic on that interface available to the host's IP stack, and this adds a direct route on the host to the subnet for that address/prefix. If the guest has an IP on that same subnet, then the arp table on the host will contain an entry for the guest (if the guest has sent any traffic from that IP address). Likewise, if an interface is of type='direct' (macvtap) and the switch connected to the physdev of that macvtap device is "hairpinning" traffic (to enable guest<->host communication), or alternately if there is a different physdev on the host connected to the same switch, then there will be an entry in the arp table on the host (again, if the guest has sent or broadcast packets from the IP address of that interface). Likewise, I have no personal experience with vhostuser but it's entirely possible that the vhostuser interface of the guest ends up being in the same broadcast domain as one of the devices (physical or virtual) on the host that has an IP address, and in that case there will be an entry in the host's arp table for the vhostuser device's MAC. I guess the conclusion I would draw from this is that we shouldn't immediately discount *any* type of interface (e.g., a hostdev interface could also show up in the arp table, but of course the change that you've made here actually *adds* the ability to look for those). I would say we should check for the MAC regardless of the type. (BTW, if you were going to do something based on the type of interface, it would be more correct to use virDomainNetGetActualType(vm->dev->nets[i]) rather than vm->dev->nets[i]->type, although I'm saying that you simply shouldn't check the type at all in this case.)
Signed-off-by: Lin Ma <lma@suse.com> --- src/qemu/qemu_driver.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 2f8d6915e1..f066e02f36 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -20940,7 +20940,9 @@ qemuARPGetInterfaces(virDomainObjPtr vm, goto cleanup;
for (i = 0; i < vm->def->nnets; i++) { - if (vm->def->nets[i]->type != VIR_DOMAIN_NET_TYPE_NETWORK) + if (vm->def->nets[i]->type == VIR_DOMAIN_NET_TYPE_ETHERNET || + vm->def->nets[i]->type == VIR_DOMAIN_NET_TYPE_VHOSTUSER || + vm->def->nets[i]->type == VIR_DOMAIN_NET_TYPE_DIRECT) continue;
virMacAddrFormat(&(vm->def->nets[i]->mac), macaddr);
On 09/15/2018 07:14 AM, Laine Stump wrote:
When we call qemuARPGetInterfaces to get IP from the host's arp table, We ignore VIR_DOMAIN_NET_TYPE_ETHERNET, VIR_DOMAIN_NET_TYPE_VHOSTUSER and VIR_DOMAIN_NET_TYPE_DIRECT due to the host's arp table won't include the ip <-> mac entries about these type of backends. That's not necessarily true. A tap device created by <interface type='ethernet'> can be given a host-side IP address/prefix (right in
On 09/13/2018 03:54 AM, Lin Ma wrote: the XML as a matter of fact: https://libvirt.org/formatdomain.html#ipconfig ) thus making traffic on that interface available to the host's IP stack, and this adds a direct route on the host to the subnet for that address/prefix. If the guest has an IP on that same subnet, then the arp table on the host will contain an entry for the guest (if the guest has sent any traffic from that IP address).
Likewise, if an interface is of type='direct' (macvtap) and the switch connected to the physdev of that macvtap device is "hairpinning" traffic (to enable guest<->host communication), or alternately if there is a different physdev on the host connected to the same switch, then there will be an entry in the arp table on the host (again, if the guest has sent or broadcast packets from the IP address of that interface).
Likewise, I have no personal experience with vhostuser but it's entirely possible that the vhostuser interface of the guest ends up being in the same broadcast domain as one of the devices (physical or virtual) on the host that has an IP address, and in that case there will be an entry in the host's arp table for the vhostuser device's MAC.
I guess the conclusion I would draw from this is that we shouldn't immediately discount *any* type of interface (e.g., a hostdev interface could also show up in the arp table, but of course the change that you've made here actually *adds* the ability to look for those). I would say we should check for the MAC regardless of the type.
(BTW, if you were going to do something based on the type of interface, it would be more correct to use virDomainNetGetActualType(vm->dev->nets[i]) rather than vm->dev->nets[i]->type, although I'm saying that you simply shouldn't check the type at all in this case.)
Thanks for your detailed explanation! So It seems that we don't need to check any of type of interface in this case, Actually, No type check at all is the design of patch V1, Please refer to https://www.redhat.com/archives/libvir-list/2018-September/msg00243.html If I use the patch v1's code plus commit message modification, What do you think? Thanks, Lin
commit b00c9c39 removed the label end_of_netlink_messages and 'return table' statement, It causes the function virArpTableGet doesn't return a proper virArpTable pointer. How to reproduce: # virsh domiflist sles12sp3 Interface Type Source Model MAC ------------------------------------------------------- vnet0 network default virtio 52:54:00:cd:02:e6 # virsh domifaddr sles12sp3 --source arp error: Failed to query for interfaces addresses error: An error occurred, but the cause is unknown It seems that the "if (nh->nlmsg_type == NLMSG_DONE)" statement won't be meted. So this patch adds 'return table' when the iterations of nlmsghdr for loop is over. Signed-off-by: Lin Ma <lma@suse.com> Reviewed-by: Chen Hanxiao <chenhanxiao@gmail.com> --- src/util/virarptable.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/util/virarptable.c b/src/util/virarptable.c index 04a6f35571..217a960d5a 100644 --- a/src/util/virarptable.c +++ b/src/util/virarptable.c @@ -152,6 +152,8 @@ virArpTableGet(void) } } + return table; + cleanup: virArpTableFree(table); return NULL; -- 2.15.1
$SUBJ: s/virarptable/util/ On 09/13/2018 03:54 AM, Lin Ma wrote:
commit b00c9c39 removed the label end_of_netlink_messages and 'return table' statement, It causes the function virArpTableGet doesn't return a proper virArpTable pointer.
How to reproduce: # virsh domiflist sles12sp3 Interface Type Source Model MAC ------------------------------------------------------- vnet0 network default virtio 52:54:00:cd:02:e6
# virsh domifaddr sles12sp3 --source arp error: Failed to query for interfaces addresses error: An error occurred, but the cause is unknown
It seems that the "if (nh->nlmsg_type == NLMSG_DONE)" statement won't be meted. So this patch adds 'return table' when the iterations of nlmsghdr for loop is over.
Signed-off-by: Lin Ma <lma@suse.com> Reviewed-by: Chen Hanxiao <chenhanxiao@gmail.com> --- src/util/virarptable.c | 2 ++ 1 file changed, 2 insertions(+)
good catch, thanks - Reviewed-by: John Ferlan <jferlan@redhat.com> (and pushed) John
participants (3)
-
John Ferlan -
Laine Stump -
Lin Ma