Re: [RFC v4 2/3] memory: add depth assert in address_space_to_flatview

[Peter Xu <peterx@redhat.com>]

Hi, Paolo,

On Wed, Dec 28, 2022 at 09:27:50AM +0100, Paolo Bonzini wrote:
> Il ven 23 dic 2022, 16:54 Peter Xu <peterx@redhat.com> ha scritto:
> 
> > > This is not valid because the transaction could happen in *another*
> > thread.
> > > In that case memory_region_transaction_depth() will be > 0, but RCU is
> > > needed.
> >
> > Do you mean the code is wrong, or the comment?  Note that the code has
> > checked rcu_read_locked() where introduced in patch 1, but maybe something
> > else was missed?
> >
> 
> The assertion is wrong. It will succeed even if RCU is unlocked in this
> thread but a transaction is in progress in another thread.

IIUC this is the case where the context:

  (1) doesn't have RCU read lock held, and,
  (2) doesn't have BQL held.

Is it safe at all to reference any flatview in such a context?  The thing
is I think the flatview pointer can be freed anytime if both locks are not
taken.

> Perhaps you can check (memory_region_transaction_depth() > 0 &&
> !qemu_mutex_iothread_locked()) || rcu_read_locked() instead?

What if one thread calls address_space_to_flatview() with BQL held but not
RCU read lock held?  I assume it's a legal operation, but it seems to be
able to trigger the assert already?

Thanks,

-- 
Peter Xu