[PATCH] network: Fix a race condition when shutdown & start vm at the same time
when shutdown vm, the qemuProcessStop cleanup virtual interface in two steps: 1. qemuProcessKill kill qemu process, and vif disappeared 2. ovs-vsctl del-port from the brige if start a vm in the middle of the two steps, the new vm will reused the vif, but removed from bridge by step 2 Signed-off-by: Bingsong Si <owen.si@ucloud.cn> --- src/qemu/qemu_process.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) 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)) + ignore_value(virNetDevOpenvswitchRemovePort( + virDomainNetGetActualBridgeName(net), + net->ifname)); } } -- 2.18.2
On 6/11/20 6:58 AM, Bingsong Si wrote:
when shutdown vm, the qemuProcessStop cleanup virtual interface in two steps:
s/when/When
1. qemuProcessKill kill qemu process, and vif disappeared 2. ovs-vsctl del-port from the brige
if start a vm in the middle of the two steps, the new vm will reused the vif,
s/if/If
but removed from bridge by step 2
Signed-off-by: Bingsong Si <owen.si@ucloud.cn> --- src/qemu/qemu_process.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-)
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))
Extra space between "||" and "!virMacAddrCmp(.." With these nits fixed: Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
+ ignore_value(virNetDevOpenvswitchRemovePort( + virDomainNetGetActualBridgeName(net), + net->ifname)); } }
Thanks for your review, I will fix in version 2. 发件人: Daniel Henrique Barboza 发送时间: 2020年6月16日星期二 上午2:36 收件人: Bingsong Si; libvir-list@redhat.com 主题: Re: [PATCH] network: Fix a race condition when shutdown & start vm at the same time On 6/11/20 6:58 AM, Bingsong Si wrote:
when shutdown vm, the qemuProcessStop cleanup virtual interface in two steps:
s/when/When
1. qemuProcessKill kill qemu process, and vif disappeared 2. ovs-vsctl del-port from the brige
if start a vm in the middle of the two steps, the new vm will reused the vif,
s/if/If
but removed from bridge by step 2
Signed-off-by: Bingsong Si <owen.si@ucloud.cn> --- src/qemu/qemu_process.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-)
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))
Extra space between "||" and "!virMacAddrCmp(.." With these nits fixed: Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
+ ignore_value(virNetDevOpenvswitchRemovePort( + virDomainNetGetActualBridgeName(net), + net->ifname)); } }
(BTW, this other patch is also trying to solve the same problem: https://www.redhat.com/archives/libvir-list/2020-June/msg00525.html I made comments there earlier, and have learned a bit more since then: https://www.redhat.com/archives/libvir-list/2020-June/msg00634.html On 6/15/20 2:36 PM, Daniel Henrique Barboza wrote:
On 6/11/20 6:58 AM, Bingsong Si wrote:
when shutdown vm, the qemuProcessStop cleanup virtual interface in two steps:
s/when/When
1. qemuProcessKill kill qemu process, and vif disappeared 2. ovs-vsctl del-port from the brige
if start a vm in the middle of the two steps, the new vm will reused the vif,
s/if/If
but removed from bridge by step 2
Signed-off-by: Bingsong Si <owen.si@ucloud.cn> --- src/qemu/qemu_process.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-)
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))
(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).
Extra space between "||" and "!virMacAddrCmp(.."
With these nits fixed:
Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
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: 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 A2) the tap's associated port (also "vnet0") is removed from the OVS switch by libvirt. While libvirt thread B is doing this: 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". B2) the new tap device is attached to an OVS switch (or possibly a Linux host bridge). 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. 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. 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. However, this just creates a smaller window for the race, and different problem for the remainder of the time. 1) smaller window - it would still be possible for the following to happen: a) old qemu terminates, tap device "vnet0" is deleted b) libvirt checks MAC address and learns that there is no device "vnet0", so it calls virNetDevOpenvswitchRemovePort(), but before ovs-vsctl can be called... c) libvirt creates a new tap device for new QEMU, kernel names it "vnet0" d) libvirt calls virNetDevOpenvswitchAddPort() and the new tap device "vnet0" to the switch e) the ovs-vsctl from (b) is finally able to run, and removes "vnet0" from the switch. Granted, this is *highly* unlikely, since there is nothing extra between checking MAC address and removing the port, but there is nothing enforcing it. 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. 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. So, a summary of the problems with this patch: 1) The race window is reduced (and may be gone in practical terms), but not guaranteed to be eliminated. 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 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 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. What do I think should be done? Good question. Possibly we could: A) Call virNetDevTapReattachBridge() rather than virNetDevTapAttachBridge() in virNetDevTapCreateInBridgePort(). This would eliminate problem (2). 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!) 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)
+ ignore_value(virNetDevOpenvswitchRemovePort( + virDomainNetGetActualBridgeName(net), + net->ifname)); } }
Thanks for your detailed analysis, I will remake a patch 发件人: Laine Stump 发送时间: Wednesday, June 17, 2020 7:46 AM 收件人: libvir-list@redhat.com 抄送: Daniel Henrique Barboza; Bingsong Si; Wei Gong 主题: Re: [PATCH] network: Fix a race condition when shutdown & start vm at the same time (BTW, this other patch is also trying to solve the same problem: https://www.redhat.com/archives/libvir-list/2020-June/msg00525.html I made comments there earlier, and have learned a bit more since then: https://www.redhat.com/archives/libvir-list/2020-June/msg00634.html On 6/15/20 2:36 PM, Daniel Henrique Barboza wrote:
On 6/11/20 6:58 AM, Bingsong Si wrote:
when shutdown vm, the qemuProcessStop cleanup virtual interface in two steps:
s/when/When
1. qemuProcessKill kill qemu process, and vif disappeared 2. ovs-vsctl del-port from the brige
if start a vm in the middle of the two steps, the new vm will reused the vif,
s/if/If
but removed from bridge by step 2
Signed-off-by: Bingsong Si <owen.si@ucloud.cn> --- src/qemu/qemu_process.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-)
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))
(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).
Extra space between "||" and "!virMacAddrCmp(.."
With these nits fixed:
Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
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: 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 A2) the tap's associated port (also "vnet0") is removed from the OVS switch by libvirt. While libvirt thread B is doing this: 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". B2) the new tap device is attached to an OVS switch (or possibly a Linux host bridge). 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. 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. 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. However, this just creates a smaller window for the race, and different problem for the remainder of the time. 1) smaller window - it would still be possible for the following to happen: a) old qemu terminates, tap device "vnet0" is deleted b) libvirt checks MAC address and learns that there is no device "vnet0", so it calls virNetDevOpenvswitchRemovePort(), but before ovs-vsctl can be called... c) libvirt creates a new tap device for new QEMU, kernel names it "vnet0" d) libvirt calls virNetDevOpenvswitchAddPort() and the new tap device "vnet0" to the switch e) the ovs-vsctl from (b) is finally able to run, and removes "vnet0" from the switch. Granted, this is *highly* unlikely, since there is nothing extra between checking MAC address and removing the port, but there is nothing enforcing it. 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. 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. So, a summary of the problems with this patch: 1) The race window is reduced (and may be gone in practical terms), but not guaranteed to be eliminated. 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 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 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. What do I think should be done? Good question. Possibly we could: A) Call virNetDevTapReattachBridge() rather than virNetDevTapAttachBridge() in virNetDevTapCreateInBridgePort(). This would eliminate problem (2). 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!) 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)
+ ignore_value(virNetDevOpenvswitchRemovePort( + virDomainNetGetActualBridgeName(net), + net->ifname)); } }
On 6/17/20 1:46 AM, Laine Stump wrote:
What do I think should be done? Good question. Possibly we could:
A) Call virNetDevTapReattachBridge() rather than virNetDevTapAttachBridge() in virNetDevTapCreateInBridgePort(). This would eliminate problem (2).
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!)
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)
I still don't quite understand how there can be a race. I mean, from system POV, libvirt creates a TAP, plugs it into a bridge (when starting the first domain). And when shutting it down and starting the second domain in parallel a new TAP device (with different index and MAC) is created independent of the first TAP, no? Michal
On 6/19/20 12:00 PM, Michal Privoznik wrote:
On 6/17/20 1:46 AM, Laine Stump wrote:
What do I think should be done? Good question. Possibly we could:
A) Call virNetDevTapReattachBridge() rather than virNetDevTapAttachBridge() in virNetDevTapCreateInBridgePort(). This would eliminate problem (2).
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!)
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)
I still don't quite understand how there can be a race. I mean, from system POV, libvirt creates a TAP, plugs it into a bridge (when starting the first domain). And when shutting it down and starting the second domain in parallel a new TAP device (with different index and MAC) is created independent of the first TAP, no?
Yes. There's no problem with the tap device creation/deletion by itself. The problem is when that tap device is attached to an OVS switch. A "port" of the same name as the tap device is created in OVS, and that port continues to exist after the tap device itself is deleted. If a new tap device of the same name is created without first deleting the port from OVS, then the new tap device is automatically connected to the existing port of the same name (I don't know the exact details of how this is done, but OVS must have it's claws somewhere in the kernel or udev to make this happen, as I've seen it (and can reproduce it) myself). Even that, by itself, isn't a problem. But if you have a tap device attached to an OVS port, and you delete the tap, that frees up the name so that the next time someone asks to create a tap device they get that same name. But if that happens before the old port has been removed from OVS (which is a separate operation), then the new tap device is attached to the old switch. In that case, simple requests to attach the new tap to a different switch (or a Linux bridge) will fail. We solve that problem when the new tap is being attached to an OVS switch by prepending "--if-exists del-port blah" to the ovs-vsctl command that attaches to the new switch, but that doesn't help in the case that the new tap is being attached to a Linux host bridge. Performing items (1) and (2) in my list will probably get rid of all bad behavior in practice, but there is still technically the possibility that the thread (B) for the new tap device will create that tap device after the thread (A) for the old tap device checks that the device doesn't exist (i.e. that a new tap with the same name hasn't yet been created) but before removing the port (of the same name) from the OVS switch. This would all be much simpler if the kernel would put a "FIN WAIT" state on all tap device names so that they couldn't be re-used for a few seconds after deletion, but it doesn't, so we have to work with what we've got.
On Fri, Jun 19, 2020 at 12:19:05PM -0400, Laine Stump wrote:
This would all be much simpler if the kernel would put a "FIN WAIT" state on all tap device names so that they couldn't be re-used for a few seconds after deletion, but it doesn't, so we have to work with what we've got.
The problem is that the kernel reuses tap device names. Can we just take the kernel out of the equation and do auto-assignment of names ourselves. Maintain a global "int nextTapID" counter, and just iterate on this. NIC names can be upto 16 bytes, so we'll create billions of devices before we have any chance of wrapping around and risking a collision with a concurrently shutting down guest. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On 6/19/20 12:26 PM, Daniel P. Berrangé wrote:
On Fri, Jun 19, 2020 at 12:19:05PM -0400, Laine Stump wrote:
This would all be much simpler if the kernel would put a "FIN WAIT" state on all tap device names so that they couldn't be re-used for a few seconds after deletion, but it doesn't, so we have to work with what we've got. The problem is that the kernel reuses tap device names.
Can we just take the kernel out of the equation and do auto-assignment of names ourselves.
Yeah, that's what we do for macvtap. It is extra code, non-trivial, and therefore prone to errors (as in, "anything over 1 line long is prone to errors"). I *thought* I had remembered that couldn't work properly for standard tap devices couldn't work due to the way that tap devices are "created" (you first open /dev/net/tun, then issue a ioctl(TUNSETIFF, "desiredname") on the handle returned by open. I had been concerned that if you did that with an existing name it would just connect you to the existing tap, but I tried it just now and got an error the 2nd time I tried to use the same tap device name, so perhaps/probably I'm again misremembering.
Maintain a global "int nextTapID" counter, and just iterate on this. NIC names can be upto 16 bytes, so we'll create billions of devices before we have any chance of wrapping around and risking a collision with a concurrently shutting down guest.
I remember I tried doing a monotonically increasing "next" counter for macvtap (never actually pushed, but shown to [some users, I forget who, maybe ovirt devs?], and they didn't like it because the devices ended up with long difficult-for-a-human-to-remember/pick-out/recite names like macvtap42301 and macvtap43021. So instead we keep track of the names that are in use, and always look for the lowest available number when creating a new one. (of course doing that would greatly increase the likelyhood of exposing any race conditions, so...) Definitely if we change the behavior in this way we're going to hear about it, though :-) Another issue I remember from macvtap is the possibility of a different process (not libvirt) having already created a macvtap device with the name that we are wanting to use. That is easily solved in the case of macvtap by just iterating through until we find one that we can create without failure. So basically you have the list/bitmap/whatever of devicenames currently in use, use that as a starting point to pick a name for the new device, but then also allow for that name to already be used, and retry. Note that since macvtap and standard tap devices (and, actually *all* network devices) in the same namespace need to not have conflicting names, we could keep a single list of in-use names. Since we would never auto-generate a tap device name that conflicts with an auto-generated macvtap name, that is probably unnecessary, but just in case it makes things easier... Also, in the name of backward compatibility, we will need to support tap device names from the config that have %d in the name, e.g. the default name we send is "vnet%d", but we allow for someone to specify "mytap%d". In the end, if we can do something simple that preserves current behavior while covering the hole, that would probably be more expedient, but I have to say the thought of naming the devices ourselves has crossed my mind.
On Fri, Jun 19, 2020 at 01:06:33PM -0400, Laine Stump wrote:
On 6/19/20 12:26 PM, Daniel P. Berrangé wrote:
Maintain a global "int nextTapID" counter, and just iterate on this. NIC names can be upto 16 bytes, so we'll create billions of devices before we have any chance of wrapping around and risking a collision with a concurrently shutting down guest.
I remember I tried doing a monotonically increasing "next" counter for macvtap (never actually pushed, but shown to [some users, I forget who, maybe ovirt devs?], and they didn't like it because the devices ended up with long difficult-for-a-human-to-remember/pick-out/recite names like macvtap42301 and macvtap43021. So instead we keep track of the names that are in use, and always look for the lowest available number when creating a new one. (of course doing that would greatly increase the likelyhood of exposing any race conditions, so...) Definitely if we change the behavior in this way we're going to hear about it, though :-)
People might complain, but I'm not convinced it really matters. Pid numbers used to be nice & short until someone raised pid max and now my processes have 7-digit long pids. It is surprising at first, but it didn't really cause me any functional problems. And there's still the option of just providing a fixed name if you really need something predictable for a guest.
Another issue I remember from macvtap is the possibility of a different process (not libvirt) having already created a macvtap device with the name that we are wanting to use. That is easily solved in the case of macvtap by just iterating through until we find one that we can create without failure. So basically you have the list/bitmap/whatever of devicenames currently in use, use that as a starting point to pick a name for the new device, but then also allow for that name to already be used, and retry.
I wasn't thinking we need a bitmap, literally just a single counter. We start where we left off, trying until we succeeed, which is what the kernel does anyway IIRC. Most other competing processes will rely on the kernel auto-allocation so won't clash with us very frequently if at all.
Also, in the name of backward compatibility, we will need to support tap device names from the config that have %d in the name, e.g. the default name we send is "vnet%d", but we allow for someone to specify "mytap%d".
We could keep a counter per prefix, or just have a single global counter. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On 6/19/20 1:16 PM, Daniel P. Berrangé wrote:
On Fri, Jun 19, 2020 at 01:06:33PM -0400, Laine Stump wrote:
On 6/19/20 12:26 PM, Daniel P. Berrangé wrote:
Maintain a global "int nextTapID" counter, and just iterate on this. NIC names can be upto 16 bytes, so we'll create billions of devices before we have any chance of wrapping around and risking a collision with a concurrently shutting down guest.
I remember I tried doing a monotonically increasing "next" counter for macvtap (never actually pushed, but shown to [some users, I forget who, maybe ovirt devs?], and they didn't like it because the devices ended up with long difficult-for-a-human-to-remember/pick-out/recite names like macvtap42301 and macvtap43021. So instead we keep track of the names that are in use, and always look for the lowest available number when creating a new one. (of course doing that would greatly increase the likelyhood of exposing any race conditions, so...) Definitely if we change the behavior in this way we're going to hear about it, though :-) People might complain, but I'm not convinced it really matters. Pid numbers used to be nice & short until someone raised pid max and now my processes have 7-digit long pids. It is surprising at first, but it didn't really cause me any functional problems.
This is a good point. But even more convincing than that is the revelation that this isn't an issue just with OVS switch ports - the same race is also causing problems with libvirt nwfilter driver bindings, as reported here: https://bugzilla.redhat.com/show_bug.cgi?id=1837395 After seeing that report and realizing that this same race is the cause, I've decided to change libvirt's tap device creation to provide names based on a monotonically increasing counter as you suggest, rather than relying on the kernel. I'm hoping to have something to send to the list in a day or two.
And there's still the option of just providing a fixed name if you really need something predictable for a guest.
Another issue I remember from macvtap is the possibility of a different process (not libvirt) having already created a macvtap device with the name that we are wanting to use. That is easily solved in the case of macvtap by just iterating through until we find one that we can create without failure. So basically you have the list/bitmap/whatever of devicenames currently in use, use that as a starting point to pick a name for the new device, but then also allow for that name to already be used, and retry. I wasn't thinking we need a bitmap, literally just a single counter. We start where we left off, trying until we succeeed, which is what the kernel does anyway IIRC. Most other competing processes will rely on the kernel auto-allocation so won't clash with us very frequently if at all.
Also, in the name of backward compatibility, we will need to support tap device names from the config that have %d in the name, e.g. the default name we send is "vnet%d", but we allow for someone to specify "mytap%d". We could keep a counter per prefix, or just have a single global counter.
Regards, Daniel
participants (6)
-
Bingsong Si -
Daniel Henrique Barboza -
Daniel P. Berrangé -
Laine Stump -
Michal Privoznik -
owen.si