When virNodeDeviceObjListRemove() is called, the passed
virNodeDeviceObj is removed from internal list of node devices
and then unrefed and unlocked. While the former is warranted (the
object was refed at the beginning of the function) the unlock is
not. In fact, it's wrong from conceptual POV. We still want
threads working on the object tu mutually exclude each other.

Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
---

v2 of:

https://listman.redhat.com/archives/libvir-list/2022-February/msg00036.html

diff to v1:
- Fixed test driver which worked around broken logic

Note, there is similar problem with virNodeDeviceObjListForEachRemove()
which removes objects from the list, even without lock, but that happens
only with mdevs and I have none to test with.

 src/conf/virnodedeviceobj.c | 2 +-
 src/test/test_driver.c      | 4 +---
 2 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/src/conf/virnodedeviceobj.c b/src/conf/virnodedeviceobj.c
index 2e4ef2df3c..7a560349d4 100644
--- a/src/conf/virnodedeviceobj.c
+++ b/src/conf/virnodedeviceobj.c
@@ -524,7 +524,7 @@ virNodeDeviceObjListRemove(virNodeDeviceObjList *devs,
     virObjectRWLockWrite(devs);
     virObjectLock(obj);
     virNodeDeviceObjListRemoveLocked(devs, obj);
-    virNodeDeviceObjEndAPI(&obj);
+    virObjectUnref(obj);
     virObjectRWUnlock(devs);
 }
 
diff --git a/src/test/test_driver.c b/src/test/test_driver.c
index 9e1fc65972..f900123941 100644
--- a/src/test/test_driver.c
+++ b/src/test/test_driver.c
@@ -6810,7 +6810,7 @@ testDestroyVport(testDriver *privconn,
                                            0);
 
     virNodeDeviceObjListRemove(privconn->devs, obj);
-    virObjectUnref(obj);
+    virNodeDeviceObjEndAPI(&obj);
 
     virObjectEventStateQueue(privconn->eventState, event);
     return 0;
@@ -7797,8 +7797,6 @@ testNodeDeviceDestroy(virNodeDevicePtr dev)
 
     virObjectLock(obj);
     virNodeDeviceObjListRemove(driver->devs, obj);
-    virObjectUnref(obj);
-    obj = NULL;
 
  cleanup:
     virNodeDeviceObjEndAPI(&obj);
-- 
2.34.1

On 2/2/22 06:00, Michal Privoznik wrote:
> When virNodeDeviceObjListRemove() is called, the passed
> virNodeDeviceObj is removed from internal list of node devices
> and then unrefed and unlocked. While the former is warranted (the
> object was refed at the beginning of the function) the unlock is
> not. In fact, it's wrong from conceptual POV. We still want
> threads working on the object tu mutually exclude each other.
> 
> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
> ---


Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>

> 
> v2 of:
> 
> https://listman.redhat.com/archives/libvir-list/2022-February/msg00036.html
> 
> diff to v1:
> - Fixed test driver which worked around broken logic
> 
> Note, there is similar problem with virNodeDeviceObjListForEachRemove()
> which removes objects from the list, even without lock, but that happens
> only with mdevs and I have none to test with.
> 
>   src/conf/virnodedeviceobj.c | 2 +-
>   src/test/test_driver.c      | 4 +---
>   2 files changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/src/conf/virnodedeviceobj.c b/src/conf/virnodedeviceobj.c
> index 2e4ef2df3c..7a560349d4 100644
> --- a/src/conf/virnodedeviceobj.c
> +++ b/src/conf/virnodedeviceobj.c
> @@ -524,7 +524,7 @@ virNodeDeviceObjListRemove(virNodeDeviceObjList *devs,
>       virObjectRWLockWrite(devs);
>       virObjectLock(obj);
>       virNodeDeviceObjListRemoveLocked(devs, obj);
> -    virNodeDeviceObjEndAPI(&obj);
> +    virObjectUnref(obj);
>       virObjectRWUnlock(devs);
>   }
>   
> diff --git a/src/test/test_driver.c b/src/test/test_driver.c
> index 9e1fc65972..f900123941 100644
> --- a/src/test/test_driver.c
> +++ b/src/test/test_driver.c
> @@ -6810,7 +6810,7 @@ testDestroyVport(testDriver *privconn,
>                                              0);
>   
>       virNodeDeviceObjListRemove(privconn->devs, obj);
> -    virObjectUnref(obj);
> +    virNodeDeviceObjEndAPI(&obj);
>   
>       virObjectEventStateQueue(privconn->eventState, event);
>       return 0;
> @@ -7797,8 +7797,6 @@ testNodeDeviceDestroy(virNodeDevicePtr dev)
>   
>       virObjectLock(obj);
>       virNodeDeviceObjListRemove(driver->devs, obj);
> -    virObjectUnref(obj);
> -    obj = NULL;
>   
>    cleanup:
>       virNodeDeviceObjEndAPI(&obj);