Re: [libvirt PATCH 1/3] util: make locking versions of virNetDevMacVLan(Reserve|Release)Name()
On 8/24/20 6:23 AM, Michal Privoznik wrote:
On 8/24/20 6:23 AM, Laine Stump wrote:
When these functions are called from within virnetdevmacvlan.c, they
are usually called with virNetDevMacVLanCreateMutex held, but when
virNetDevMacVLanReserveName() is called from other places (hypervisor
drivers keeping track of already-in-use macvlan/macvtap devices) the
lock isn't acquired. This could lead to a situation where one thread
is setting a bit in the bitmap to notify of a device already in-use,
while another thread is checking/setting/clearing a bit while creating
a new macvtap device.
In practice this *probably* doesn't happen, because the external calls
to virNetDevMacVLan() only happen during hypervisor driver init
routines when libvirtd is restarted, but there's no harm in protecting
ourselves.
(NB: virNetDevMacVLanReleaseName() is actually never called from
outside virnetdevmacvlan.c, so it could just as well be static, but
I'm leaving it as-is for now. This locking version *is* called
from within virnetdevmacvlan.c, since there are a couple places that
we used to call the unlocked version after the lock was already
released.)
Signed-off-by: Laine Stump
---
src/util/virnetdevmacvlan.c | 42 ++---
1 file changed, 34 insertions(+), 8 deletions(-)
diff --git a/src/util/virnetdevmacvlan.c b/src/util/virnetdevmacvlan.c
index dcea93a5fe..69a9c784bb 100644
--- a/src/util/virnetdevmacvlan.c
+++ b/src/util/virnetdevmacvlan.c
@@ -196,7 +196,7 @@ virNetDevMacVLanReleaseID(int id, unsigned int flags)
/**
- * virNetDevMacVLanReserveName:
+ * virNetDevMacVLanReserveNameInternal:
*
* @name: already-known name of device
* @quietFail: don't log an error if this name is already in-use
@@ -208,8 +208,8 @@ virNetDevMacVLanReleaseID(int id, unsigned int flags)
* Returns reserved ID# on success, -1 on failure, -2 if the name
* doesn't fit the auto-pattern (so not reserveable).
*/
-int
-virNetDevMacVLanReserveName(const char *name, bool quietFail)
+static int
+virNetDevMacVLanReserveNameInternal(const char *name, bool quietFail)
{
unsigned int id;
unsigned int flags = 0;
@@ -237,8 +237,21 @@ virNetDevMacVLanReserveName(const char *name,
bool quietFail)
}
+int
+virNetDevMacVLanReserveName(const char *name, bool quietFail)
+{
+ /* Call the internal function after locking the macvlan mutex */
+ int ret;
+
+ virMutexLock(&virNetDevMacVLanCreateMutex);
+ ret = virNetDevMacVLanReserveNameInternal(name, quietFail);
+ virMutexUnlock(&virNetDevMacVLanCreateMutex);
+ return ret;
+}
Hopefully, we won't use any of these in a forked off process because
these are not async-signal safe anymore.
Interesting point (not because I think it could happen in this case, but
because I hadn't even been thinking about it when I added to the mutex
usage (and created a new mutex in the next patch)).
But of course this could be said for any code that uses a mutex (and in
this case, even without the mutex we can't use the global counter in a
forked off process and expect to get unique numbers).
I wonder if there's a way a static code checker could verify that
certain bits of code can never be in the call chain in a forked process...
Re: [libvirt PATCH 1/3] util: make locking versions of virNetDevMacVLan(Reserve|Release)Name()
On 8/24/20 6:23 AM, Laine Stump wrote:
When these functions are called from within virnetdevmacvlan.c, they
are usually called with virNetDevMacVLanCreateMutex held, but when
virNetDevMacVLanReserveName() is called from other places (hypervisor
drivers keeping track of already-in-use macvlan/macvtap devices) the
lock isn't acquired. This could lead to a situation where one thread
is setting a bit in the bitmap to notify of a device already in-use,
while another thread is checking/setting/clearing a bit while creating
a new macvtap device.
In practice this *probably* doesn't happen, because the external calls
to virNetDevMacVLan() only happen during hypervisor driver init
routines when libvirtd is restarted, but there's no harm in protecting
ourselves.
(NB: virNetDevMacVLanReleaseName() is actually never called from
outside virnetdevmacvlan.c, so it could just as well be static, but
I'm leaving it as-is for now. This locking version *is* called
from within virnetdevmacvlan.c, since there are a couple places that
we used to call the unlocked version after the lock was already
released.)
Signed-off-by: Laine Stump
---
src/util/virnetdevmacvlan.c | 42 ++---
1 file changed, 34 insertions(+), 8 deletions(-)
diff --git a/src/util/virnetdevmacvlan.c b/src/util/virnetdevmacvlan.c
index dcea93a5fe..69a9c784bb 100644
--- a/src/util/virnetdevmacvlan.c
+++ b/src/util/virnetdevmacvlan.c
@@ -196,7 +196,7 @@ virNetDevMacVLanReleaseID(int id, unsigned int flags)
/**
- * virNetDevMacVLanReserveName:
+ * virNetDevMacVLanReserveNameInternal:
*
* @name: already-known name of device
* @quietFail: don't log an error if this name is already in-use
@@ -208,8 +208,8 @@ virNetDevMacVLanReleaseID(int id, unsigned int flags)
* Returns reserved ID# on success, -1 on failure, -2 if the name
* doesn't fit the auto-pattern (so not reserveable).
*/
-int
-virNetDevMacVLanReserveName(const char *name, bool quietFail)
+static int
+virNetDevMacVLanReserveNameInternal(const char *name, bool quietFail)
{
unsigned int id;
unsigned int flags = 0;
@@ -237,8 +237,21 @@ virNetDevMacVLanReserveName(const char *name, bool
quietFail)
}
+int
+virNetDevMacVLanReserveName(const char *name, bool quietFail)
+{
+/* Call the internal function after locking the macvlan mutex */
+int ret;
+
+virMutexLock(&virNetDevMacVLanCreateMutex);
+ret = virNetDevMacVLanReserveNameInternal(name, quietFail);
+virMutexUnlock(&virNetDevMacVLanCreateMutex);
+return ret;
+}
Hopefully, we won't use any of these in a forked off process because
these are not async-signal safe anymore.
Michal
[libvirt PATCH 1/3] util: make locking versions of virNetDevMacVLan(Reserve|Release)Name()
When these functions are called from within virnetdevmacvlan.c, they
are usually called with virNetDevMacVLanCreateMutex held, but when
virNetDevMacVLanReserveName() is called from other places (hypervisor
drivers keeping track of already-in-use macvlan/macvtap devices) the
lock isn't acquired. This could lead to a situation where one thread
is setting a bit in the bitmap to notify of a device already in-use,
while another thread is checking/setting/clearing a bit while creating
a new macvtap device.
In practice this *probably* doesn't happen, because the external calls
to virNetDevMacVLan() only happen during hypervisor driver init
routines when libvirtd is restarted, but there's no harm in protecting
ourselves.
(NB: virNetDevMacVLanReleaseName() is actually never called from
outside virnetdevmacvlan.c, so it could just as well be static, but
I'm leaving it as-is for now. This locking version *is* called
from within virnetdevmacvlan.c, since there are a couple places that
we used to call the unlocked version after the lock was already
released.)
Signed-off-by: Laine Stump
---
src/util/virnetdevmacvlan.c | 42 ++---
1 file changed, 34 insertions(+), 8 deletions(-)
diff --git a/src/util/virnetdevmacvlan.c b/src/util/virnetdevmacvlan.c
index dcea93a5fe..69a9c784bb 100644
--- a/src/util/virnetdevmacvlan.c
+++ b/src/util/virnetdevmacvlan.c
@@ -196,7 +196,7 @@ virNetDevMacVLanReleaseID(int id, unsigned int flags)
/**
- * virNetDevMacVLanReserveName:
+ * virNetDevMacVLanReserveNameInternal:
*
* @name: already-known name of device
* @quietFail: don't log an error if this name is already in-use
@@ -208,8 +208,8 @@ virNetDevMacVLanReleaseID(int id, unsigned int flags)
* Returns reserved ID# on success, -1 on failure, -2 if the name
* doesn't fit the auto-pattern (so not reserveable).
*/
-int
-virNetDevMacVLanReserveName(const char *name, bool quietFail)
+static int
+virNetDevMacVLanReserveNameInternal(const char *name, bool quietFail)
{
unsigned int id;
unsigned int flags = 0;
@@ -237,8 +237,21 @@ virNetDevMacVLanReserveName(const char *name, bool
quietFail)
}
+int
+virNetDevMacVLanReserveName(const char *name, bool quietFail)
+{
+/* Call the internal function after locking the macvlan mutex */
+int ret;
+
+virMutexLock(&virNetDevMacVLanCreateMutex);
+ret = virNetDevMacVLanReserveNameInternal(name, quietFail);
+virMutexUnlock(&virNetDevMacVLanCreateMutex);
+return ret;
+}
+
+
/**
- * virNetDevMacVLanReleaseName:
+ * virNetDevMacVLanReleaseNameInternal:
*
* @name: already-known name of device
*
@@ -248,8 +261,8 @@ virNetDevMacVLanReserveName(const char *name, bool
quietFail)
*
* returns 0 on success, -1 on failure
*/
-int
-virNetDevMacVLanReleaseName(const char *name)
+static int
+virNetDevMacVLanReleaseNameInternal(const char *name)
{
unsigned int id;
unsigned int flags = 0;
@@ -277,6 +290,19 @@ virNetDevMacVLanReleaseName(const char *name)
}
+int
+virNetDevMacVLanReleaseName(const char *name)
+{
+/* Call the internal function after locking the macvlan mutex */
+int ret;
+
+virMutexLock(&virNetDevMacVLanCreateMutex);
+ret = virNetDevMacVLanReleaseNameInternal(name);
+virMutexUnlock(&virNetDevMacVLanCreateMutex);
+return ret;
+}
+
+
/**
* virNetDevMacVLanIsMacvtap:
* @ifname: Name of the interface
@@ -967,7 +993,7 @@ virNetDevMacVLanCreateWithVPortProfile(const char
*ifnameRequested,
return -1;
}
if (isAutoName &&
-(reservedID = virNetDevMacVLanReserveName(ifnameRequested, true))
< 0) {
+(reservedID = virNetDevMacVLanReserveNameInternal(ifnameRequested,
true)) < 0) {
reservedID = -1;
goto create_name;
}
@@ -975,7 +1001,7 @@ virNetDevMacVLanCreateWithVPortProfile(const char
*ifnameRequested,
if (virNetDevMacVLanCreate(ifnameRequested, type, macaddress,
linkdev, macvtapMode, &do_retry) < 0) {
if (isAutoName) {
-virNetDevMacVLanReleaseName(ifnameRequested);
+virNetDevMacVLanReleaseNameInternal(ifnameRequested);
reservedID = -1;
goto create_name;
}
--
2.26.2
