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 <laine@redhat.com>
---
 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

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 <laine@redhat.com>
> ---
>   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

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 <laine@redhat.com>
>> ---
>>   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...