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

[Christian Schoenebeck <qemu_oss@crudebyte.com>]

On Donnerstag, 20. August 2020 12:54:49 CEST Paolo Bonzini wrote:
> More on the practical side, recursive mutex are an easy way to get a
> deadlock.  It's a common idiom to do
> 
>     /* Need to take foo->lock outside bar->lock.  */
>     mutex_unlock(&bar->lock);
>     mutex_lock(&foo->lock);
>     mutex_lock(&bar->lock);

The general theoretical implications about recursive locks was clear to me. 
AFAICS your point is that a recursive lock could mislead poeple taking things 
easy and running into a deadlock scenario like outlined by you.

My point was if it happens for whatever reason that a main IO mutex lock was 
accidentally introduced, i.e. without knowing it was already locked on a 
higher level, wouldn't it make sense to deal with this in some kind of 
defensive way?

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.

> My suggestion is to work towards protecting the audio code with its own
> mutex(es) and ignore the existence of the BQL for subsystems that can do
> so (audio is a prime candidate).  Also please add comments to
> audio_int.h about which functions are called from other threads than the
> QEMU main thread.

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.

Best regards,
Christian Schoenebeck