[v1] migration: Unlock mutex in error case

[PATCH] migration: Unlock mutex in error case

Posted by Juan Quintela 1 year ago

We were not unlocking bitmap mutex on the error case.
Coverity discovered the problem.

Fixes: a2326705e5 ("migration: Stop migration immediately in RDMA error paths")
Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 migration/ram.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/migration/ram.c b/migration/ram.c
index 34724e8fe8..8c4df60f29 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -3040,6 +3040,7 @@ static int ram_save_iterate(QEMUFile *f, void *opaque)
         ret = rdma_registration_start(f, RAM_CONTROL_ROUND);
         if (ret < 0) {
             qemu_file_set_error(f, ret);
+            qemu_mutex_unlock(&rs->bitmap_mutex);
             goto out;
         }
 
-- 
2.41.0

Re: [PATCH] migration: Unlock mutex in error case

Posted by Peter Maydell 1 year ago

On Thu, 2 Nov 2023 at 09:13, Juan Quintela <quintela@redhat.com> wrote:
>
> We were not unlocking bitmap mutex on the error case.
> Coverity discovered the problem.
>
> Fixes: a2326705e5 ("migration: Stop migration immediately in RDMA error paths")
> Signed-off-by: Juan Quintela <quintela@redhat.com>

CID 1523750.

thanks
-- PMM

Re: [PATCH] migration: Unlock mutex in error case

Posted by Alex Bennée 1 year ago

Juan Quintela <quintela@redhat.com> writes:

> We were not unlocking bitmap mutex on the error case.
> Coverity discovered the problem.
>
> Fixes: a2326705e5 ("migration: Stop migration immediately in RDMA error paths")
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> ---
>  migration/ram.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/migration/ram.c b/migration/ram.c
> index 34724e8fe8..8c4df60f29 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -3040,6 +3040,7 @@ static int ram_save_iterate(QEMUFile *f, void *opaque)
>          ret = rdma_registration_start(f, RAM_CONTROL_ROUND);
>          if (ret < 0) {
>              qemu_file_set_error(f, ret);
> +            qemu_mutex_unlock(&rs->bitmap_mutex);

I see the function uses the WITH_RCU_READ_LOCK_GUARD() macro to autofree
the RCU lock so why not use WITH_QEMU_LOCK_GUARD() instead of manually
checking the error cases?

>              goto out;
>          }

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

Re: [PATCH] migration: Unlock mutex in error case

Posted by Juan Quintela 1 year ago

Alex Bennée <alex.bennee@linaro.org> wrote:
> Juan Quintela <quintela@redhat.com> writes:
>
>> We were not unlocking bitmap mutex on the error case.
>> Coverity discovered the problem.
>>
>> Fixes: a2326705e5 ("migration: Stop migration immediately in RDMA error paths")
>> Signed-off-by: Juan Quintela <quintela@redhat.com>
>> ---
>>  migration/ram.c | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/migration/ram.c b/migration/ram.c
>> index 34724e8fe8..8c4df60f29 100644
>> --- a/migration/ram.c
>> +++ b/migration/ram.c
>> @@ -3040,6 +3040,7 @@ static int ram_save_iterate(QEMUFile *f, void *opaque)
>>          ret = rdma_registration_start(f, RAM_CONTROL_ROUND);
>>          if (ret < 0) {
>>              qemu_file_set_error(f, ret);
>> +            qemu_mutex_unlock(&rs->bitmap_mutex);
>
> I see the function uses the WITH_RCU_READ_LOCK_GUARD() macro to autofree
> the RCU lock so why not use WITH_QEMU_LOCK_GUARD() instead of manually
> checking the error cases?

You are right.

Changing to that.

Later, Juan.