Re: [PATCH v5 1/1] audio/jack: fix use after free segfault

[Paolo Bonzini]

On 20/08/20 14:00, Christian Schoenebeck wrote:
> One way would be a recursive type and logging a warning, which you obviously 
> don't like; another option would be an assertion fault instead to make 
> developers immediately aware about the double lock on early testing. Because 
> on a large scale project like this, it is almost impossible for all 
> developers 
> to be aware about all implied locks. Don't you think so?
> 
> At least IMO the worst case would be a double unlock on a non-recursive main 
> thread mutex and running silently into undefined behaviour.

Yes, more assertions are always fine.

We were using errorcheck mutexes until a few years ago, unfortunately we
couldn't because they are broken with respect to fork (commit 24fa90499,
"qemu-thread: do not use PTHREAD_MUTEX_ERRORCHECK", 2015-03-05).

> That main thread lock came up here because I noticed this API comment on 
> qemu_bh_cancel():
> 
>   "While cancellation itself is also wait-free and thread-safe, it can of     
>     
>    course race with the loop that executes bottom halves unless you are 
>    holding the iothread mutex.  This makes it mostly useless if you are not 
>    holding the mutex."
> 
> So this lock was not about driver internal data protection, but rather about 
> dealing with the BH API correctly.

Yes, on the other hand that is not a problem if the BH is idempotent.
For example something like this is okay:

bh_body_locked()
{
        free(foo);
        foo = NULL;
}

bh_body()
{
        qemu_mutex_lock(&foo_lock);
        bh_body_locked();
        qemu_mutex_unlock(&foo_lock);
}

...

        qemu_mutex_lock(&foo_lock);
        qemu_bh_delete(foo_bh);         // also calls qemu_bh_cancel
        bh_body_locked();
        qemu_mutex_unlock(&foo_lock);

Paolo