[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

[libvirt PATCH v2 0/2] Eliminate old tap/macvtap teardown stomping on new tap setup



V1 is here: https://www.redhat.com/archives/libvir-list/2020-August/msg00756.html

The problem and this solution are very well described in patches 2 and
3, but in short - because we (libvirt for macvtap, the kernel for tap)
always try to assign the lowest numbered names possible to macvtap and
tap devices, we sometimes create a new tap for a new guest using the
same name as an old tap for an old guest that is shutting down
simultaneous to setting up the new guest/tap. This can lead to the old
guest teardown stomping on the new guest setup.

This is the problem that the authors were attempting to solve in these
two patches sent earlier in the summer:

  https://www.redhat.com/archives/libvir-list/2020-June/msg00481.html
  https://www.redhat.com/archives/libvir-list/2020-June/msg00525.html

and also in this V2 patch, which Bingsong Si sent in response to my
poorly-thought-out advice in my response to his original patch:
  
  https://www.redhat.com/archives/libvir-list/2020-June/msg00755.html

Somewhere during that discussion, danpb suggested that in order to
*really* solve the problem, we should use our own counter for
auto-generated tap device names (instead of relying on the kernel) and
just never re-use a name until the counter rolls over. That's
essentially what these two patches do.

One possibly undesirable side effect of this (and the other) patch is
that the longer a host is running without reboot, the higher the
numbers tap device names will get. While users are accustomed to
always seeing vnet0 and vnet1, they may be a bit surprised to now see
vnet39283 or macvtap735. It has been pointed out to me (again by
danpb) that the same thing happened with PIDs a few years ago, and
while it looked strange at first, everyone is now accustomed to it.

Changes from V1:

Patch 1 from V1 was removed - everything it changed is now
removed/replaced in the new Patch 1 (which was Patch 2 in V1). And so
of course, what was Patch 3 in V1 is now Patch 2 in V2.

I eliminated the old bitmap reservation system in the macvtap patch
(Patch 1) rather than adding on top of it as I had in V1 - it really
was beyond redundant and unnecessary, and just clouded up the whole
situation. This also allowed me to get rid of the 8192 limit (which
was there only to limit the size of the virBitmap, which we no longer
need), and allow the device names to count up until they overflow
either the ifname[IFNAMSIZ] buffer, or reach INT_MAX.

Likewise, I modified the standard tap patch to remove the artificial
maximum of 99999, and just let it count up until it overflows.

(Jano suggested that I should have a test case to test the entire
range, but I don't think anyone would be happy with that. If I was
masochistic and wanted to mock a bunch of virNetDev functions I could
artificially test it by bumping up the counters with calls to the
virNetDevTapReserveName() function, but it's 1AM. I did test the
rollover of both cases (macvtap, where it overflows the buffer size
first, and standard tap where it overflow the 32 bit int first) with a
one-off build that started the counter just a few below the overflow
point, and it does work correctly.)


Laine Stump (2):
  util: replace macvtap name reservation bitmap with a simple counter
  util: assign tap device names using a monotonically increasing integer

 src/libvirt_private.syms    |   2 +-
 src/libxl/libxl_driver.c    |   2 +-
 src/lxc/lxc_process.c       |   2 +-
 src/qemu/qemu_process.c     |  22 +-
 src/util/virnetdevmacvlan.c | 402 +++++++++++++-----------------------
 src/util/virnetdevmacvlan.h |   6 +-
 src/util/virnetdevtap.c     | 108 +++++++++-
 src/util/virnetdevtap.h     |   4 +
 8 files changed, 275 insertions(+), 273 deletions(-)

-- 
2.26.2


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]