<html xmlns:o="urn:schemas-microsoft-com:office:office" xmlns:w="urn:schemas-microsoft-com:office:word" xmlns:m="http://schemas.microsoft.com/office/2004/12/omml" xmlns="http://www.w3.org/TR/REC-html40"><head><meta http-equiv=Content-Type content="text/html; charset=utf-8"><meta name=Generator content="Microsoft Word 15 (filtered medium)"><style></style></head><body lang=en-US link=blue vlink="#954F72"><div class=WordSection1>Thanks for your detailed analysis, I will remake a patch<o:p></o:p><o:p> </o:p><o:p> </o:p><o:p> </o:p><div style='mso-element:para-border-div;border:none;border-top:solid #E1E1E1 1.0pt;padding:3.0pt 0cm 0cm 0cm'>发件人: <a href="mailto:laine@redhat.com">Laine Stump</a> 发送时间: Wednesday, June 17, 2020 7:46 AM 收件人: <a href="mailto:libvir-list@redhat.com">libvir-list@redhat.com</a> 抄送: <a href="mailto:danielhb413@gmail.com">Daniel Henrique Barboza</a>; <a href="mailto:owen.si@ucloud.cn">Bingsong Si</a>; <a href="mailto:gongwei@smartx.com">Wei Gong</a> 主题: Re: [PATCH] network: Fix a race condition when shutdown & start vm at the same time</div><o:p> </o:p>(BTW, this other patch is also trying to solve the same problem:<o:p> </o:p><o:p> </o:p>https://www.redhat.com/archives/libvir-list/2020-June/msg00525.html<o:p> </o:p><o:p> </o:p>I made comments there earlier, and have learned a bit more since then:<o:p> </o:p><o:p> </o:p>https://www.redhat.com/archives/libvir-list/2020-June/msg00634.html<o:p> </o:p><o:p> </o:p>On 6/15/20 2:36 PM, Daniel Henrique Barboza wrote:><o:p> </o:p>><o:p> </o:p>> On 6/11/20 6:58 AM, Bingsong Si wrote:>> when shutdown vm, the qemuProcessStop cleanup virtual interface in >> two steps:><o:p> </o:p>> s/when/When><o:p> </o:p>>> 1. qemuProcessKill kill qemu process, and vif disappeared>> 2. ovs-vsctl del-port from the brige>><o:p> </o:p>>> if start a vm in the middle of the two steps, the new vm will reused >> the vif,><o:p> </o:p>> s/if/If><o:p> </o:p>>> but removed from bridge by step 2>><o:p> </o:p>>> Signed-off-by: Bingsong Si <owen.si@ucloud.cn>>> --->> src/qemu/qemu_process.c | 8 +++++--->> 1 file changed, 5 insertions(+), 3 deletions(-)>><o:p> </o:p>>> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c>> index d36088ba98..706248815a 100644>> --- a/src/qemu/qemu_process.c>> +++ b/src/qemu/qemu_process.c>> @@ -7483,9 +7483,11 @@ void qemuProcessStop(virQEMUDriverPtr driver,>> if (vport->virtPortType == >> VIR_NETDEV_VPORT_PROFILE_MIDONET) {>> ignore_value(virNetDevMidonetUnbindPort(vport));>> } else if (vport->virtPortType == >> VIR_NETDEV_VPORT_PROFILE_OPENVSWITCH) {>> - ignore_value(virNetDevOpenvswitchRemovePort(>> - virDomainNetGetActualBridgeName(net),>> - net->ifname));>> + virMacAddr mac;>> + if (virNetDevGetMAC(net->ifname, &mac) < 0 || >> !virMacAddrCmp(&mac, &net->mac))<o:p> </o:p><o:p> </o:p>(Before anything else - virNetDevGetMAC() will actually *log* an error in libvirt's logs if the device isn't found (which will be in nearly 100% of all cases). That will lead to people reporting it as a bug, which gets very time consuming and expensive for anyone providing commercial support for a product that uses libvirt. If it is really necessary to check the MAC address of a device that legitimately may or may not exist, then there will need to be a "Quiet" version of the function that doesn't log any errors.)(Update after thinking about it - I don't think we should be checking the MAC address anyway, as it doesn't reliably differentiate "new" tap from "old" tap).<o:p> </o:p><o:p> </o:p>><o:p> </o:p>> Extra space between "||" and "!virMacAddrCmp(.."><o:p> </o:p>><o:p> </o:p>> With these nits fixed:><o:p> </o:p>><o:p> </o:p>> Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com><o:p> </o:p><o:p> </o:p><o:p> </o:p>This patch is attempting to solve a race condition between one guest starting at the same time another is stopping, and the mess that results if the new guest uses the same name for its tap device as the old guest used. For example, lets say libvirt thread A / guest A is doing this:<o:p> </o:p><o:p> </o:p>A1) the QEMU process is terminated (and tap device, e.g. "vnet0", is implicitly deleted) either by libvirtd or by some external force (including possibly the QEMU process itself<o:p> </o:p>A2) the tap's associated port (also "vnet0") is removed from the OVS switch by libvirt.<o:p> </o:p><o:p> </o:p>While libvirt thread B is doing this:<o:p> </o:p>B1) a new tap device is created for a new QEMU process. If A1 has already happened, then the kernel will likely give the new tap the same name - "vnet0".<o:p> </o:p>B2) the new tap device is attached to an OVS switch (or possibly a Linux host bridge).<o:p> </o:p><o:p> </o:p>The problem occurs when if B2 happens before A2, which could result in B2 attaching the new tap to the OVS switch, and then A2 disconnecting it from the switch. So libvirt thinks the new QEMU guest tap is attached to the switch, but it isn't.<o:p> </o:p><o:p> </o:p>This patch attempts to eliminate the race by checking, prior to removing "old tap"s port on the switch, that 1) the tap device doesn't exist, or that if it does 2) that the MAC address of the tap device is unchanged.<o:p> </o:p><o:p> </o:p>Assuming that the two guests would not use the same MAC address for their tap devices (which is probably the case, but isn't *required* to be true), this does significantly narrow the potential time for a race condition, and in particular makes sure that we never remove a port that hasn't just been "re-added" by the new QEMU.<o:p> </o:p><o:p> </o:p>However, this just creates a smaller window for the race, and different problem for the remainder of the time.<o:p> </o:p>1) smaller window - it would still be possible for the following to happen:<o:p> </o:p><o:p> </o:p> a) old qemu terminates, tap device "vnet0" is deleted<o:p> </o:p> b) libvirt checks MAC address and learns that there is no device "vnet0",<o:p> </o:p> so it calls virNetDevOpenvswitchRemovePort(), but before ovs-vsctl<o:p> </o:p> can be called...<o:p> </o:p> c) libvirt creates a new tap device for new QEMU, kernel names it "vnet0"<o:p> </o:p> d) libvirt calls virNetDevOpenvswitchAddPort() and the new tap<o:p> </o:p> device "vnet0" to the switch<o:p> </o:p> e) the ovs-vsctl from (b) is finally able to run, and removes "vnet0"<o:p> </o:p> from the switch.<o:p> </o:p><o:p> </o:p>Granted, this is *highly* unlikely, since there is nothing extra between checking MAC address and removing the port, but there is nothing enforcing it.<o:p> </o:p>2) New Problem - I think the testing here was done with two guests who both attached their tap to the same (or another) OVS switch. It's relying on the ability to attach the tap device to a new switch/bridge even if it is already attached to some other switch bridge. Normally ovs-vsctl would refuse to add a port to a switch if a port by that same name was already on any OVS switch. I just looked it up though, and in this case libvirt is able to make this work by including "--if-exists del-port $ifname" in the ovs-vsctl command that *adds* the new port. However, if you have the same situation but the new switch device is instead a Linux bridge, the attempt to attach to the bridge fails.<o:p> </o:p><o:p> </o:p>So, if thread "B" has already created the new tap device by the time thread "A" is deciding whether or not to remove the old port, the port won't be removed, and if the new guest "B" is using a Linux host bridge, libvirt will fail to attach the new tap to the bridge.<o:p> </o:p><o:p> </o:p>So, a summary of the problems with this patch:<o:p> </o:p><o:p> </o:p>1) The race window is reduced (and may be gone in practical terms), but not guaranteed to be eliminated.<o:p> </o:p><o:p> </o:p>2) For the time during the "previous race window start" and "new race window start" a new problem has been created - if the new guest uses a Linux host bridge, the connection will fail<o:p> </o:p><o:p> </o:p>3) virNetDevGetMAC() will put an error in the system logs if the device doesn't exist (and it almost always will *not* exist, so this will be significant<o:p> </o:p><o:p> </o:p>4) Although it is almost always the case that two guests will not use the same MAC address for their network interfaces, there is nothing preventing it - we shouldn't assume that MAC addresses are unique. I think that check is actually superfluous, since the qemu process has always been terminated by the time we get to that place in the code, so the tap device should have been auto-deleted.<o:p> </o:p><o:p> </o:p>What do I think should be done? Good question. Possibly we could:<o:p> </o:p><o:p> </o:p>A) Call virNetDevTapReattachBridge() rather than virNetDevTapAttachBridge() in virNetDevTapCreateInBridgePort(). This would eliminate problem (2).<o:p> </o:p><o:p> </o:p>B) Instead of checking if the tap device MAC address matches, just call virNetDevExists() - if it exists, then skip the RemovePort() - this eliminates problems (3) and (4). (NB - this would fail if it turns out that tap device deletion isn't completed synchronously with qemu process termination!)<o:p> </o:p><o:p> </o:p>C) If we want to make it 100% sound, we need to make "check for interface existence + removeport" an atomic operation, and mutually exclusive with virNetDevTapCreate(). This would eliminate problem (1)<o:p> </o:p><o:p> </o:p><o:p> </o:p>><o:p> </o:p>>> + ignore_value(virNetDevOpenvswitchRemovePort(>> + virDomainNetGetActualBridgeName(net),>> + net->ifname));>> }>> }>><o:p> </o:p>><o:p> </o:p><o:p> </o:p><o:p> </o:p></div></body></html>